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

[Aleix Pol Gonzalez]

This revision was automatically updated to reflect the committed changes.
Closed by commit R249:a54ae95b520e: Move .po and .ts files look-up to 
build-time (authored by apol).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D5167?vs=13404&id=13654#toc

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=13404&id=13654

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Harald Sitter]

sitter accepted this revision.
sitter added a comment.
This revision is now accepted and ready to land.


  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

REPOSITORY
  R249 KI18n

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 13404.
apol added a comment.


  AUTHOR_WARNING

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=13392&id=13404

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, ltoscano, ilic, sitter
Cc: aacid

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

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> KF5I18NMacros.cmake:136
> +function(ki18n_install_ts_files _lang)
> +message(WARNING "KI18N_INSTALL_TS_FILES is deprecated!")
> +ki18n_install(${_lang})

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 ;))

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, ltoscano, ilic, sitter
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 13392.
apol marked 2 inline comments as done.
apol added a comment.


  Address some issues pointed at by sitter

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=13118&id=13392

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, ltoscano, ilic, sitter
Cc: aacid

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

[Harald Sitter]

sitter added a comment.


  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 😠)

INLINE COMMENTS

> KF5I18NMacros.cmake:62
> -#usage: KI18N_INSTALL_TS_FILES("ja" ${scripts_dir})
> -function(KI18N_INSTALL_TS_FILES lang scripts_dir)
> -   file(GLOB_RECURSE ts_files RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} 
> ${scripts_dir}/*)

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 (:

> KF5I18NMacros.cmake:101
> +
> +string(MD5 pathmd5 ${podir})
> +

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 ^^)

> KF5I18NMacros.cmake:121
> +
> +if (NOT TARGET pofiles)
> +add_custom_target(pofiles)

style: no space between if and (

> build-pofiles.cmake:27
> +
> +file(GLOB_RECURSE pofiles RELATIVE "${PO_DIR}" "${PO_DIR}/**.po")
> +

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).

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, ltoscano, ilic, sitter
Cc: aacid

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

[Aleix Pol Gonzalez]

apol added a comment.


  @ltoscano ping? (you asked me for this... 🙂)

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 13118.
apol added a comment.


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

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=12987&id=13118

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 12987.
apol added a comment.


  Remove unneeded changes

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=12931&id=12987

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol added a comment.


  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
  
  F3394527: Spectacle.h29248.png 

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 12931.
apol added a comment.


  Fix

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=12930&id=12931

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 12930.
apol added a comment.


  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

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=12867&id=12930

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 12867.
apol added a comment.


  Install in the correct directory

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=12862&id=12867

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol added a comment.


  This is KAlgebra in catalan with the translations generated with this method: 
F3317742: Spectacle.jU4256.png 
  
  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.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol updated this revision to Diff 12862.
apol added a comment.


  Uncomment test, remove unneeded glob

REPOSITORY
  R249 KI18n

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5167?vs=12772&id=12862

BRANCH
  fetchbuildtime

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Albert Astals Cid]

aacid added a comment.


  s/Tried/try

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Albert Astals Cid]

aacid added a comment.


  Tried it in kde-l10n-sr for example

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Albert Astals Cid]

aacid added a comment.


  Have you tried this?
  
  That commented out test doesn't seem like a good idea.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic
Cc: aacid

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

[Aleix Pol Gonzalez]

apol added a reviewer: ilic.

REPOSITORY
  R249 KI18n

REVISION DETAIL
  https://phabricator.kde.org/D5167

To: apol, #frameworks, sitter, ltoscano, ilic

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

[Aleix Pol Gonzalez]

apol created this revision.
Restricted Application added a project: Frameworks.

REVISION 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.

REPOSITORY
  R249 KI18n

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D5167

AFFECTED FILES
  CMakeLists.txt
  autotests/CMakeLists.txt
  cmake/KF5I18NMacros.cmake
  cmake/build-pofiles.cmake
  cmake/build-tsfiles.cmake

To: apol, #frameworks, sitter, ltoscano