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 |
hebasto
force-pushed
the
cmake-staging
branch
from
ff2f574
to
dacc6f7
Compare
hebasto
force-pushed
the
240124-cmake-AZ
branch
from
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. |
hebasto
force-pushed
the
240124-cmake-AZ
branch
from
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. |
hebasto
force-pushed
the
240124-cmake-AZ
branch
from
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 |
hebasto
force-pushed
the
240124-cmake-AZ
branch
3 times, most recently
from
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.
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.
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.
hebasto
force-pushed
the
240124-cmake-AZ
branch
from
3d65ab6
to
d8ad47d
Compare
|
Rebased. |
hebasto
force-pushed
the
240124-cmake-AZ
branch
from
d8ad47d
to
31b4028
Compare
|
Trying to get my head around this in the next few days. |
hebasto
force-pushed
the
240124-cmake-AZ
branch
2 times, most recently
from
05b573b
to
6c76048
Compare
|
Rebased. |
There was a problem hiding this comment.
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. |
hebasto
force-pushed
the
240124-cmake-AZ
branch
2 times, most recently
from
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.
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).
hebasto
force-pushed
the
240124-cmake-AZ
branch
from
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.
hebasto
force-pushed
the
240124-cmake-AZ
branch
from
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_INITvariables instead of customDEPENDS_<LANGUAGE>_COMPILER_FLAGSones. 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=Nonewhen building with depends.Fixes cross compilation for macOS.
Allows the user to use
{C,CXX}FLAGSenvironment 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.