Make KI18N_INSTALL() not rely on only LOCALE_INSTALL_DIR
Needs ReviewPublic

Authored by kossebau on Apr 30 2020, 2:59 PM.

Details

Reviewers
ilic
heikobecker
aacid
ltoscano
Group Reviewers
Frameworks
Summary

a) LOCALE_INSTALL_DIR is a deprecated variable from KDEInstallDirs,

which will not be set with KDE_INSTALL_DIRS_NO_DEPRECATED

b) if using KI18n without KDEInstallDirs, but e.g. GnuInstallDirs or

custom install dir variables, having to explicitly set LOCALE_INSTALL_DIR
before calling KI18N_INSTALL() is not best DX. Best is to follow
typical install command macros and have this as explicit argument
by the name DESTINATION, instead of (KDE-specific) automagic

Diff Detail

Repository
R249 KI18n
Branch
adddestinationargtoki18ninstall
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26141
Build 26159: arc lint + arc unit
kossebau created this revision.Apr 30 2020, 2:59 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 30 2020, 2:59 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Apr 30 2020, 2:59 PM

Have to yet test this properly, uploaded for now as alternative approach to D29136

kossebau updated this revision to Diff 81597.Apr 30 2020, 3:21 PM
  • fix typo: ARGS_DESTINATION
  • small doc improvement

Tested now (with DESTINATION passed, without, without & LOCALE_INSTALL_DIR not set), so state: working proposal :)

Fixes the problem I had with marble, which prompted the creation of https://phabricator.kde.org/D29136. Passing the destination as an parameter seems indeed like a better way, so +1 from me.

pino added a subscriber: pino.Apr 30 2020, 3:39 PM

The problem is that ki18n_install() is rarely used manually, and generally it is appended by the release scripts to the top-level CMakeLists.txt file that goes into the tarballs.
This means that either the majority of the packages will not make use of this, or a mass-bump of the ki18n dependency (which I'd rather not do).

I personally prefer Heiko's approach (D29136), followed by making ECM required to use ki18n.

pino added a comment.Apr 30 2020, 3:46 PM

The problem is that ki18n_install() is rarely used manually

OK, at least from LXR search it seems a bit more than that: KF packages using ki18n, some extragear stuff, and few Plasma bits.
This means that KF packages can switch to that immediately (because of the strict dependency requirement), however most of the rest won't soon (like Marble, which will stay unfixed until an ECM requirement bump).

kossebau added a comment.EditedApr 30 2020, 3:59 PM

The patch should not require existing users to adapt (though personally I would favour explit DESTINATION args, perhaps a KF6 time thing).
That is also why there is no warning emitted in case the DESTINATION is not passed.

So nothing should need to be changed here after this patch, besides unbreaking things with KDE_INSTALL_DIRS_NO_DEPRECATED being set, as well as making ki18n easier to use for external non-ECM users (e.g. with GnuInstallDirs now it becomes ki18n_install(po DESTINATION ${CMAKE_INSTALL_LOCALEDIR}), easy to understand code.

pino added a comment.Apr 30 2020, 4:05 PM

The patch should not require existing users to adapt

Yes, that's also what I wrote earlier.

Also, your patch basically includes D29136 in the case of no DESTINATION parameter specified, hence my suggestion is:

  • edit D29136 to do the fallback using the same logic introduced here: this way marble is already fixed with no other changes, and ki18n_install will work also with KDE_INSTALL_DIRS_NO_DEPRECATED (e.g. for release-service packages)
  • have this to add the DESTINATION parameter, so packages can opt-in to use it if they can/want
kossebau added a comment.EditedApr 30 2020, 4:10 PM
In D29299#660453, @pino wrote:

The problem is that ki18n_install() is rarely used manually, and generally it is appended by the release scripts to the top-level CMakeLists.txt file that goes into the tarballs.

While being an orthogonal topic, IMHO this should be changed. There is just too much magic going on when it comes to translations, the avaerage KDE devloper does not understand it, at least by what I gathered :) Also would some not consider it a good practice to patch sources before adding to the tarball, making the sources different to what is tagged.

Personally I would favour rather explicit statements, with a comment explaining why there is no po/ directory in the sources and a link to a website where to learn more about things.

In D29299#660465, @pino wrote:

Also, your patch basically includes D29136 in the case of no DESTINATION parameter specified, hence my suggestion is:

  • edit D29136 to do the fallback using the same logic introduced here: this way marble is already fixed with no other changes, and ki18n_install will work also with KDE_INSTALL_DIRS_NO_DEPRECATED (e.g. for release-service packages)
  • have this to add the DESTINATION parameter, so packages can opt-in to use it if they can/want

Not exactly sure what you mean? Do you want two separate commits/reviews, one per issue?

pino added a comment.Apr 30 2020, 4:13 PM
In D29299#660465, @pino wrote:

Also, your patch basically includes D29136 in the case of no DESTINATION parameter specified, hence my suggestion is:

  • edit D29136 to do the fallback using the same logic introduced here: this way marble is already fixed with no other changes, and ki18n_install will work also with KDE_INSTALL_DIRS_NO_DEPRECATED (e.g. for release-service packages)
  • have this to add the DESTINATION parameter, so packages can opt-in to use it if they can/want

Not exactly sure what you mean? Do you want two separate commits/reviews, one per issue?

Yes, and we have them already: D29136 (to reopen) and this (which would need to rebased on the former).

In D29299#660468, @pino wrote:
In D29299#660465, @pino wrote:

Also, your patch basically includes D29136 in the case of no DESTINATION parameter specified, hence my suggestion is:

  • edit D29136 to do the fallback using the same logic introduced here: this way marble is already fixed with no other changes, and ki18n_install will work also with KDE_INSTALL_DIRS_NO_DEPRECATED (e.g. for release-service packages)
  • have this to add the DESTINATION parameter, so packages can opt-in to use it if they can/want

Not exactly sure what you mean? Do you want two separate commits/reviews, one per issue?

Yes, and we have them already: D29136 (to reopen) and this (which would need to rebased on the former).

D29136 in the current version though changes behaviour by favouring KDE_INSTALL_LOCALEDIR over LOCALE_INSTALL_DIR. Which at least in theory might somewhere in some distant galaxy break things ;) Strapped this patch here into D29303 as alternative for trhe given aspect.

pino added a comment.Apr 30 2020, 4:31 PM

D29136 in the current version though changes behaviour by favouring KDE_INSTALL_LOCALEDIR over LOCALE_INSTALL_DIR. Which at least in theory might somewhere in some distant galaxy break things ;)

Which is correct, since KDE_INSTALL_LOCALEDIR is set only by ECM (and hopefully nothing else), so it should be safe to use as preferred location.

Do you have any specific concern, instead of "might" and "distant galaxy"? Because if not, I'd rather just pull D29136 in, and call it a day.

Because people do strange things, and I prefer to rather not break their card house unless necessary. Only looking at the undeprecated variable second should be fine, no?

pino added a comment.Apr 30 2020, 4:45 PM

Because people do strange things, and I prefer to rather not break their card house unless necessary.

Again, in which cases? The only way it might change the path is: use ki18n AND build with cmake AND use the ki18n_install macro AND define a KDE_INSTALL_LOCALEDIR variable yourself. Too many factors to make it even something to even bother thinking about TBH.

Let's just use D29136, so it will use the non-deprecated variable no matter whether KDE_INSTALL_DIRS_NO_DEPRECATED is set.

In D29299#660519, @pino wrote:

Because people do strange things, and I prefer to rather not break their card house unless necessary.

Again, in which cases? The only way it might change the path is: use ki18n AND build with cmake AND use the ki18n_install macro AND define a KDE_INSTALL_LOCALEDIR variable yourself. Too many factors to make it even something to even bother thinking about TBH.

Mileages difer :)

One does not need to define KDE_INSTALL_LOCALEDIR oneself. One only needs to use find_package(KF5Plasma) or find_package(KF5Package), because some subrepo also does a plasma applet for Plasma integration or have someone cluelessly copied cmake code together because ENOTIMEFORBUILDSYSTEM and copying from the best=KDE and now-it-builds-so-do-not-touch-again.

I have seen quite some cmake code, otherwise I would not be so passive here in the change (and yes, there is a world outside kde repos) :) And when you recommneded ki18n outside of kde spheres over the sad Qt i18n stuff, you do not want to see things falling apart, You yourself might not bother, but I do, because I have been hit myself too often in my life by people carelessly changing default things in minor releases, because worksformecanotimaginesomethingstrangewhybother.

TBH I would not bother thinking about possibly breaking things for people, but just play safe. There is KF6 upcoming to change things. Why stop people caring for doing things more safely? :)

pino added a comment.Apr 30 2020, 6:28 PM
In D29299#660519, @pino wrote:

Because people do strange things, and I prefer to rather not break their card house unless necessary.

Again, in which cases? The only way it might change the path is: use ki18n AND build with cmake AND use the ki18n_install macro AND define a KDE_INSTALL_LOCALEDIR variable yourself. Too many factors to make it even something to even bother thinking about TBH.

Mileages difer :)

One does not need to define KDE_INSTALL_LOCALEDIR oneself. One only needs to use find_package(KF5Plasma) or find_package(KF5Package), because some subrepo also does a plasma applet for Plasma integration or have someone cluelessly copied cmake code together because ENOTIMEFORBUILDSYSTEM and copying from the best=KDE and now-it-builds-so-do-not-touch-again.

In this case it will be generated automatically, and it will be the same as LOCALE_INSTALL_DIR.

I have seen quite some cmake code, otherwise I would not be so passive here in the change (and yes, there is a world outside kde repos) :) And when you recommneded ki18n outside of kde spheres over the sad Qt i18n stuff, you do not want to see things falling apart, You yourself might not bother, but I do, because I have been hit myself too often in my life by people carelessly changing default things in minor releases, because worksformecanotimaginesomethingstrangewhybother.

Let me clarify: I do care about software outside kde.org, you are definitely not the only one that does that. What I wrote above about "not bothering" refers to that specific situation I listed, not to everything else.
I'm also one of the people that says "beware of compatibility"... when it makes sense. In this case, I do not see anything to keep compatibility with, and the only situation is already a broken case on its own.

Again: beside what I said earlier, do you see any actual potential other scenarios, nor situation where what I proposed would not be safe?

Why stop people caring for doing things more safely? :)

I am not, please do not put thoughts in my mind that were not mine, thanks :)

In D29299#660620, @pino wrote:

One does not need to define KDE_INSTALL_LOCALEDIR oneself. One only needs to use find_package(KF5Plasma) or find_package(KF5Package), because some subrepo also does a plasma applet for Plasma integration or have someone cluelessly copied cmake code together because ENOTIMEFORBUILDSYSTEM and copying from the best=KDE and now-it-builds-so-do-not-touch-again.

In this case it will be generated automatically, and it will be the same as LOCALE_INSTALL_DIR.

Only if nothing else sets this variable. And people do set such variables manually, e.g. after having looked up the docs of the macros they use. Because they have some crazy ideas/needs about installation/deploy layouts. Besides, LOCALE_INSTALL_DIR is also the name used commonly in custom variants of KDEInstallDirs/GnuInstallDirs (e.g. check github search for DefineInstallationPaths.cmake copies or just grep for LOCALE_INSTALL_DIR itself), possibly inspired by KDE's initial KDE4 times list. And this does not blend well, people miss where variables are from and just manually override things or reorder includes until things work.

Again, LOCALE_INSTALL_DIR is the variable documented in the dox and the code which people have seen and made sure it is set to a value they want. And it is a variable used in some projects, independent of KDE. KDEInstallDirs messes around with that as well, but projects which have that included knowingly or unknowingly have worked around that. Now if suddenly KDE_INSTALL_LOCALEDIR gets to rule over LOCALE_INSTALL_DIR, it will break things for them.
(people could also have unset LOCALE_INSTALL_DIR to enforce the default, and then KDE_INSTALL_LOCALEDIR as first fallback would screw that, ,but I see even lesser chance in that, not a pattern I have come across)

Again: beside what I said earlier, do you see any actual potential other scenarios, nor situation where what I proposed would not be safe?

I cannot point you to any existing public available code which will fall out over this. But given my experiences on other people's cmake code, I am pretty sure there some using ki18n outside kde which run chance. People do have random "include(KDEInstallDirs)" around, due to helpless cmake code copy&paste. People try to write Plasma applets, thus injecting KDEInstallDirs via find(Plasma) into their otherwise not-ECM-based cmake system.

Why stop people caring for doing things more safely? :)

I am not, please do not put thoughts in my mind that were not mine, thanks :)

Just telling what thoughts are created at receiver's end about the sending one :)

For some context: I care a lot about ECM. After all I am one of the top-2 ECM contributors for the last 2 years. When it comes to KDEInstallDirs, you can find lots of commits from me over all repos which ported code to use non-deprecated KDEInstallDirs variables. And it was me who introduced the use of KDE_INSTALL_DIRS_NO_DEPRECATED with Marble's buildsystem to marry the desire of Marble maintainers to have a Qt-only dependency build variant together with my desire of a full-featured ECM/KF/Plasma one, without cmake variables stepping on each other toes. At least at that time to success.

It seems it boils down to: you think no-one writes such broken/fragile cmake code that can be trapped by this. I do believe there are some. Both variants of the patches are correct (right?) and fix things for KDE_INSTALL_DIRS_NO_DEPRECATED. The variant I prefer though increases the chance to not break things for any potential fragile code out there. Can't you just give me the credit my gut feelings are based on experience? Is there any actual reason the variant you prefer has to be used instead?