Bug 369554 - sleeper design bug in the installation layout due to assumptions about filesystem case sensitivity
Summary: sleeper design bug in the installation layout due to assumptions about filesy...
Status: REPORTED
Alias: None
Product: buildsystem
Classification: Developer tools
Component: general (show other bugs)
Version: unspecified
Platform: Other Other
: NOR major
Target Milestone: ---
Assignee: Alexander Neundorf
URL: https://git.reviewboard.kde.org/r/127...
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-30 08:28 UTC by RJVB
Modified: 2019-06-08 11:12 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Porting kpeople as an example (2.50 KB, patch)
2019-03-04 12:33 UTC, David Faure
Details
alternative patch, for Attica (2.61 KB, text/plain)
2019-03-04 13:26 UTC, RJVB
Details

Note You need to log in before you can comment on or make changes to this bug.
Description RJVB 2016-09-30 08:28:36 UTC
There is a design sleeper bug in the installation layout that has serious effects when triggered. I see it throughout the frameworks and have to presume it is made in other places too, which is why I am reporting it in this section (the only relevant generic one that doesn't only have old-and-cold tickets).

The underlying issue here is that the design of at least the framework header file tree assumes it is installed onto a case-sensitive filesystem. While this is the norm on traditional Unix and Linux, conceivable for good reasons, it simply isn't the norm in the rest of the world - and that rest is in fact a (much) bigger target market.

The perfect illustration of this design bug ls in the KF5 frameworks header file install locations. KF5 applies the usual convention that C headers are lower-case and have a .h extension and C++ headers are capitalised and have no extension: foo.h and Foo . A number of frameworks go beyond that, and put the C headers in a subdirectory "foo" and the C++ headers in a directory "Foo". For example:

KIOCore/KIO/Global
KIOCore/kio/global.h

Installing this on a case-insensitive filesystem 2 things may happen:
1) when KIO is created after kio (or vice-versa) an error is raised that the directory already exists
2) the underlying routines consider KIO and kio to be the same directory, and one ends up with

KIOCore/KIO/Global
KIOCore/KIO/global.h

or

KIOCore/kio/Global
KIOCore/kio/global.h

(presuming that the FS is case-preserving).

This is not a problem as long as the installed files are used only on the local host (and the software itself doesn't reject an opened file if its path isn't identical to the requested name).

The sleeper bug is triggered when installers are created on a case-insensitive host and deployed on a case-sensitive host. That target host will receive the same merged directory, but there the underlying system routines will take case into account, and in this example, including "KIO/Global" or "kio/kio.h" will fail.

We have had to deal with this bug on (Mac) OS (X) for a long time; in KDE4 days it was limited to a select few of badly designed projects (like nepomuk-core).

At the moment I have only noticed this design issue in the header file install location, which is of course an area that may not affect end-users so much. But given that the issue is most likely the result of insufficient consideration of all possible deployment targets it is bound to be present in other areas too.

Reproducible: Always

Steps to Reproduce:
1. Build any project on a host with case-insensitive filesystem
2. Create an installer
3. Deploy on a host with a case-sensitive filesystem


Actual Results:  
If the project contains 'foo' and 'Foo' directories in the same parent directory, these will be merged in the final install, either as 'foo' or as 'Foo' depending on the OS and the order of unpacking. As a result, opening either "foo/foo.h" or "Foo/Foo" will fail.

This also happens on a case-insensitive filesystem, but there it is only a problem if higher-level code checks the name of the file actually opened against the expected name (i.e. if it rejects "Foo/foo.h" or "foo/Foo").

Expected Results:  
No issues that result from assumptions about filesystem case sensitivity.

This design issue has implications for Mac and PC users. I have less view of PC user practices, but it is quite common for more advanced Mac users to set their system to case-sensitive, or at least use case-sensitive volumes (externals, partitions, ...). 

One easy way to get around this issue would be to test for and enforce case-sensitivity in the build system, so that installers can only be created on such systems. However, with the typical Mac way of installing by dragging an application icon to the target destination that won't be enough. Users will expect that they can drag their applications along with them, possibly transporting them on a thumb drive or other media with a case-insensitive FS like FAT32.

Note that Qt5 also uses "foo.h" C headers and "Foo" C++ headers (which then include the C header). However, it installs both versions into the same directory from the onset so the issue reported here shouldn't occur.
Comment 1 RJVB 2016-09-30 09:05:11 UTC
I don't really know how invasive it would be to address this issue properly, or rather, how to fix this without being overly invasive. Looking at KPeople I see

/opt/local/include/KF5/KPeople/KPeople/Widgets/Actions
/opt/local/include/KF5/KPeople/kpeople/widgets/actions.h

which means that on a case-insensitive-but-preserving FS (HFS, NTFS) you can end up with 4 different situations:

/opt/local/include/KF5/KPeople/kpeople/widgets/{Actions,actions.h}
/opt/local/include/KF5/KPeople/KPeople/widgets/{Actions,actions.h}
/opt/local/include/KF5/KPeople/kpeople/Widgets/{Actions,actions.h}
/opt/local/include/KF5/KPeople/KPeople/Widgets/{Actions,actions.h}

and that's presuming that the case preservations works reliably. Transfer such a tree via a FAT32 thumbdrive and all bets are off. Evidently that's less likely to happen with a headerfile directory (and just shouldn't be done) but I'm concerned that the underlying implicit assumption about the FS has been made elsewhere too. And as I note in the ticket: Mac OS app bundles should support being transferred to and from case-insensitive volumes and still be usable on a case-sensitive volume (idem for KF5 framework bundles if they ever see the light).

The most straightforward fix would be to use only CamelCase OR lowercase header directories, i.e. one of
/opt/local/include/KF5/KPeople/kpeople/widgets/{Actions,actions.h}
/opt/local/include/KF5/KPeople/KPeople/Widgets/{Actions,actions.h}

This is also what Qt5 does, and one can presume they have seen a lot more testing of this approach on many more platforms.
NB: one could create the appropriate symlinks when installing to a case-sensitive FS, but evidently that makes it impossible to copy the result to a case-insensitive FS.

I realise that doing this is an invasive change that requires patching lots of dependent code, and even if those patches could be generated automatically the patched code would then no longer build against older versions of the frameworks. Still, it'd be an almost perfect way to force people to look for other places where the assumption about FS case-sensitivity could lead to problems.
Comment 2 David Faure 2019-03-04 12:33:55 UTC
Created attachment 118529 [details]
Porting kpeople as an example

One solution would be to move the forwarding headers (the camelcase ones) to a different directory.

So instead of 

/opt/local/include/KF5/KPeople/KPeople/Widgets/Actions
/opt/local/include/KF5/KPeople/kpeople/widgets/actions.h

you'd have

/opt/local/include/KF5/KPeople/fwd/KPeople/Widgets/Actions
/opt/local/include/KF5/KPeople/kpeople/widgets/actions.h

Since cmake exports those paths as a target property, applications (using cmake) wouldn't have to change anything.
Same for apps using the generated pkgconfig or .pri files.

See proof-of-concept patch for kpeople attached.
It would of course break any projects that make assumptions about the layout of the include dirs (e.g. in hand-written Makefiles or something) instead of using the cmake targets, but I can live with that, it's the reason to use a proper buildsystem.

TODO: improving ECMGeneratePriFile.cmake and ECMGeneratePkgConfigFile.cmake to support multiple include dirs as input.
Comment 3 RJVB 2019-03-04 13:26:57 UTC
Created attachment 118533 [details]
alternative patch, for Attica

Wow, it's been ages since I thought of this one, good thing I wrote a very detailed issue description!

I had an alternative but comparable patch for Attica up for review but I can no longer connect to the old ReviewBoard site (AAARGH!) so I'm copying it here.

I stopped using it when it started causing me issues on Linux, I never figured out the what and why.

My idea was simply to put the forwarding (C++) headers into a c++ subdirectory.

If at all possible I would advise not to maintain camelcase for directories
Comment 4 David Faure 2019-03-05 17:47:23 UTC
Well the lowercase headers contain C++ code too, so the name C++ doesn't really make sense for the forwarding headers, I think "fwd" makes more sense.
Other than that, it's pretty much the same idea indeed.
You renamed other things (KF5Attica -> KF5/Attica), for reasons that don't seem related to this issue. Maybe the "problems on Linux" come from there.

Let's test my patch on Linux for a while... (after removing the old install dir for camelcase headers of course).

For me it works, I just built plasma-desktop/applets/kicker/plugin successfully (to pick one directory that uses KPeople). Heh actually it wasn't using the fwd headers for some old reason, but even after applying http://www.davidfaure.fr/2019/kicker_plugin_fix.diff it still builds. I suggest you update your Attica patch to work exactly like mine and try again?
Comment 5 RJVB 2019-03-09 10:05:39 UTC
>Well the lowercase headers contain C++ code too, so the name C++ doesn't really

Yeah, but that isn't really the point, is it? The point is where they're used.
Maybe I should ask why these headers actually exist?

>You renamed other things (KF5Attica -> KF5/Attica), for reasons that don't seem
I think my reasoning there was that this could reduce the number of -I arguments on command lines (a single -I/path/to/KF5 might be enough) and/or envision the creation of a single Mac "meta framework" (like the Accelerate.framework).

>http://www.davidfaure.fr/2019/kicker_plugin_fix.diff it still builds. I suggest
>you update your Attica patch to work exactly like mine and try again?

Working on it :)

One thought: couldn't the ecm_generate_headers macro take care too of setting the install destination for the headers it generates?
Comment 6 RJVB 2019-03-09 13:31:55 UTC
So this works with Attica, but then again, I haven't found a single case where its forwarding headers are used nowadays...

Now just to backpedal a bit: what is the interest of adding an additional "layer" of directories in the end? If only KPeople and KAttica are concerned, can't we just get rid of that over-organisation and use the same installation layout for all frameworks? The majority of code that needs to be adapted is in a tiny number of other frameworks (so in need of a 1-time change). For the code in plasma-desktop one should be able to use only the forwarding headers which should work with the current install layout and the simplified layout.
Comment 7 David Faure 2019-03-13 10:18:49 UTC
This is definitely not just about two frameworks, it's about ALL of them. Look at it, they all install forwarding headers.

Reducing the number of -I arguments is not a goal, it's in fact counter-productive. We want to make sure each framework is explicitly used, not just by chance.

Users of Attica forwarding headers: indeed I didn't see any either.
Fixed. https://commits.kde.org/discover/712a7dfcab388b93dba7477eb55dc95d48bcafca
Comment 8 RJVB 2019-03-13 13:11:21 UTC
>This is definitely not just about two frameworks, it's about ALL of them. Look
>at it, they all install forwarding headers.

Did you miss the bit where I said the others all install the forwarding headers at the same level? :)

>counter-productive. We want to make sure each framework is explicitly used, not
>just by chance.

A moot concern given how you still keep the framework-specific directories if you use

include/KF5/Attica/AccountBalance
include/KF5/Attica/accountbalance.h
#include <Attica/AccountBalance>
#include <Attica/accountbalance.h>

instead of

include/KF5/Attica/fwd/Attica/AccountBalance
include/KF5/Attica/attica/accountbalance.h
#include <Attica/AccountBalance>
#include <attica/accountbalance.h>

In addition, the former implementation will allow installing the frameworks as proper (Mac) frameworks.

>Fixed.

I'm still not convinced that I find this an improvement (= fix); increase the amount of I/O required just to get (supposedly) nicer looking #include lines?
Comment 9 David Faure 2019-03-13 13:42:39 UTC
Ah you're still confused by non-namespaced frameworks vs namespaced frameworks.
KIO, KParts and others behave like Attica: the class is namespaced (KParts::Part) so the forwarding header is namespaced (KParts/Part). And it still has to be inside the framework-specific directory, so yes, KF5/KParts/fwd/KParts/Part.


include/KF5/Attica/AccountBalance
#include <Attica/AccountBalance>
Would mean the include works even if you didn't ask to use Attica via find_package, that's what we want to prevent here.

Can we accept this and move on? All of this is just additional discussion that is not related to the bug at hand.
Comment 10 RJVB 2019-03-13 20:55:48 UTC
>Ah you're still confused by non-namespaced frameworks vs namespaced frameworks.

Confused, no, I simply wasn't aware that one could want to try to apply namespacing to headers too - and fail to see the point.

It's a bit of a miracle I never ran into problems with the KIO headers!

>Would mean the include works even if you didn't ask to use Attica via
>find_package, that's what we want to prevent here.

Idem, don't see really the point nor why it would require an extra level of indirection for the forwarding headers (assuming you apply the same principle to the non-forwarding headers).

I do see the drawback: those huge commandlines I mentioned earlier. Try finding something specific in there when you need to (and I can still remember when my builds failed because commandlines simply got too long).

>Can we accept this and move on?

I guess this is where I get to say "whatever" ... :)
Comment 11 RJVB 2019-03-14 10:38:19 UTC
OK, so I was wrong in thinking that only a minority of frameworks do this let's-add-another-directory-layer thing (which still feels like a solution in search of a problem to me).

Here's the list I've compiled so far, based on the frameworks I install - which excludes anything (really) not supported on Mac and of course everything that's not part of the frameworks collection.

Given the length of that list we're looking at a lot of copy/paste and code duplication (and there's the unknown number of "mere" libraries that may be making the same error).
So I'd really urge to set the install destitination in the ECM macro used to generate the forwarding headers, so the change to individual frameworks CMakeList files can be limited to removing some newly-redundant code:

- attica
- baloo
- kactivities
- kactivities-stats
- kdeclarative{,calendarevents,kquickaddons,quickaddons}
- kdesu
- kdnssd 
- kfilemetadata 
- kio{core,gui,widgets} 
- kjsembed
- kpackage
- kparts
- kpeople{,backend}
- kross
- krunner
- ktexteditor
- kunitconversion
- kxmlrpcclient
- purpose
- solid
- sonnet
- threadweaver 

PS: am I right that these forwarding headers could be symlinks on Unix-based platforms?
Comment 12 David Faure 2019-03-24 22:13:06 UTC
Are you saying that non-namespaced-headers frameworks like KCoreAddons, do not trigger any issue with case sensitive filesystem?

I hope you didn't spend too much time making that list, it's easy to find with 

grep -lw '^\s*PREFIX' **/CMakeLists.txt

e.g. you missed kholidays.

Anyhow, does my kpeople patch work for you? Want to do this everywhere?

It's not ecm_generate_header's job to install them. That would just make the porting job much bigger btw.
Comment 13 RJVB 2019-03-25 10:55:21 UTC
>Are you saying that non-namespaced-headers frameworks like KCoreAddons, do not
>trigger any issue with case sensitive filesystem?

I suppose you mean case INsensitive filesystems :) Indeed, non-namespaced frameworks are fine.

The problem here is with creating 2 sibling directories that differ only in case. Those cannot coexist on a case-insensitive FS, and typically the system will just treat them as a single directory (by case-folding the name).
It's always a bit tricky to reason about this (and understand WTH goes on) but as outlined in the OP
- files for foo/ and Foo/ end up in a single directory
- depending on OS and the install procedure, the directory creation step may in fact lead to a rename (i.e. foo/ becomes Foo/ if mkdir("Foo") is called when foo/ already exists). The OS could also fail on the creation step but if that happened the issue would probably have been addressed long ago.

Please re-read the initial summary if you have doubts.

>I hope you didn't spend too much time making that list, it's easy to find with 

No, just a little trawler+grep filter on my destdirs in my frameworks build directory.

>grep -lw '^\s*PREFIX' **/CMakeLists.txt

What does the double asterisk do?

>e.g. you missed kholidays.

Did I forget to mention I only checked the frameworks that I have installed?

>Anyhow, does my kpeople patch work for you?

I must have confirmed that it works, as expected. It removes the potential conflict. I'm still not convinced that all those path levels are useful but that's a different issue.

>Want to do this everywhere?

You mean would I like to sit down and apply those changes everywhere? Not particularly (see below) :D

>It's not ecm_generate_header's job to install them.

Maybe not, but IMHO it could easily set an additional variable that holds the suggested/official install location. It's not like there are no cmake functions and macros that set a whole bunch of related variables.

>That would just make the
>porting job much bigger btw.

How so? It would if you rename the macro and its implementation file so they match the new behaviour but even that should be just a step that's easy to do with a global search/replace. Personally I'd rather do that and then remove a few lines from a large bunch of files rather than having to do a point edit which you can almost only do by hand (and requires many more eye/hand/finger co-ordinated actions).
Comment 14 David Faure 2019-06-08 11:12:23 UTC
The double-asterix is a zsh shortcut for a recursive search, very convenient.

I'm not convinced that your proposed change isn't just as "manual" as the one I have in mind. But feel free to prove me by showing me a patch...