doc: swap CPPFLAGS for APPEND_CPPFLAGS #31819

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:no_more_cppflags changing 1 files +2 −2
  1. fanquake commented at 4:09 pm on February 7, 2025: member

    APPEND_CPPFLAGS will be understood by our CMake, whereas CPPFLAGS will not. Attempting what is currently documented will just give:

    0CMake Warning:
    1  Ignoring extra path from command line:
    2
    3   "CPPFLAGS=-DDEBUG_LOCKCONTENTION"
    
  2. doc: swap CPPFLAGS for APPEND_CPPFLAGS
    APPEND_CPPFLAGS will be understood by our CMake, whereas CPPFLAGS will
    not.
    ea687d2029
  3. DrahtBot commented at 4:09 pm on February 7, 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/31819.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto, fjahr

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

  4. DrahtBot added the label Docs on Feb 7, 2025
  5. hebasto approved
  6. hebasto commented at 4:13 pm on February 7, 2025: member
    ACK ea687d202934ee9aa26912cda21993da219cd418.
  7. purpleKarrot commented at 5:08 pm on February 7, 2025: none
    Where can I read about the decision to chose CPPFLAGS and APPEND_CPPFLAGS? I think those names are unfortunate and may cause confusion with cmake’s default handling of the CXXFLAGS environment variable.
  8. theuni commented at 6:03 pm on February 7, 2025: member

    @purpleKarrot It’s an artifact of a few things:

    • The migration from autotools to CMake. autotools made CPPFLAGS a first-class citizen whereas CMake expects users to just pass them in as CXXFLAGS. APPEND_CPPFLAGS could probably be rolled into APPEND_CXXFLAGS, we just maintain the split because it can be a helpful distinction.
    • In many cases CXXFLAGS or CMAKE_CXX_FLAGS or CMAKE_CXX_FLAGS_BUILDTYPE will work, but not always. CMake provides no way to append CXXFLAGS, only to prepend or replace. This is really annoying. Mostly because setting things where position matters (like -Ofoo or -UBAR -DBAR=2) is impossible with the built-in vars. Hence our APPEND variables.
  9. purpleKarrot commented at 6:26 pm on February 7, 2025: none

    CMake provides no way to append CXXFLAGS, …

    Not sure I understand. On the first run, CMake initializes CMAKE_CXX_FLAGS by prepending CMAKE_CXX_FLAGS_INIT with the environment variable CXXFLAGS. See: https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_INIT.html

    In other words, CMAKE_CXX_FLAGS_INIT is appended to CXXFLAGS. But you say it provides no way to append to CXXFLAGS?

  10. theuni commented at 6:36 pm on February 7, 2025: member

    I didn’t mean CXXFLAGS the env var, I meant CXXFLAGS the concept (as-in, what ultimately gets passed to the compiler). There are just some things that either we set internally or that CMake sets that are impossible for the user to override without affecting something else (by using a None build type for example).-Ox is one of them.

    So we provide these vars with guaranteed append semantics.

  11. theuni commented at 6:38 pm on February 7, 2025: member
    Btw, if your argument is “CXXFLAGS=-DDEBUG_LOCKCONTENTION would work for the sake of this PR”, I agree with you there.
  12. fjahr commented at 8:28 pm on February 8, 2025: contributor

    ACK ea687d202934ee9aa26912cda21993da219cd418

    Just used APPEND_CPPFLAGS yesterday for some debugging, so it works :)

  13. fanquake merged this on Feb 10, 2025
  14. fanquake closed this on Feb 10, 2025

  15. fanquake deleted the branch on Feb 10, 2025

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-03-14 09:13 UTC

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