Use "${BIN_INSTALL_DIR}/data" for DATAROOTDIR on Windows.
ClosedPublic

Authored by vonreth on Jun 14 2016, 2:50 PM.

Details

Summary

Instead of using "share" use "${BIN_INSTALL_DIR}/data" on Windows,
this is the location provided by QStandardPaths for GenericDataLocation
on Windows.

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
vonreth updated this revision to Diff 4464.Jun 14 2016, 2:50 PM
vonreth retitled this revision from to Use "${BIN_INSTALL_DIR}/data" for DATAROOTDIR on Windows..
vonreth updated this object.
vonreth edited the test plan for this revision. (Show Details)
vonreth added a reviewer: dfaure.
vonreth added a subscriber: kfunk.Jun 14 2016, 2:50 PM
dfaure edited edge metadata.Jun 14 2016, 3:05 PM

+1 on the approach (given that I suggested it ;-)

kde-modules/KDEInstallDirs.cmake
493

Should this use something like
_define_relative(DATAROOTDIR BINDIR "data" "...." SHARE_INSTALL_PREFIX) instead ?

This seems to be the way to define vars that are relative to other vars, and it's already what's done for LIBEXEC on Windows, for instance, so I assume it works :-)

vonreth updated this revision to Diff 4465.Jun 14 2016, 3:13 PM
vonreth edited edge metadata.

Use _define_relative

vonreth updated this revision to Diff 4466.Jun 14 2016, 3:14 PM

Fix syntax.

dfaure accepted this revision.Jun 14 2016, 3:17 PM
dfaure edited edge metadata.
This revision is now accepted and ready to land.Jun 14 2016, 3:17 PM
vonreth closed this revision.Jun 14 2016, 8:28 PM

applied

mutlaqja reopened this revision.Jun 28 2016, 8:35 PM
mutlaqja added a subscriber: mutlaqja.

This breaks KStars on Windows. I'm using the latest emerge/qt/etc.. on Windows 10. While this is indeed the correct behavior, it breaks the normal usage of:

QString filename = QStandardPaths::locate(QStandardPaths::DataLocation, fileToSearch);

On Linux, the location above are:

"~/.local/share/<APPNAME>", "/usr/local/share/<APPNAME>", "/usr/share/<APPNAME>"

e.g. /usr/share/kstars which is where the read-only data are stored. But now under Windows, the locations are:

"C:/Users/<USER>/AppData/Local/<APPNAME>", "C:/ProgramData/<APPNAME>", "<APPDIR>", "<APPDIR>/data"

In my emerge environment, it was searching for the file under $KDEROOT/bin and $KDEROOT/bin/data

KStars data was installed to $KDEROOT/bin/data/kstars. Now, the problem is that QStandardPaths::locate(QStandardPaths::DataLocation, fileToSearch); searches in $KDEROOT/bin/data and NOT $KDEROOT/bin/data/kstars and therefore fails to find any data files.

We use that function extensively throughout KStars, so what's the solution for this?

This revision is now accepted and ready to land.Jun 28 2016, 8:35 PM
kfunk added a comment.Jun 29 2016, 6:33 AM

(snip)

KStars data was installed to $KDEROOT/bin/data/kstars. Now, the problem is that QStandardPaths::locate(QStandardPaths::DataLocation, fileToSearch); searches in $KDEROOT/bin/data and NOT $KDEROOT/bin/data/kstars and therefore fails to find any data files.

What does fileToSearch look like? Yes, QStandardPaths::locate(QStandardPaths::DataLocation) will just get you $KDEROOT/bin/data. You need to pass it the component name as well, i.e. QStandardPaths::locate(QStandardPaths::DataLocation, "kstars/mydatafile"), then it will look in the correct folder.

Feel free to check out kdevelop.git, and grep a bit for QStandardPaths::DataLocation to learn how it's supposed to be used.

We use that function extensively throughout KStars, so what's the solution for this?

So how is this suppose to work under Linux then?

QStandardPaths::locate(QStandardPaths::DataLocation, "kstars/mydatafile") will look in /usr/share/kstars and then look for "kstars/mydatafile", it will fail!

First, DataLocation is the deprecated name for AppLocalDataLocation. This means, it points to an app-specific directory (at least on Linux), so indeed passing "kstars/" after it is wrong.

One solution is GenericDataLocation + "kstars/myfile", that works for sure.

The other solution is DataLocation + "myfile", and installing the file in bin/data, NOT bin/data/kstars.
<APPDIR> is assumed to be "app-specific" already on Windows, so there's no need for a "/kstars" subdir in there.

kfunk added a comment.Jun 29 2016, 7:49 AM
In D1873#37676, @dfaure wrote:

First, DataLocation is the deprecated name for AppLocalDataLocation. This means, it points to an app-specific directory (at least on Linux), so indeed passing "kstars/" after it is wrong.

One solution is GenericDataLocation + "kstars/myfile", that works for sure.

For the record (already discussed on IRC), I was confusing DataLocation with GenericDataLocation. In KDevelop we're using GenericDataLocation + "kdevmanpage/foo", as you hinted. This works for us.

The other solution is DataLocation + "myfile", and installing the file in bin/data, NOT bin/data/kstars.
<APPDIR> is assumed to be "app-specific" already on Windows, so there's no need for a "/kstars" subdir in there.

We use the the following to install the data files:

install(FILES .... DESTINATION ${KDE_INSTALL_DATADIR}/kstars)
install(FILES .... DESTINATION ${KDE_INSTALL_DATADIR}/kstars/textures)
install(FILES .... DESTINATION ${KDE_INSTALL_DATADIR}/kstars/scripts)

So it's already appending kstars to $KDE_INSTALL_DATADIR, so you're suggesting we omit "kstars" under Windows?

Probably it's easier to use GenericDataLocation + "/kstars/" + file

Unknown Object (User) added a subscriber: Unknown Object (User).Jun 29 2016, 9:06 AM
This comment was removed by bcooksley.
Unknown Object (User) added a subscriber: Unknown Object (User).Jun 29 2016, 9:47 AM
This comment was removed by bcooksley.

Hurray after upgrading to ecm-5.24.0 I unsuspectingly ran into this.

Something like this is imo a very breaking change as it will require changes in software using kde frameworks. I'm sorry I missed this change before it was released, I would have argued against this. I'm trying to use frameworks as a somewhat stable base in gpg4win. I find that a change like this in a minor release is pretty massive and should have been optional with the old behavior as default.

I thought we learned the lesson that not using FHS layout under Windows is just inviting problems. At least we learned that in Gpg4win and just went back to a complete FHS installation layout. Moving everything into bin is just broken imo. Regardless of whether QStandardPaths is broken in the same way. Especially as I'm not only packaging KDE Software but other things like GTK or GnuPG which expect their data under share.
Now I have some translations under share/locale some under bin/locale... fun!

I'm reverting this change in a patch to gpg4win now. But this will probably break if other Frameworks expect this layout now. I hope though that Frameworks are flexible enough to handle this with QStandardpaths.

Can we make this optional / over-ridable? (Or is it and I'm failing to see the mechanism for overriding this)

I had to update my application to be compatible with this approach so I hope this does NOT get reverted.

I had to update my application to be compatible with this approach so I hope this does NOT get reverted.

Yes what's done (released) is done. After reading more doc I think i can just override it in our general CMake build command and hope that no one hardcordes the expectancy of the default location in some place.

Sorry for the first grumpy comment, I think your intention is, that if we didn't fix QStandardPaths we have to adapt to it's weird / brokenness on windows. But that does not mean I'm happy with it because as you can see from my comments, I think the installation layout "forced" by QStandardPaths is really wrong. QStandardPaths should be configurable at least because there is no "one size fits all layout" on windows. :-)

Regards

QStandardPaths looks into a number of locations (not just one), and more could be added, but to make the additional locations "configurable" rather than hardcoded, there was the idea (and even a patch floating around on the kde-windows list) to make it read additional paths from qt.conf. I'm still open to that idea, it's just that it didn't turn out to be necessary for KDE apps on Windows (kate and other people having chosen the solution in this patch).

"Moving everything into bin" is of course only the way it looks like when your bindir is "bin", the more common layout on Windows is that this is all at the root of the install prefix.

vonreth closed this revision.Mar 16 2018, 7:36 AM

Merged long ago