pgaskin / NickelMenu

The easiest way to launch scripts, change settings, and run actions on Kobo e-readers.
https://pgaskin.net/NickelMenu
MIT License
591 stars 32 forks source link

Honor CPPFLAGS #5

Closed NiLuJe closed 4 years ago

NiLuJe commented 4 years ago

(Not that we need it w/ the current TC ;)).

Honor CXXFLAGS when linking. This is necessary when using LTO with such an ancient GCC version.

NiLuJe commented 4 years ago

Not that I'd recommend actually using LTO with GCC 4.9, but this happens to be my default setup, and I'd forgotten to disable it, which made me notice this ;).

pgaskin commented 4 years ago

Is there a reason why you don't include it in LDFLAGS directly? IIRC, other build systems like CMake won't put CXXFLAGS in the linker by default.

NiLuJe commented 4 years ago

Because it has nothing whatsoever to do with linker flags? They're technically needed because we go through the GCC/G++ driver. And, admittedly, that's only an actual issue with legacy LTO handling. Current LTO implementations remember the flags in the LTO objects directly.

I wouldn't take CMake as an example for... anything, really. It's a flaming mess, to put it mildly ^^. (Among its many and varied issues, it completely ignores CPPFLAGS, for instance, which is just plain madness).

There's indeed a number of broken buildsystems that require you to put CPPFLAGS in C*FLAGS and/or LDFLAGS in C*FLAGS, but that's just broken by design (and autotools will shout sternly at you if it detects you did so).

pgaskin commented 4 years ago

I've been on the fence or merging this one, since it's mainly to fix a GCC issue, but since the only compiler you're supposed to with this is GCC, I've decided to merge it. One reason why I wanted to keep LDFLAGS only in the linked is to enforce that linker flags should be linker flags, and cflags should be only that, without mixing the two together. Also, I believe the original way is cleaner. Nevertheless, mixing the flags also has its own merits regarding consistency for the only TC which is meant to be used with this.

NiLuJe commented 4 years ago

I'll admit to having strong opinions on stuff like that ;).

In this case, it's effectively a no-op unless someone enables LTO, which one really shouldn't with GCC 4.9 ;).

(I don't actually know if Clang ever suffered from the same drawbacks when LTO was in its infancy there, but, it's certainly no longer the case, ThinLTO or not. GCC fixed it in GCC 5, I think).

pgaskin commented 4 years ago

Or unless someone puts a linker flag into the compiler flags.

NiLuJe commented 4 years ago

Assuming they're escaped properly, which they always should, since they're already escaped (by which I mean -Wl,) in LDFLAGS, that's still a noop?

(And also not specific to this PR, I didn't add LDFLAGS to compile-only stages, because that makes no sense ^^. But would still be a no-op if escaped properly).