Bug 369554

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.

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.

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

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?

>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?

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.

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

>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?

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.

>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" ... :)

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?

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.

>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).

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...