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?

How do we move this or https://phabricator.kde.org/D29136 forward? @ilic As a maintainer, do you have an opinion?

dfaure added a subscriber: dfaure.Sep 12 2020, 12:44 PM

@pino Other than the fact that you think D29136 is "good enough", do you have any concrete objection to this version?

My own objection would simply be that "backward-compatible fallback" is a bit too strongly worded; it reads like "this smells bad and might go away one day".
How about we rather call this ECM integration, to make it clear that as long as you're using ECM, we do not expect every caller of ki18n_install to pass the destination manually?

cmake/KF5I18nMacros.cmake.in
83

(to be updated to 5.75 now)

97

typo: s/DESITNATION/DESTINATION/

pino added a comment.Sep 12 2020, 12:57 PM

@pino Other than the fact that you think D29136 is "good enough", do you have any concrete objection to this version?

I think I already wrote enough comments on this...

I asked for actual valid use cases when using the new variables first would break, and I still got none. There is a limit to how much you can keep broken code working... assuming such broken code exists. I don't think there is any of this such situation, as ki18n_install() is basically used by KF sources that use ECM already, with marble being the only exception (and even that, marble won't break).

Oh, and just to make it clear: none of my comments implies that I don't care about ECM, nor about any users of it, nor that I "like" to purposefully break cmake scripts.

In D29299#676445, @pino wrote:

I asked for actual valid use cases when using the new variables first would break, and I still got none. There is a limit to how much you can keep broken code working... assuming such broken code exists. I don't think there is any of this such situation, as ki18n_install() is basically used by KF sources that use ECM already, with marble being the only exception (and even that, marble won't break).

As you know, there are KF5-based applications outside the realm of what we can see in LXR.
One of the primary goals of KF5 is to be useable by other applications not written by the KDE community (I actually know quite a few).
As such, it's not hard to imagine a cmake-based application that uses Qt and GNUInstallDirs [with qmake going away this will happen more and more], and one day it wants to use one of the frameworks. At that point, it shouldn't be forced to switch to ECMInstallDirs. Therefore I definitely see value in keeping the two things separate, as long as we keep making things easy for what is the most common case for us: using both.

This is why I'm requesting that the integration with ECM is called integration and not "backwards compat fallback".

You say you don't want to support broken code. I agree. Would you agree that the situation I'm describing here is NOT broken code?

Oh, and just to make it clear: none of my comments implies that I don't care about ECM, nor about any users of it, nor that I "like" to purposefully break cmake scripts.

I didn't say any of this, and you're replying to me here, not to Friedrich. I'm trying to bring this whole thing to a solution, so let's move aside all such accusations and concentrate on what might be helpful to resolve the technical issue.

pino added a comment.Sep 12 2020, 1:15 PM
In D29299#676445, @pino wrote:

I asked for actual valid use cases when using the new variables first would break, and I still got none. There is a limit to how much you can keep broken code working... assuming such broken code exists. I don't think there is any of this such situation, as ki18n_install() is basically used by KF sources that use ECM already, with marble being the only exception (and even that, marble won't break).

As you know, there are KF5-based applications outside the realm of what we can see in LXR.
One of the primary goals of KF5 is to be useable by other applications not written by the KDE community (I actually know quite a few).
As such, it's not hard to imagine a cmake-based application that uses Qt and GNUInstallDirs [with qmake going away this will happen more and more], and one day it wants to use one of the frameworks. At that point, it shouldn't be forced to switch to ECMInstallDirs. Therefore I definitely see value in keeping the two things separate, as long as we keep making things easy for what is the most common case for us: using both.

Sigh. I know this, I never, ever, ever, and let me say it again, never, forgot about this.

This is why I'm requesting that the integration with ECM is called integration and not "backwards compat fallback".

You say you don't want to support broken code. I agree. Would you agree that the situation I'm describing here is NOT broken code?

Sure. But it is not what I referred to when I spoke about "broken code".

Oh, and just to make it clear: none of my comments implies that I don't care about ECM, nor about any users of it, nor that I "like" to purposefully break cmake scripts.

I didn't say any of this, and you're replying to me here, not to Friedrich. I'm trying to bring this whole thing to a solution, so let's move aside all such accusations and concentrate on what might be helpful to resolve the technical issue.

Yes, sorry, you are not Friedrich. OTOH, it would be nice to not be painted as "the one that wants to break things without second thoughts". Can we please agree on this? Because if not, there is no more point for me to keep contribute to a discussion.

Again, I already proposed a technical solution. Let me quote it again:

In D29299#660465, @pino wrote:

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
In D29299#676447, @pino wrote:
In D29299#676445, @pino wrote:

One of the primary goals of KF5 is to be useable by other applications not written by the KDE community (I actually know quite a few).
As such, it's not hard to imagine a cmake-based application that uses Qt and GNUInstallDirs [with qmake going away this will happen more and more], and one day it wants to use one of the frameworks. At that point, it shouldn't be forced to switch to ECMInstallDirs. Therefore I definitely see value in keeping the two things separate, as long as we keep making things easy for what is the most common case for us: using both.

Sigh. I know this, I never, ever, ever, and let me say it again, never, forgot about this.

OK sorry for stating the obvious then.
When you wrote "ki18n_install() is basically used by KF sources that use ECM already" it seemed to me that this was looking at KDE community code only; it's hard to make such a broad statement if including also external code. If you agree that being able to use ki18n without ECM is better, then indeed we all agree.

Sure. But it is not what I referred to when I spoke about "broken code".

I have to apologize again, then, because I don't understand what is the "broken code" we're talking about then.

Yes, sorry, you are not Friedrich. OTOH, it would be nice to not be painted as "the one that wants to break things without second thoughts". Can we please agree on this?

We do agree on this. I don't think I was painting you that way at all. I'm merely trying to understand both sides of the argument and find a solution that works for everyone.

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)

Would this be what is done in D29303? (I just learned about this third option...)

pino added a comment.EditedSep 12 2020, 2:38 PM

When you wrote "ki18n_install() is basically used by KF sources that use ECM already" it seemed to me that this was looking at KDE community code only

FWIW, I also checked https://sources.debian.org/ (which is the collection of sources in the Debian releases). I'm not aware of other services like this.

If you agree that being able to use ki18n without ECM is better, then indeed we all agree.

If that wouldn't had been the case, I wouldn't have said to prefer D29136.

Sure. But it is not what I referred to when I spoke about "broken code".

I have to apologize again, then, because I don't understand what is the "broken code" we're talking about then.

"broken code" is when a cmake script (mis)uses undocumented/internal bits/variables not documented as such. Or for example when you redefine macros, internal variables, or stuff like that.

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)

Would this be what is done in D29303? (I just learned about this third option...)

Kinda, with the difference that it still prefers the deprecated variables. This is why I prefer D29136.