cmake: passing options from depends build to main build #30813

issue ryanofsky openend this issue on September 4, 2024
  1. ryanofsky commented at 3:21 pm on September 4, 2024: contributor

    Originally posted by @hebasto in #30800 (comment)

    I think we should first prioritize coordinating the optimization and debugging flags between the depends subsystem and the main build system:

    1. Should the optimization and debugging flags coincide, or are they allowed to differ?
    2. If the optimization and debugging can differ between the depends subsystem and in the main build system, which should take precedence?
  2. fanquake commented at 3:23 pm on September 4, 2024: member
    I think this is a duplicate of #29796? (or at least, it’d be good to try and keep the discussion in one place).
  3. ryanofsky commented at 3:25 pm on September 4, 2024: contributor

    Created a separate issue for this because I have some thoughts about it and don’t want to hijack the discussion in #30800 which is more about _FORTIFY_SOURCE and detecting which optimization flags are set (as opposed to setting them). Working on an idea which I’ll post below.

    I saw #29796 too, but it seems to be a discussion of which optimization flags are set, not how the flags passed from the depends build to the main build.

  4. fanquake commented at 3:39 pm on September 4, 2024: member

    I saw #29796 too, but it seems to be a discussion of which optimization flags are set, not how the flags are set.

    Sure. If the discussion is in general, about how options should be passed from depends to CMake, then I think (ideally) the answer is just, a toolchain file, and our CMake build should take (pretty much verbatim) what is set in that file (and not try and detect if it’s a “depends” build, and behave differently if that is the case). That seems like the only way to do things, if we want to have an interface into our CMake build system, that isn’t Core specific, which can be used by any other projects/builders/distros etc, that will/or might want to bring their own toolchains.

    Should the optimization and debugging flags coincide, or are they allowed to differ?

    Not sure I understand this question, but they have to be allowed to differ. There’s no “correct” answer for what the optimisation flags might be when debugging.

    If the optimization and debugging can differ between the depends subsystem and in the main build system, which should take precedence?

    I guess this is, in the default case, answered by the above, but also requires a change like #29796, so the options are coherent.

  5. ryanofsky commented at 3:43 pm on September 4, 2024: contributor

    I think the way things would work ideally is that depends system would set default build flags, and main build system would pick them up reliably, but the defaults could be freely overridden and edited by users running ccmake or passing -D values to cmake. Specifically this would mean:

    1. Depends settings in the toolchain file would reliably take precedence over cmake defaults and over defaults normally set by the main build system.
    2. User-defined -D and ccmake settings would reliably take precedence over depends settings in the toolchain file.

    Right now looking at depends/toolchain.cmake.in it seems like both of these things can happen in some cases, but neither of them happen reliably. Specifically in the current system:

    • The toolchain file force-writes into the cache and overwrites user settings.
    • The toolchain file is only able to weakly set *_INIT build flags that are later modified by cmake, instead of setting variables that will be used reliably.
    • The depends system sets DEPENDS_COMPILE_DEFINITIONS* variables that don’t show up in the cmake configuration and users can’t edit.

    I think to implement desired behavior, toolchain.cmake.in could be changed to:

    1. Stop setting cache variables like BUILD_GUI and WITH_QRENCODE and instead set custom non-cache variables like DEFAULT_BUILD_GUI DEFAULT_WITH_QRENCODE.
    2. Stop setting CMAKE_*_FLAGS*_INIT variables and instead set custom DEFAULT_*_FLAGS* variables.
    3. Stop setting DEPENDS_COMPILE_DEFINITIONS* variables and instead use DEFAULT_*_FLAGS*.

    To support this, main build system CMakeLists.txt file could be changed to:

    1. Use DEFAULT_* variables (if set), as default values for corresponding * options.
    2. Use DEFAULT_*_FLAGS* variables (if set) as default values for CMAKE_*_FLAGS* variables.

    Implementing the second part is a little tricky because setting CMAKE_*_FLAGS* directly would override user values. And CMake does provide CMAKE_*_FLAGS*_INIT variables to be able to influence the defaults of the corresponding flag variables, but these are not used verbatim. For example cmake appends -g to CMAKE_CXX_FLAGS_DEBUG_INIT after the toolchain sets it, uses the modified value as the default CMAKE_CXX_FLAGS_DEBUG value before it is cached. There are number of solutions for this listed in https://stackoverflow.com/questions/28732209/change-default-value-of-cmake-cxx-flags-debug-and-friends-in-cmake but it seems like a very clean one is to set the CMAKE_USER_MAKE_RULES_OVERRIDE hook variable to point to a cmake file that contains:

    0if (DEFINED DEFAULT_CXX_FLAGS_DEBUG)
    1  set(CMAKE_CXX_FLAGS_DEBUG_INIT "${DEFAULT_CXX_FLAGS_DEBUG}")
    2endif()
    

    repeated for different DEFAULT_*_FLAGS* variables

    I think also for this to work, our current ProcessConfigurations.cmake file would need to be tweaked to not force-set cache variables that have a corresponding DEFAULT_* variable set.


    I only did a little experimentation with toolchain and make_rules_overrides settings, so I’m not sure if this idea fully works, or is the best way implement the desired behavior.

    I’m also not sure the desired behavior described above (having depends defaults take precedence over non-depends defaults, and not letting depends system clobber or ignore values explicitly set by the user) is really the best behavior either. Maybe it’s not safe or flexible enough, or wouldn’t work in an important use-case. Just posting this as a suggestion to consider or discuss.

  6. fanquake added the label Build system on Sep 5, 2024
  7. fanquake added the label Brainstorming on Sep 5, 2024
  8. hebasto commented at 12:05 pm on September 6, 2024: member

    Right now looking at depends/toolchain.cmake.in it seems like both of these things can happen in some cases, but neither of them happen reliably. Specifically in the current system:

    • The toolchain file force-writes into the cache and overwrites user settings.

    I think to implement desired behavior, toolchain.cmake.in could be changed to:

    1. Stop setting cache variables like BUILD_GUI and WITH_QRENCODE and instead set custom non-cache variables like DEFAULT_BUILD_GUI DEFAULT_WITH_QRENCODE.

    I don’t think this describes the current behaviour precisely. Let’s consider these lines of code:https://github.com/bitcoin/bitcoin/blob/0e5cd608da5d8c3d9c758dbfa3fc36df4af4a100/depends/toolchain.cmake.in#L114-L119

    CMake documents the set(... CACHE ...) command behaviour as follows:

    Since cache entries are meant to provide user-settable values this does not overwrite existing cache entries by default.

  9. hebasto commented at 8:51 pm on September 6, 2024: member

    it seems like a very clean one is to set the CMAKE_USER_MAKE_RULES_OVERRIDE hook

    FWIW, something similar was considered during work on the CMake staging branch.

  10. ryanofsky commented at 10:34 pm on September 6, 2024: contributor

    I don’t think this describes the current behaviour precisely.

    Yes this is definitely not describing current behavior, it’s trying to describe how current behavior “could be changed” to avoid writing the cache. The first part of #30813 (comment) describes ideal behavior and contrasts it with current behavior, and the second part describes a specific way I think ideal behavior could be implemented.

    I think current behavior is ok, but if we are running into problems or are looking for ways to improve it, this approach could get rid of current limitations and remove surprises.

  11. ryanofsky commented at 10:49 pm on September 6, 2024: contributor

    it seems like a very clean one is to set the CMAKE_USER_MAKE_RULES_OVERRIDE hook

    FWIW, something similar was considered during work on the CMake staging branch.

    Interesting! Although that PR was using CMAKE_USER_MAKE_RULES_OVERRIDE it was doing something pretty different with it. It was overriding cmake’s low level compile command lines, which is definitely not something I would suggest doing here.

    Suggestion above is to use CMAKE_USER_MAKE_RULES_OVERRIDE only to set _INIT variables more reliably than they can be set from the toolchain file. If they are set in the toolchain file, cmake will modify the _INIT values and append from the environment, while if they are set in the override file, cmake will use the set values with no changes. Doing this may be helpful if we want to give depends system more control over the flags that are used and get rid of DEPENDS_COMPILE_DEFINITIONS* variables. I believe it would be good to get rid of DEPENDS_COMPILE_DEFINITIONS* variables because they are not visible in the cache, not customizable, and nonstandard while also being redundant with standard variables.

  12. hebasto commented at 10:39 am on September 7, 2024: member

    I don’t think this describes the current behaviour precisely.

    Yes this is definitely not describing current behavior, it’s trying to describe how current behavior “could be changed” to avoid writing the cache.

    I’m still confused. Why should we “avoid writing the cache” for variables such as WITH_ZMQ? Using cache variables is a standard CMake method that allows the user to set their values.

  13. hebasto commented at 11:25 am on September 7, 2024: member

    For example cmake appends -g to CMAKE_CXX_FLAGS_DEBUG_INIT after the toolchain sets it

    That’s true. From CMake CMAKE_<LANG>_FLAGS_<CONFIG>_INIT variable’s docs:

    CMake may prepend or append content to the value based on the environment and target platform.

    Suggestion above is to use CMAKE_USER_MAKE_RULES_OVERRIDE only to set _INIT variables more reliably than they can be set from the toolchain file.I agree that this approach would work. But it has a drawback making the main build system less compatible with toolchain files provided by other developers / distro maintainers etc.

    I agree that this approach would work, but it has a drawback: it makes the main build system less compatible with toolchain files provided by other developers, distro maintainers, etc.

  14. ryanofsky commented at 12:37 pm on September 9, 2024: contributor

    I’m still confused. Why should we “avoid writing the cache” for variables such as WITH_ZMQ? Using cache variables is a standard CMake method that allows the user to set their values.

    Sorry that was not clear. I should have said that “force writing the cache” and overwriting values set by users should be avoided. Ideal behavior is that user can run depends build and it will set default cache values. Then they can run ccmake or plain cmake and change settings freely and those values will be respected, instead of overwritten. Approach where depends build provides non-cached DEFAULT_<option> values instead of force-setting <option> values in the cache provides this, and avoid user value being silently discarded, which happens currently.

    I agree that this approach would work, but it has a drawback: it makes the main build system less compatible with toolchain files provided by other developers, distro maintainers, etc.

    This is not true because suggestion is for toolchain to set DEFAULT_<option> variables, not to overwrite <option> variables. If the CMAKE_USER_MAKE_RULES_OVERRIDE file is populated as suggested with:

    0if (DEFINED DEFAULT_CXX_FLAGS_DEBUG)
    1  set(CMAKE_CXX_FLAGS_DEBUG_INIT "${DEFAULT_CXX_FLAGS_DEBUG}")
    2endif()
    

    The build would be compatible with outside or pre-existing toolchain files because those toolchains would not define DEFAULT_<option> values, so any <option>_INIT variables they did set would be unchanged.

  15. hebasto commented at 11:42 am on September 25, 2024: member
    Additionally, if we remove settings of non-toolchain variables, such as BUILD_GUI or ENABLE_WALLET from the toolchain file, it allows the reuse of this toolchain file to build depends themselves, instead of relying on this code:https://github.com/bitcoin/bitcoin/blob/39219fe145e5e6e6f079b591e3f4b5fea8e71804/depends/funcs.mk#L173-L199
  16. hebasto commented at 10:27 am on October 2, 2024: member

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-01-06 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me