Move .po and .ts files look-up to build-time
ClosedPublic

Authored by apol on Mar 24 2017, 6:08 PM.

Details

Summary

As specified in cmake's documentation, file(GLOB) is not recommended for looking
up sources because it requires cmake to be regenerated to be able to process
what needs to be executed.
This patch moves this process into the compilation time where we'll combine
the look-up and the generation.

Test Plan

Tested on a project with only po files, will need testing on projects
that do include scripts.

Diff Detail

Repository
R249 KI18n
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Mar 24 2017, 6:08 PM
Restricted Application added a project: Frameworks. Β· View Herald TranscriptMar 24 2017, 6:08 PM
apol added a reviewer: ilic.Mar 24 2017, 6:10 PM
aacid added a subscriber: aacid.Mar 26 2017, 10:05 PM

Have you tried this?

That commented out test doesn't seem like a good idea.

Tried it in kde-l10n-sr for example

s/Tried/try

apol updated this revision to Diff 12862.Mar 27 2017, 3:37 PM

Uncomment test, remove unneeded glob

apol added a comment.Mar 27 2017, 3:53 PM

This is KAlgebra in catalan with the translations generated with this method:

If I use LANGUAGE=sr it doesn't work, I don't know why though, it doesn't error but it's likely I'm messing up somewhere.

apol updated this revision to Diff 12867.Mar 27 2017, 3:56 PM

Install in the correct directory

apol updated this revision to Diff 12930.Mar 28 2017, 5:06 PM

Fix several issues

Properly test projects with ts and pmap files
Make sure we install the files in the correct place
Make sure we don't fail if the project calls KI18N_INSTALL twice

apol added a comment.Mar 28 2017, 5:26 PM

Now it seems to work properly with scripts:

$ LANGUAGE=pl kgeography 
KTranscript: Loaded property map: /home/apol/devel/kde5/share/locale/pl/LC_SCRIPTS/kgeography/general.pmapc
KTranscript: Loaded module: /home/apol/devel/kde5/share/locale/pl/LC_SCRIPTS/kgeography/kgeography.js

apol updated this revision to Diff 12987.Mar 29 2017, 5:58 PM

Remove unneeded changes

apol updated this revision to Diff 13118.Apr 5 2017, 5:23 PM

Makes it possible to have ${CMAKE_SOURCE_DIR} and ${CMAKE_BINARY_DIR}

apol added a comment.Apr 6 2017, 5:25 PM

@ltoscano ping? (you asked me for this... πŸ™‚)

sitter edited edge metadata.Apr 7 2017, 10:46 AM

The code currently isn't working for po/ in source dir and you are losing a public function. See inline comments.

(Why there are no autotests for a complicated, involved, and important function like ki18n_install is entirely beyond my appreciation. How in the world is anyone meant to be able to confidently change or even review a change to this if there are a million cases to consider that are not even written down as a spec and cannot be readily tested as you first have to look for bloody real world examples where this stuff appears. This is making me so 😠)

cmake/KF5I18NMacros.cmake
62

It seems you are losing the public function(KI18N_INSTALL_TS_FILES seeing as that was public I would think a compat function calling the centralized ki18n_install function is needed + deprecation warning and note about deprecatedness in documentation please (:

136

I do not think that this is good enough.

Say I have a tree like this:

.
β”œβ”€β”€ CMakeLists.txt
β”œβ”€β”€ po
└── subsrc
    β”œβ”€β”€ CMakeLists.txt
    └── po

I would be calling ki18n_install twice with the same podir argument ki18n_install(po), so your string md5 ends up the same and cmake will fail due to name overlap in the targets. They are in different CURRENT_SOURCE_DIR contexts though.
What you need to do is something like

get_filename_component(abs podir ABSOLUTE)`
file(RELATIVE_PATH rel ${CMAKE_SOURCE_DIR} ${abs})

to resolve the path of podir relative to the source and use that to drive the md5 calculation as that path should then be unique (unless of course one calls ki18n_install in the same CMakeLists.txt more than once, which arguably is using it incorrectly ^^)

156

style: no space between if and (

cmake/build-pofiles.cmake
28

This does not work. As the pofiles-a1b2c3 target is forking another cmake the current source dir here is the binary dir not the original source dir, so while this works for the new fetch-translations target which puts the po in $bindir/po, it breaks for actually released software which will need the translations to be found in $srcdir/po.

I think the only reasonable way to solve this is to have KI18N_INSTALL create a target for src and one for bin and then pass in the absolute paths of each.

set(podirs "${podir}")
if(NOT IS_ABSOLUTE ${podir})
  set(bin_podir "${CMAKE_CURRENT_SOURCE_DIR}/${podir}")
  set(src_podir "${CMAKE_CURRENT_BINARY_DIR}/${podir}")
  set(podirs "${bin_podir};${src_podir}")
  list(REMOVE_DUPLICATES podirs) # src == bin
endif()

foreach(podir IN LISTS podirs)
    string(MD5 pathmd5 ${podir})
    add_custom_target(pofiles-${pathmd5} ALL
.......

If only one of them exists the other target will simply create nothing as the glob comes back empty. If both exist both create potentially the same targets, which may be undesirable, so perhaps this code should live in the build*.cmake helper files and instead of creating two targets you simply pass in the podirs list. The helpers then glob both in an exclusive fashion (i.e. foreach until one of the list entries comes back with a non-empty GLOB).

i.e. in function(KI18N_INSTALL:

set(podirs "${podir}")
if(NOT IS_ABSOLUTE ${podir})
  set(bin_podir "${CMAKE_CURRENT_SOURCE_DIR}/${podir}")
  set(src_podir "${CMAKE_CURRENT_BINARY_DIR}/${podir}")
  set(podirs "${bin_podir};${src_podir}")
  list(REMOVE_DUPLICATES podirs) # src == bin
endif()

and passing -DPO_DIRS=${podirs}

and in the helpers:

foreach(podir IN LISTS PO_DIRS)
    file(GLOB_RECURSE pofiles RELATIVE "${podir}" "${podir}/**.po")
    list(LENGTH pofiles pofiles_length)
    if(${pofiles_length} GREATER 0)
        set(PO_DIR ${podir})
        break()
    endif()
endforeach()

For testing this you can use a tag of plasma-framework for example (that also has a scripts dir FWIW).

apol updated this revision to Diff 13392.Apr 13 2017, 2:57 PM
apol marked 2 inline comments as done.

Address some issues pointed at by sitter

sitter added inline comments.Apr 13 2017, 3:04 PM
cmake/KF5I18NMacros.cmake
136

Please use message(AUTHOR_WARNING ... this warning has only relevance to a developer, not a user compiling an affected source tree. (neon's auto-QA for example ignores dev warnigns ;))

apol updated this revision to Diff 13404.Apr 13 2017, 6:50 PM

AUTHOR_WARNING

sitter accepted this revision.Apr 20 2017, 1:50 PM

Note that there's still an ordering problem with the new fetch-translations target now. The {ts,po}files-* targets can run before the fetch-translations target so on first run you may have no BINDIR/po directory yet, if KDE_L10N_AUTO_TRANSLATIONS was use anyway. make πŸ‘‰ pofiles-123abc runs and does nothing on account of the dir not existing πŸ‘‰ fetch-translations runs creating the dir πŸ‘‰ pofiles haven't been made.

I am not sure how to best solve this but I guess you'll need an if target fetch-translations; add_dependencies(fetch-translations pofiles) or somesuch.

Not really a problem with the change though, so this is good enough for landing for me. Still would prefer if it had tests though :P

This revision is now accepted and ready to land.Apr 20 2017, 1:50 PM
This revision was automatically updated to reflect the committed changes.