RFC: an improved ki18n_install

[Albert Astals Cid]

El dimarts, 7 de març de 2023, a les 9:21:07 (CET), Milian Wolff va escriure:
> On Dienstag, 7. März 2023 02:51:06 CET Aleix Pol wrote:
> > On Mon, Mar 6, 2023 at 8:31 PM Milian Wolff <mail at milianw.de> wrote:
> > > Hey all,
> > > 
> > > for context please see [1] and [2].
> > > 
> > > [1]: https://bugs.kde.org/show_bug.cgi?id=460245
> > > [2]:
> > > https://discourse.cmake.org/t/feature-request-add-custom-command-all/760
> > > 9
> > > 
> > > The gist is that me and Igor are annoyed by `ki18n_install` abusing
> > > `add_custom_target`, which makes the build always dirty. I.e. every time
> > > we
> > > run `ninja` we will, at the very least, run the two mo & ts generation
> > > targets. For kdevelop, these do non-trivial amounts of work that takes
> > > time
> > > even on a beefy laptop:
> > > 
> > > ```
> > > $ ninja && time ninja
> > > [2/2] Generating mo...
> > > [2/2] Generating mo...
> > > 
> > > real    0m1,780s
> > > user    0m5,742s
> > > sys     0m2,327s
> > > ```
> > > 
> > > I would like to fix this, but first want to get feedback from the KDE
> > > developers at large.
> > > 
> > > First of all: Are all projects using ki18n_install in the way we do? Why
> > > is
> > > no-one else complaining about this so far? Are your po files so small
> > > that
> > > this doesn't take a significant amount of time? Or are there potentially
> > > just so few of them in your project? KDevelop is heavily plugin based
> > > which means we have tons of po files. 2464 *.po files to be exact...
> > > 
> > > Secondly: does anyone know how to best approach a solution to this
> > > issue?
> > > The problem is that I don't see an easy way to solve it: While Kyle
> > > Edwards was nice enough to show me a way to tell CMake to not make the
> > > custom target always-dirty, `k18n_install` suffers from another design
> > > deficiency: It uses globbing internally and has an undefined amount of
> > > inputs and outputs, which basically makes it impossible for us to
> > > leverage `add_custom_command(OUTPUT` here, or?
> > > 
> > > As such, it seems like one would need a different approach to this
> > > problem
> > > anyways. Globbing is a known-evil in cmake land, and I would personally
> > > prefer to have the inputs explicitly listed, just like we do for source
> > > files etc. Because we have many languages though, listing all possible
> > > *.po or *.ts files is cumbersome. Instead, what about we maintain the
> > > list of known languages in the ki18n framework? Then we could have
> > > something like
> > > 
> > > ```
> > > ki18n_target(
> > > 
> > >     # the target for which we are handling messages
> > >     TARGET KF5I18n
> > >     # the translation domain, added as compile_options and to find files
> > >     TRANSLATION_DOMAIN ki18n5
> > >     # the root po folder location, to look for the .po/.js files
> > >     PO_FOLDER ${KI18n_SOURCE_DIR}/po
> > > 
> > > )
> > > ```
> > > 
> > > Internally, this would do what is currently done manually:
> > > 
> > > ```
> > > target_compile_options(KF5I18n PRIVATE -DTRANSLATION_DOMAIN=\"ki18n5\")
> > > ```
> > > 
> > > Then it would iterate over the list of known languages and try to find
> > > the
> > > .po or .js file. For every match, we would add_custom_target to generate
> > > the corresponding .mo/.ts file and add the reverse dependency.
> > > 
> > > What do others say to this approach?
> > > 
> > > Thanks
> > > 
> > > --
> > > Milian Wolff
> > > http://milianw.de
> > 
> > +1 to getting the problem solved. I've also been bothered by this and
> > thought about looking into this so thanks for stepping up! (also I'm
> > pretty sure this code is originally mine from a time when we didn't
> > generate translations on every single build ^^').
> > 
> > Having ninja/make do this dependency generation can end up being more
> > work than worth it because then we need to have a static list and then
> > we need to re-run it when the po files change and it's all a mess.
> 
> The only mess that I can see is having to maintain the static list.
> Otherwise, we would just piggy back on the underlying buildsystem to drive
> the generation for us. Meaning, we would get separate targets for every
> .po/.js, leading to proper parallelization too. And only new/changed files
> would get re-generated as needed.
> 
> Anyhow, reaping those benefits is going to be hard due to the static list.
> As Albert said in his email, my proposal has no advantages over just using
> glob. As such, we have basically three options:
> 
> a) status quo:
>  + trivial to setup
>  + will always correctly track changes to the .po/.js files
>  - serial instead of parallel generation of .mo/.ts files
>  - always dirty ALL target
> 
> b) glob with separate targets:
>  + trivial to setup
>  + parallelized generation of .mo/.ts files
>  + clean ALL target
>  - detecting new .po/.js files requires manual re-run of cmake (changes to
> existing .po/.js files will be detected automatically)
> 
> c) static list:
>  + parallelized generation of .mo/.ts files
>  + clean ALL target
>  - hard to setup, to maintain the static list
> 
> I think c) would only be an option paired with some scripting. I.e. ideally
> we would let scripty maintain the static list for us when it syncs
> translations?

Yes, that is probably possible, someone has to make scripty do that though 
(and adapt the ki18n_install to use that file if available, probably defaulting 
to the current behaviour as default if not).

Personally I do not have the time to do that on neither of the sides.

Cheers,
  Albert

> 
> If we do that, then I think it would be the best solution. What do others
> think?
> 
> > How
> > about changing build-pofiles/build-tsfiles to only execute the process
> > if the origin file is newer than the target?
> > 
> > I've put together a MR to explain what I meant (and I guess out of
> > guilt for having produced this :D)
> > https://invent.kde.org/frameworks/ki18n/-/merge_requests/81
> 
> Thanks, that sounds like a great improvement that should get in one way or
> another. Can you also backport that to KF5 (or will there be no more
> releases of that?)
> 
> Thanks