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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170407/33ba5d8a/attachment-0001.html>