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.
Details
- Reviewers
sitter ltoscano ilic - Group Reviewers
Frameworks - Commits
- R249:a54ae95b520e: Move .po and .ts files look-up to build-time
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.
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.
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
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
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. 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). |
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 ;)) |
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