cmake: Respect user-provided configuration-specific flags #32356

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:250426-var-override changing 2 files +11 −8
  1. hebasto commented at 11:46 am on April 26, 2025: member

    This PR addresses this comment:

    I suppose that should only happen if the -O3 isn’t coming from an explicitly set CMAKE_CXX_FLAGS_RELEASE.

    With this PR:

    0$ cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"
1<snip>
2C++ compiler flags .................... -O3 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/home/hebasto/dev/bitcoin/src=. -fmacro-prefix-map=/home/hebasto/dev/bitcoin/src=. -fstack-reuse=none -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
3Linker flags .......................... -O3 -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie

    and

    0$ cmake -B build -DCMAKE_BUILD_TYPE=Release
1<snip>
2C++ compiler flags .................... -O2 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/home/hebasto/dev/bitcoin/src=. -fmacro-prefix-map=/home/hebasto/dev/bitcoin/src=. -fstack-reuse=none -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
3Linker flags .......................... -O2 -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie

    When calling cmake repeatedly using the same build directory, each newly provided CMAKE_CXX_FLAGS_RELEASE value will be accommodated. In such a scenario, if the user wishes to revert to the build system defaults, they should unset the CMAKE_CXX_FLAGS_RELEASE variable by passing -UCMAKE_CXX_FLAGS_RELEASE to cmake.

    This PR does not aim to resolve all issues mentioned in #31491.

  2. hebasto added the label Bug on Apr 26, 2025
  3. hebasto added the label Build system on Apr 26, 2025
  4. DrahtBot commented at 11:46 am on April 26, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32356.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK janb84, purpleKarrot, ryanofsky
    Concept ACK l0rinc

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  5. l0rinc commented at 12:44 pm on April 26, 2025: contributor

    Concept ACK

    Nit: the title is a bit ambiguous, not sure what “replacing them” mean in this context

  6. hebasto commented at 12:53 pm on April 26, 2025: member

    Concept ACK

    Nit: the title is a bit ambiguous, not sure what “replacing them” mean in this context

    I meant “we try to replace -O3 with -O2 but only when CMAKE_CXX_FLAGS_RELEASE is not specified by the user explicitly”. I’ll be happy to adjust wording for a better suggestion.

  7. l0rinc commented at 1:35 pm on April 26, 2025: contributor

    How about:

    0cmake: Respect user-provided compiler optimization level
  8. hebasto force-pushed on Apr 26, 2025
  9. hebasto commented at 6:20 pm on April 26, 2025: member
    The commit message has been amended.
  10. hebasto renamed this:
    cmake: Do not override flags set by the user when replacing them
    cmake: Respect user-provided configuration-specific flags
    on Apr 26, 2025
  11. in cmake/module/ProcessConfigurations.cmake:109 in 2f68531c53 outdated 
    112-    CACHE STRING
113-    "Flags used by the CXX compiler during ${config_uppercase} builds."
114-    FORCE
115-  )
116+  set(var_name CMAKE_CXX_FLAGS_${config_uppercase})
117+  if("${var_name}" IN_LIST precious_variables)
    janb84 commented at 8:40 am on April 28, 2025:
    NIT: precious_variables is used but not defined or passed into the function. Would it not be better (more pure) to pass the variable into the function ?
    purpleKarrot commented at 8:07 pm on May 5, 2025:
    Would it be possible to put get_directory_property(precious_variables CACHE_VARIABLES) here inside the function scope?
    hebasto commented at 12:45 pm on May 6, 2025:

    Would it be possible to put get_directory_property(precious_variables CACHE_VARIABLES) here inside the function scope?

    If you do that, it will collect additional cache variables set by subsequent commands such as project() and enable_language(). This list will include CMAKE_CXX_FLAGS_RELEASE, which breaks the patch.

  12. in cmake/module/ProcessConfigurations.cmake:112 in 2f68531c53 outdated 
    115-  )
116+  set(var_name CMAKE_CXX_FLAGS_${config_uppercase})
117+  if("${var_name}" IN_LIST precious_variables)
118+    return()
119+  endif()
120+  string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" ${var_name} "${${var_name}}")
    janb84 commented at 8:50 am on April 28, 2025: 
    0  string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" "${var_name}" "${${var_name}}")

    NIT: add quotes to ensure proper handling of variable name

    hebasto commented at 12:52 pm on May 6, 2025:
    The var_name contains no spaces, so I’ll leave it as is to avoid cluttering the code.
  13. in cmake/module/ProcessConfigurations.cmake:114 in 2f68531c53 outdated 
    117+  if("${var_name}" IN_LIST precious_variables)
118+    return()
119+  endif()
120+  string(REGEX REPLACE "(^| )${old_flag}( |$)" "\\1${new_flag}\\2" ${var_name} "${${var_name}}")
121+  set(${var_name} "${${var_name}}" PARENT_SCOPE)
122+  set_property(CACHE ${var_name} PROPERTY VALUE "${${var_name}}")
    janb84 commented at 8:52 am on April 28, 2025: 
    0  set_property(CACHE "${var_name}" PROPERTY VALUE "${${var_name}}")

    NIT: add quotes to ensure proper handling of variable name

    hebasto commented at 12:52 pm on May 6, 2025:
    The var_name contains no spaces, so I’ll leave it as is to avoid cluttering the code."
  14. janb84 commented at 8:55 am on April 28, 2025: contributor

    Few suggestions, although the changes do work as described (tested)

    Master: cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"

    0C++ compiler flags .................... -O2 -std=c++20 -fPIC -fdebug-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -fmacro-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -mbranch-protection=bti
1Linker flags .......................... -O2 -Wl,-dead_strip -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -fstack-protector-all -Wl,-fixup_chains -fPIE -Xlinker -pie

    This PR: cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"

    0C++ compiler flags .................... -O3 -std=c++20 -fPIC -fdebug-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -fmacro-prefix-map=/Users/arjan/Projects/boss/bitcoin/src=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -mbranch-protection=bti
1Linker flags .......................... -O3 -Wl,-dead_strip -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -fstack-protector-all -Wl,-fixup_chains -fPIE -Xlinker -pie

    (as described calling cmake again without -03 and without deleting the dir will still result in -03)

  15. purpleKarrot commented at 11:04 am on May 5, 2025: contributor

    The best approach to respect user-provided flags is to treat all CMAKE_... variables as read-only. The only place where those variables may be set, is outside the project (ie: env vars, toolchain files, initial cache, cli arguments).

    Consider you have a function in C. Inside that function, you have to perform a division by one of the arguments. Since you cannot divide by zero, you treat the value 0 as if the user provided the value 2 instead. Would you consider that a valid approach? Every C programmer should agree that it would be better to have a narrow contract that excludes the value 0 as input with assert(arg != 0); for example.

    The same applies in CMake. A project should not modify compile flags, irrespective of where the flags are defined. What a project may do, is provide a defensive error/warning when problematic flags are detected:

    0if(CMAKE_CXX_FLAGS MATCHES "-O3")
1   message(FATAL_ERROR "Building this project with -O3 is not supported.")
2endif()
  16. hebasto commented at 11:59 am on May 5, 2025: member

    The same applies in CMake. A project should not modify compile flags, irrespective of where the flags are defined. What a project may do, is provide a defensive error/warning when problematic flags are detected:

    0if(CMAKE_CXX_FLAGS MATCHES "-O3")
1   message(FATAL_ERROR "Building this project with -O3 is not supported.")
2endif()

    This results in behaviour where every configuration step using -DCMAKE_BUILD_TYPE=Release with the default flags ends in an error, which is a terrible UX.

  17. purpleKarrot commented at 12:51 pm on May 5, 2025: contributor

    This results in behaviour where every configuration step using -DCMAKE_BUILD_TYPE=Release with the default flags ends in an error, which is a terrible UX.

    Then don’t use -DCMAKE_BUILD_TYPE=Release. You get better UX with less typing.

    For local development, just use the default build type. CI and package builds are done with scripts that allow enough ways to specify compile flags.

    Distro packagers want to build all packages with the same compile flags. They will patch out all project specific hacks and also avoid CMake’s builtin build types. (For example, Debian uses build type None, see here). Any project specific hacks, like enforcing that the selected build type is one of Debug;Release;RelWithDebInfo;MinSizeRel or fiddling around compile flags creates trouble for them, because it means that they have to maintain a patch to remove that - in their view - nonsense.

  18. ryanofsky commented at 4:38 pm on May 5, 2025: contributor

    Concept ACK.

    I think I agree with @purpleKarrot that it is bad to override CMAKE_ variables, and in general the build does too much overriding (which I’ve mentioned before in places like #30813 (comment)).

    But it seems like the PR is moving in the right direction and making a strict improvement by overriding less aggressively than before. I think hebasto’s point about wanting CMAKE_BUILD_TYPE=Release (and release builds in multi-config environments like IDE’s) to work out of the box is pretty compelling and good reason to try to override the cmake’s -O3 default while trying not to override -O3 when it is specified externally.

  19. cmake: Respect user-provided configuration-specific flags edde96376a
  20. in cmake/module/ProcessConfigurations.cmake:109 in 2f68531c53 outdated 
    111-  set(CMAKE_CXX_FLAGS_${config_uppercase} "${new_flags}"
112-    CACHE STRING
113-    "Flags used by the CXX compiler during ${config_uppercase} builds."
114-    FORCE
115-  )
116+  set(var_name CMAKE_CXX_FLAGS_${config_uppercase})
    purpleKarrot commented at 8:08 pm on May 5, 2025: 
    0  string(TOUPPER "CMAKE_CXX_FLAGS_${config}" var_name)
    hebasto commented at 12:46 pm on May 6, 2025:
    Thanks! Updated.
  21. hebasto force-pushed on May 6, 2025
  22. hebasto commented at 12:47 pm on May 6, 2025: member
    The feedback from @purpleKarrot has been addressed.
  23. janb84 commented at 1:14 pm on May 6, 2025: contributor
    ACK edde963
  24. DrahtBot requested review from ryanofsky on May 6, 2025
  25. DrahtBot requested review from l0rinc on May 6, 2025
  26. purpleKarrot commented at 3:56 pm on May 6, 2025: contributor

    ACK edde96376a2961dec3730331b3d171ddf972589f

    it seems like the PR is moving in the right direction and making a strict improvement by overriding less aggressively than before.

    :+1:

  27. fanquake commented at 10:08 am on May 8, 2025: member

    @ryanofsky / @theuni want to circle back?

    I think we’ll also backport this for 29.x.

  28. fanquake added the label Needs backport (29.x) on May 8, 2025
  29. in CMakeLists.txt:24 in edde96376a 
    18@@ -19,6 +19,10 @@ if(POLICY CMP0171)
19   cmake_policy(SET CMP0171 NEW)
20 endif()
21 
22+# When adjusting CMake flag variables, we must not override those explicitly
23+# set by the user. These are a subset of the CACHE_VARIABLES property.
24+get_directory_property(precious_variables CACHE_VARIABLES)
    ryanofsky commented at 2:42 pm on May 8, 2025:

    In commit “cmake: Respect user-provided configuration-specific flags” (edde96376a2961dec3730331b3d171ddf972589f)

    Would be good for the comment here to note that this needs to be called before the project() command otherwise the list will include variables not actually set by the user. (I wouldn’t have known that unless I saw the discussion at #32356 (review))

  30. ryanofsky approved
  31. ryanofsky commented at 2:59 pm on May 8, 2025: contributor
    Code review ACK edde96376a2961dec3730331b3d171ddf972589f
  32. ryanofsky assigned ryanofsky on May 8, 2025
  33. ryanofsky merged this on May 8, 2025
  34. ryanofsky closed this on May 8, 2025
  35. hebasto deleted the branch on May 8, 2025
  36. fanquake referenced this in commit f9d2c67a0c on May 8, 2025
  37. fanquake removed the label Needs backport (29.x) on May 8, 2025
  38. fanquake commented at 3:37 pm on May 8, 2025: member
    Backported to 29.x in #32292.


hebasto DrahtBot l0rinc janb84 purpleKarrot ryanofsky fanquake


l0rinc

Labels
Bug Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-29 12:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me