cmake: Get rid of undocumented BITCOIN_GENBUILD_NO_GIT environment variable #32220

1. 
   hebasto commented at 3:05 pm on April 4, 2025: member

   In general, the Bitcoin Core build system attempts to fetch commit or tag details from git. This is handled by the cmake/script/GenerateBuildInfo.cmake script, which generates the src/bitcoin-build-info.h header within the build tree.

   However, there are cases where the retrieved details may be incorrect—for example, when building from a source tarball or as a subproject within a git-aware project.

   In the Autotools-based build system, the BITCOIN_GENBUILD_NO_GIT environment variable was introduced in v0.20.0 to address such scenarios:

   The process for generating the source code release (“tarball”) has changed in an effort to make it more complete, however, there are a few regressions in this release:

   • Instead of running make simply, you should instead run BITCOIN_GENBUILD_NO_GIT=1 make.

   This PR automagically handles both of the aforementioned cases and removes the need for BITCOIN_GENBUILD_NO_GIT.

   The user is still able to configure the build with -DCMAKE_DISABLE_FIND_PACKAGE_Git=ON to disable git execution manually, for reasons we don’t know in advance.

   Closes #31999.

2. hebasto added the label Build system on Apr 4, 2025
3. 
   DrahtBot commented at 3:05 pm on April 4, 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/32220.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept NACK	luke-jr

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32217 (Bugfix: Only use git for build info if the repository is actually the right one by luke-jr)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. hebasto added this to the milestone 29.0 on Apr 4, 2025
5. 
   luke-jr commented at 9:05 pm on April 4, 2025: member

   Concept NACK to removing the ability for the user to disable git execution manually (perhaps for reasons we don’t know in advance).
6. in cmake/script/GenerateBuildInfo.cmake:32 in 159b5a43a8 outdated

   27@@ -28,9 +28,12 @@ else()
   28   set(WORKING_DIR ${CMAKE_CURRENT_SOURCE_DIR})
   29 endif()
   30 
   31-set(GIT_TAG)
   32-set(GIT_COMMIT)
   33-if(NOT "$ENV{BITCOIN_GENBUILD_NO_GIT}" STREQUAL "1")
   34+set(IS_SOURCE_TARBALL FALSE)
   35+# git will put "set(IS_SOURCE_TARBALL TRUE)" on the next line inside archives. $Format:%nset(IS_SOURCE_TARBALL TRUE)$
   

   ---

   ---

   

   luke-jr commented at 9:08 pm on April 4, 2025:

   Maybe we should use $Format:%(describe)%$ to actually include build info?

   ---

   

   hebasto commented at 1:17 pm on April 7, 2025:

   Maybe we should use $Format:%(describe)%$ to actually include build info?

   Could you clarify how this improves the PR?

   ---

   

   luke-jr commented at 0:57 am on April 10, 2025:

   This way we can put the correct build info in, rather than just failing to get any build info.
7. 
   hebasto commented at 1:26 am on April 5, 2025: member

   Concept NACK to removing the ability for the user to disable git execution manually (perhaps for reasons we don’t know in advance).

   The user is still able to configure the build with -DCMAKE_DISABLE_FIND_PACKAGE_Git=ON.

8. 
   maflcko commented at 8:02 am on April 7, 2025: member

   The user is still able to configure the build with -DCMAKE_DISABLE_FIND_PACKAGE_Git=ON.

   Could mention in the pull description? Also, it looks like the tail needs editing? Ref:

   This PR handles with both mentioned cases automagically and gets rid of BITCOIN_GENBUILD_NO_GIT.

   This PR automagically handles both of the aforementioned cases and removes the need for BITCOIN_GENBUILD_NO_GIT.

9. 
   hebasto commented at 10:23 am on April 7, 2025: member

   The user is still able to configure the build with -DCMAKE_DISABLE_FIND_PACKAGE_Git=ON.

   Could mention in the pull description? Also, it looks like the tail needs editing? Ref:

   This PR handles with both mentioned cases automagically and gets rid of BITCOIN_GENBUILD_NO_GIT. This PR automagically handles both of the aforementioned cases and removes the need for BITCOIN_GENBUILD_NO_GIT.

   Thanks! Updated.

10. maflcko closed this on Apr 7, 2025

    ---

11. maflcko reopened this on Apr 7, 2025

    ---

12. hebasto marked this as a draft on Apr 7, 2025
13. hebasto force-pushed on Apr 7, 2025
14. hebasto marked this as ready for review on Apr 7, 2025
15. 
    whitslack commented at 1:45 pm on April 7, 2025: contributor

    The user is still able to configure the build with -DCMAKE_DISABLE_FIND_PACKAGE_Git=ON to disable git execution manually, for reasons we don’t know in advance.

    But that won’t work, will it? The find_package(Git) call is in cmake/script/GenerateBuildInfo.cmake, which is executed recursively in an independent CMake process by the generate_build_info custom target of src/CMakeLists.txt. (Did we really learn nothing from “Recursive Make Considered Harmful”? :man_facepalming:) The only reason BITCOIN_GENBUILD_NO_GIT was effective in the sub-CMake process is because the environment is inherited by child processes. CMake variable definitions are not inherited.

16. cmake, refactor: Move `find_package(Git)` to `src/CMakeLists.txt`

    This change allows the use of the `CMAKE_DISABLE_FIND_PACKAGE_Git`
    variable to explicitly disable git usage.

    54dcbb91a5
17. cmake: Remove unnecessary `BITCOIN_GENBUILD_NO_GIT` environment variable

    This functionality is now covered by the `CMAKE_DISABLE_FIND_PACKAGE_Git`
    variable.
    
    Note for reviewers: consider reviewing with `git diff -w`.

    eb6d4c2d08
18. cmake: Skip using git when building from source tarball or as subproject e2c21ba65c
19. hebasto force-pushed on Apr 7, 2025
20. 
    hebasto commented at 2:23 pm on April 7, 2025: member

    @whitslack

    The user is still able to configure the build with -DCMAKE_DISABLE_FIND_PACKAGE_Git=ON to disable git execution manually, for reasons we don’t know in advance.

    But that won’t work, will it? The find_package(Git) call is in cmake/script/GenerateBuildInfo.cmake, which is executed recursively in an independent CMake process by the generate_build_info custom target of src/CMakeLists.txt. (Did we really learn nothing from “Recursive Make Considered Harmful”? 🤦‍♂️) The only reason BITCOIN_GENBUILD_NO_GIT was effective in the sub-CMake process is because the environment is inherited by child processes. CMake variable definitions are not inherited.

    Thank you for your feedback!

    Reworked.

21. maflcko added the label DrahtBot Guix build requested on Apr 16, 2025

Contributors
hebasto DrahtBot luke-jr maflcko whitslack

Labels
Build system DrahtBot Guix build requested

Milestone
29.0