Build ignores compile flags given to CMake

[0EVSG]

Since commit cd041e2779ce14e7a4d822629b4a900e1390df84 the MusE build completely ignores compile flags given to CMake. The project's compile flags just override any CMAKE_CXX_FLAGS* variable instead of combining them.

This is problematic for packagers, often there's a need for platform-specific compile flags. It also causes the symptom of #1066. What happens there is:

• The default include path is given to CMake in the compile flags.
• CMake assumes the default include path to be always present because of the compile flags.
• Therefore CMake ignores any subsequent include_directories() of the default include path.
• MusE build just ignores the compile flags, overrides them.
• Compile fails, since it's missing the default include path.

Any chance this particular behavior could be reverted? There's already commented out lines in the CMakeLists.txt that treat the compile flags correctly.

[terminator356]

I have reverted the four lines. I hope that helps and I hope it is correct. I wonder what effect the CACHE keyword has in those lines. Might it refuse to accept any further changes to passed cxx flags, forcing a complete rebuild?

I don't quite understand how this caused issue 1066. Shouldn't CMake have 'just found' libsamplerate and the other libraries and their absolute paths, and why would it depend on some default path?

Hope this helps. Let me know if any more trouble or further refinements are required. Kybos changed those lines, I am a little uneasy whether we might be breaking something here, but we will see....

Thanks. Tim.

[0EVSG]

It does help in my case, thank you!

Good point about the CACHE keyword though. According to docs with FORCE it rewrites the cached variables into CMakeCache.txt every time CMake is run on the project. For the build system itself that doesn't make sense here. Variable cache is meant to remember user-settable options, not a static list of compile flags from the CMakeLists.txt. I can only guess what Kybos wanted to do there, maybe grep the complete CMAKE_CXX_FLAGS from CMakeCache.txt, or wrestling with some external tool.

If you feel uneasy about the change, I suppose there's no urgent need for a solution until the next release. That's when packagers will want to pick it up. Should give you some time to discuss it with Kybos. As a base for discussion, my recommendations would be:

• Use the lines you uncommented now, which allows a user to set compile flags.
• Get rid of the CACHE and FORCE keywords, thus extend the compile flags set by the user on the fly.
• Remove duplicates, since CMAKE_CXX_FLAGS and e.g. CMAKE_CXX_FLAGS_RELEASE are concatenated.
• Check CMAKE_CXX_FLAGS_RELWITHDEBINFO, I think it's missing a debug flag.

About #1066, CMake does find libsamplerate and the other libraries. The headers are found in the system default include path, /usr/local/include on FreeBSD. That's also the path that include_directories() wants to add. Except at this point in time, CMAKE_CXX_FLAGS still contains the default include path given by the user. And CMake decides to not include it again.

Then later on, CMAKE_CXX_FLAGS is overwritten, and the default include path is neither in the compile flags nor in the include directories. Hope this helps, it took me quite a while to understand that problem...

Regards

Flo

[terminator356]

Yeah it's still not correct, and that FORCE is evil. Lemme try to digest the dozens of pages I just read. For now, here's what I'm thinking: We should be able to remove the CACHE and FORCE keywords. (Without CACHE it uses your settings within this scope but does not overwrite the cache, which obviously seems equivalent to using CACHE and FORCE but without the harm to the cache.) I also think the ${CMAKE_CXX_FLAGS_xxx} terms that I just re-enabled should come before the items that we really wanted to set ourselves. Otherwise the existing CMAKE_CXX_FLAGS_xxx will overwrite what we really wanted. Get rid of the -Ox overrides (yikes, no -O3 for release?) and the redundant flags. Yeah it seems that first line is mis-named, it says "default cxx flags" but that is a bit of a mistake, it should be called "common cxx flags" or something like that.

I seem to recall that -O3 caused some trouble many years ago. Possibly with one of the libs we use. Whatever the problem, hopefully we can fix it now.

[0EVSG]

That pretty much sums up my conclusions too, so fully agree.

You could place some sane default flags before the ${CMAKE_CXX_FLAGS_xxx}, and what is strictly necessary to get a functional build after that. Regarding -O3, that sometimes blows up code size and can be slower, you have to benchmark. LTO almost always helps, but is not easily portable between platforms / compilers.

Anyway, feel free to close this PR or keep it for reference. I suppose #1066 can be closed too, when this is solved.

[terminator356]

Arrright, I changed it. CACHE and FORCE are gone and I put all our flags after the existing ones. Messages now print the flags before and after, just for fun. -O -g and -DNDEBUG were added where warranted before existing flags as a sort of 'suggested default'. Especially Debug build type since there is usually no existing -O. Having -O0 there is really important for devs.

Hm something weird. I cannot edit or test all these changes here at the moment, so I am editing the files directly on github. Our automatic AppImage builder is now showing a message, that I added, that libatomic is now required. It wasn't before, and not for the first few iterations of these attempted changes. Now it is. Flags problems? I tried forcing -O2 for Release but same thing. Weird since that's what it was before.

Nightly AppImage may be weird until I sort this out...

[terminator356]

So the message I added reveals that the github building host does not support atomics without libatomic. It was always like that, but this just revealed it. And I think the reason is that the building host is a virtual machine, not a real one. Seems to me this is probably a good thing, since we wouldn't want our nightly AppImage to be tied to one type of architecture at build time which happens to support atomics without the library.

[0EVSG]

FYI, I built a local MusE 4.1 with your changes patched in, means it works on FreeBSD / Clang. Compile flags look good to me.

[github-actions[bot]]

This issue is stale because it has been inactive for two years. Remove Stale label or write a comment, otherwise it will be closed in 30 days.

[github-actions[bot]]

Issue has been closed automatically after two years of inactivity. Feel free to reopen if the issue is still relevant for current MusE version.