Use non-deprecated KDEInstallDir
Needs ReviewPublic

Authored by heikobecker on Apr 23 2020, 6:33 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

I noticed that when installeding marble, which sets
KDE_INSTALL_DIRS_NO_DEPRECATED, which then invalidates a possibly
custom location set via KDEInstallIDirs, causing translations to
land in a surprising locations.

Test Plan

Marble's translations land in the desired location

Diff Detail

Repository
R249 KI18n
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25787
Build 25805: arc lint + arc unit
heikobecker created this revision.Apr 23 2020, 6:33 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 23 2020, 6:33 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
heikobecker requested review of this revision.Apr 23 2020, 6:33 PM

Added missing parentheses

Just came by, have to get back later, but first comment:
using kdeinstalldirs variables needs to ensure that KDEInstallDirs has been included before, also introduces ahard dependency on ECM for any users of KI18n. While 99% of apps using KI18n might do this, by design idea of KDE Frameworks KI18n as tier1 should not pull in another dependency, even ECM (so someone using plain cmake & GnuInstallDirs should be still able to use tier1 stuff). This needs some more pondering then...

using kdeinstalldirs variables needs to ensure that KDEInstallDirs has been included before, also introduces ahard dependency on ECM for any users of KI18n. While 99% of apps using KI18n might do this, by design idea of KDE Frameworks KI18n as tier1 should not pull in another dependency, even ECM (so someone using plain cmake & GnuInstallDirs should be still able to use tier1 stuff). This needs some more pondering then...

Isn't that a mere theoretical concern? At least when building ki18n yourself, you need ECM anyway, which is easy to install and almost infinitesimal small in size compared to Qt. And btw, where is the "should not pull in another dependency, even ECM" documentend? I only know of "Tier 1 Frameworks can depend only on Qt official frameworks or other system libraries" [1], which admittedly already creates a discrepancy with the ECM (build) requirement of ki18n itself.

I'd also note that the macro already used KDEInstallDirs before, apparently without anybody complaining about it, even though I don't want to cargo cult this. Furthermore I'm not sure how to solve the bug I encountered differently, other than making marble stop using KDE_INSTALL_DIRS_NO_DEPRECATED (or using GnuInstallDirs, which possibly might break existing things).

[1] https://community.kde.org/Frameworks/Policies

I understand the desire for pragmatic solution, but I am too old now and have seen too many places where being pragmatic at one point resulted in tightly coupled systems which later prevented progress.
When it comes to build dependencies, forcing users of a product to use the same build tools as used for the product is not well received by me in principle, as it lowers flexibility and choice as user.

So let me see for now how we can get ahead here without needing to extent KI18nConfig with find_dependecy(ECM).

Where would you see "that the macro already used KDEInstallDirs before"? When it comes to "LOCALE_INSTALL_DIR", that is set to a default is not set when calling the macro. Ideally would be documented though. (my first approach would be to also allow a soft dependency here on KDEInstallDirs, checking whether KDE_INSTALL_LOCALEDIR is defined and picking its value), similar with CMAKE_INSTALL_LOCALEDIR to support GnuInstallDirs automatically).

Where would you see "that the macro already used KDEInstallDirs before"? When it comes to "LOCALE_INSTALL_DIR", that is set to a default is not set when calling the macro. Ideally would be documented though. (my first approach would be to also allow a soft dependency here on KDEInstallDirs, checking whether KDE_INSTALL_LOCALEDIR is defined and picking its value), similar with CMAKE_INSTALL_LOCALEDIR to support GnuInstallDirs automatically).

As you indicate, if one calls ki18_install() in a project which includes KDEInstallDirs before that call (and KDE_INSTALL_DIRS_NO_DEPRECATED isn't set) the value of LOCALE_INSTALL_DIR is used instead of the default "share/locale". I extended the same to check K_I_LOCALEDIR (line 99+). Isn't that the soft dependency you suggested (minus the GnuInstallDirs part, obviously?)

Where would you see "that the macro already used KDEInstallDirs before"? When it comes to "LOCALE_INSTALL_DIR", that is set to a default is not set when calling the macro. Ideally would be documented though. (my first approach would be to also allow a soft dependency here on KDEInstallDirs, checking whether KDE_INSTALL_LOCALEDIR is defined and picking its value), similar with CMAKE_INSTALL_LOCALEDIR to support GnuInstallDirs automatically).

As you indicate, if one calls ki18_install() in a project which includes KDEInstallDirs before that call (and KDE_INSTALL_DIRS_NO_DEPRECATED isn't set) the value of LOCALE_INSTALL_DIR is used instead of the default "share/locale". I extended the same to check K_I_LOCALEDIR (line 99+). Isn't that the soft dependency you suggested (minus the GnuInstallDirs part, obviously?)

I was more thinking something along D29299 (turned my draft code into a proper patch for discussing, while at it :) ).

From what I meanwhile found, people using GnuInstallDirs would only have CMAKE_INSTALL_LOCALEDIR set, so they can be ignored here then, at least I would rather advocate for explicite argument passing as in the alternative patch.

heikobecker reclaimed this revision.May 1 2020, 8:42 AM
dfaure added a subscriber: dfaure.Sep 12 2020, 12:45 PM

(to remove some confusion: the previous comment had the wrong link and should have said "Abandoned in favour of https://phabricator.kde.org/D29299" -- but now it's reopened anyway, as an alternative to D29299)