-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmake: Rework handling compiler flags #82
Conversation
Added a new commit, which suggests a middle-ground, i.e. reordering flags from the |
It's not really clear to me from the PR description what is changing, or how someone should/would use the build system after this change. Do standard C/CXX/LD flags still work? Are they equivalent to
I don't really like this approach at all. We shouldn't have to make up non-standard flags/env vars just to do basic configuration of a build system. |
The
No, and have never been. https://discourse.cmake.org/t/strictly-appending-to-cmake-lang-flags/6478/5:
The |
ff2f574
to
dacc6f7
Compare
d693ad9
to
b834740
Compare
Rebased. |
Not sure about this patch. It is currently misreporting the contents of the If providing an empty configuration in combination with environment variables, or passing the mentioned build-type suffixed arguments Alternatively we could lean in to it and provide more build type configurations, or cmake presets as the need arises. I get that presets are only supported starting cmake 3.19, but it is something that users could opt in to. |
b834740
to
6dcdbc5
Compare
We discussed this with @TheCharlatan offline.
No, we shouldn't. Following the accepted CMake practices is more future-proof way.
The only reason, at least for me, was to achieve 100% compatible to the current build system behavior. This PR has been updated to leverage native CMake practices. |
I have another argument in support of the current PR branch. The
|
Both of these sound like a problem of not setting a proper minimum version, that shouldn't be the case when we are cross-compiling, because the minimum version is set in our c and cxx flags? Is this only an issue with the current CMake branch? |
Yes. This PP is a bug fix. |
Just a quick comment as I review: it would be very helpful to add more meaningful commit messages to the fixup commits. I understand that they'll be squashed away eventually, but it would save reviewers some time trying to figure out exactly what is being fixed by each one. |
6dcdbc5
to
a0d8113
Compare
Sure. The commit messages have been improved. |
Is there are reason CFLAGS ................................ -O3 -flto
C++ compiler .......................... /Library/Developer/CommandLineTools/usr/bin/c++
CXXFLAGS .............................. -O3 -flto
Common compile options ................ -Wstack-protector -fstack-protector-all -mbranch-protection=bti
Common link options ................... -Wl,-dead_strip -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -Wl,-fixup_chains
Linker flags for executables ..........
Linker flags for shared libraries .....
Build type (configuration):
- CMAKE_BUILD_TYPE ................... RelWithDebInfo
- Preprocessor defined macros ........
- CFLAGS ............................. -O2 -g
- CXXFLAGS ........................... -O2 -g
- LDFLAGS for executables ............
- LDFLAGS for shared libraries ....... This is confusing, and it also means you don't actually know the flag ordering? i.e in this case. Is |
The flag ordering is known: configuration-agnostic flags are followed by the configuration-specific one. That's why the have the same order in the summary. Also please consider multi-configuration support:
|
Sure, but this isn't obivous or intuitive. Is there any reason we can't have a single line of the compiler flags that are going to be used, with clear ordering. Rather than having multiple lines, and ambiguity. |
Added a commit, which addresses @fanquake's comment. |
|
This is better, but I still think we can improve this. It's still not clear why c and cxxflags are separete from common compile options. They are also "common compile options" ? |
CMake treats
Common compile/link options are encapsulated in the |
b8af63f
to
d4a99f5
Compare
Reworked. The PR description updated. |
-e 's|@CFLAGS@|$(strip $(host_CFLAGS) $(host_$(release_type)_CFLAGS))|' \ | ||
-e 's|@CXXFLAGS@|$(strip $(host_CXXFLAGS) $(host_$(release_type)_CXXFLAGS))|' \ | ||
-e 's|@CPPFLAGS@|$(strip $(host_CPPFLAGS) $(host_$(release_type)_CPPFLAGS))|' \ | ||
-e 's|@CFLAGS@|$(strip $(host_CFLAGS))|' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only doing 1 type of build at a time, so why do we need to split all of this up into 3 different variables for each flag? This wont scale if there are more build types added in the future (which could be a possibility with the use of presets etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to convey build type specific flags to the CMAKE_<LANGUAGE>_FLAGS_<CONFIG>
variable, not to CMAKE_<LANGUAGE>_FLAGS
. Combined with other changes in this PR, this fixes the issue with overriding optimisation flags form depends with per-configure flags. That means, no need to use the non-documented "None" build type any more.
3d65ab6
to
d8ad47d
Compare
Rebased. |
d8ad47d
to
31b4028
Compare
Trying to get my head around this in the next few days. |
05b573b
to
6c76048
Compare
Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's already clear for you and I'm just not used to it yet but we shouldn't make it more explicit in the summary what are the defaults/ initial values (CFLAGS and CXXFLAG
, eg: replace_cxx_flag_in_config(Debug -g -g3)
), the values set by the user (eg DCMAKE_CXX_FLAGS=
) and the ones set at compiling depends time (CMAKE_<LANGUAGE>_FLAGS_INIT
)?
Like which ones are getting replaced/ overriden or don't match (eg when compiler diagnostic flags are added - #84).
I do agree with @TheCharlatan on the presets at some point in the future if possible + that the user also could still add flags (if possible).
Rebased. |
3cf702f
to
fe87888
Compare
There are a few way to justify this branch changes:
To recap the CMake's logic, let's consider C++ compiler flags only for brevity:
Interacting with own depends build system is quite a unique property I did not find in other open source projects that uses CMake. It is basically cross-compiling, which implies using toolchain files. It is required that significant flags are set in the toolchainfile. However, CMake allows keeping such flags as a part of a compiler invocation command, but that is no longer the case after bitcoin#29233.
So, this PR follows CMake philosophy and is compatible with the current set of use cases. |
@@ -462,9 +492,27 @@ setup_split_debug_script() | |||
add_maintenance_targets() | |||
add_windows_deploy_target() | |||
|
|||
|
|||
if(CMAKE_CROSSCOMPILING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving a note that we should probably document this somehow. It was not clear to me that every depends build we do is considered a cross-compile (even if it isn't a cross compile).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #125.
fe87888
to
d268b29
Compare
Rebased on top of the #123. |
I've updated hebasto/oss-fuzz#3 for reviewers' convenience. |
1. Use standard CMake's way to pass flags from depends to the main build system using CMAKE_<LANGUAGE>_FLAGS_INIT variables instead of custom DEPENDS_<LANGUAGE>_COMPILER_FLAGS ones. This guaranties using those flags during various checks at the configuration stage. 2. Setting flags is decoupled from setting compilers in the toolchain file. 3. Per-configuration flags are cached properly. 4. No longer needed to set -DCMAKE_BUILD_TYPE=None when building with depends. Fixes cross compilation for macOS. Allows the user to use {C,CXX}FLAGS environment variables for depends and the main build system without drawbacks/workarounds.
If any of {C,CXX,CPP,LD}FLAGS is specified it should be assigned to a non-type-specific variable.
d268b29
to
dbd2f7a
Compare
My Guix builds:
|
ACK, think we should move forward here.
|
b231e61 depends: Add `CMAKE_SYSTEM_VERSION` to CMake toolchain file (Hennadii Stepanov) 6ff697a depends: Rename `cmake_system` > `cmake_system_name` (Hennadii Stepanov) a856151 [FIXUP] cmake: Do not trigger cross-compiling unnecessarily (Hennadii Stepanov) Pull request description: As it was [pointed](#82 (comment)) out during today's call, the toolchain file, which is generated by the depends build system, unnecessarily triggers CMake's [cross-compiling](https://cmake.org/cmake/help/latest/variable/CMAKE_CROSSCOMPILING.html) mode by setting the [`CMAKE_SYSTEM_NAME`](https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html) variable even the host and build systems coincides. That behavior deviates from the current master branch and potentially might have unwanted/unexpected side effects. This PR fixes this issue. All Guix builds remain cross-compiling. Top commit has no ACKs. Tree-SHA512: a42d4b4dc2d0f0698d75255254876b6c16b12d5f3380cd647985e1650f9859a455d0aaf507fa6297e92c7330dfbf152045618759ccce49b804512bf1fc6713e9
Use standard CMake's way to pass flags from depends to the main build system using
CMAKE_<LANGUAGE>_FLAGS_INIT
variables instead of customDEPENDS_<LANGUAGE>_COMPILER_FLAGS
ones. This guaranties using those flags during various checks at the configuration stage.Setting flags is decoupled from setting compilers in the toolchain file.
Per-configuration flags are cached properly.
No longer needed to set
-DCMAKE_BUILD_TYPE=None
when building with depends.Fixes cross compilation for macOS.
Allows the user to use
{C,CXX}FLAGS
environment variables for depends and the main build system without drawbacks/workarounds.The flags presentation in the summary (#82 (comment)) will be reworked in a separated PR.