Bugfix: Only use git for build info if the repository is actually the right one

luke-jr commented at 11:21 pm on April 3, 2025: member

cmake port of fix originally fixed for autotools in #7522

Original-Github-Pull: #7522

903fc971ed

DrahtBot commented at 11:21 pm on April 3, 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/32217.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32220 (cmake: Get rid of undocumented BITCOIN_GENBUILD_NO_GIT environment variable by hebasto)

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.

fanquake commented at 11:57 pm on April 3, 2025: member

So this is fixing a CMake regression? Also seems like the obvious time to document BITCOIN_GENBUILD_NO_GIT (#31999) cc @whitslack

in cmake/script/GenerateBuildInfo.cmake:57 in 903fc971ed

52+      )
53+      if(SCRIPT_STATUS_EXITCODE)
54+        set(IS_INSIDE_WORK_TREE 0)
55+      elseif(SCRIPT_STATUS MATCHES "^$|\\?")
56+        set(IS_INSIDE_WORK_TREE 0)
57+      endif()

whitslack commented at 1:27 am on April 4, 2025:

Does CMake have no logical-or operator that could be used to unite these two conditional blocks?

fanquake added this to the milestone 29.0 on Apr 4, 2025

hebasto commented at 12:46 pm on April 4, 2025: member

So this is fixing a CMake regression?

If so, could it be demonstrated that something works in v28.1 but is broken in v29.0rc3?

in cmake/script/GenerateBuildInfo.cmake:54 in 903fc971ed

49+        OUTPUT_VARIABLE SCRIPT_STATUS
50+        OUTPUT_STRIP_TRAILING_WHITESPACE
51+        ERROR_QUIET
52+      )
53+      if(SCRIPT_STATUS_EXITCODE)
54+        set(IS_INSIDE_WORK_TREE 0)

hebasto commented at 12:48 pm on April 4, 2025:

In what real-life scenarios is this branch executed?

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

I don’t know of any, it’s included for correctness since the alternative is clearly wrong.

in cmake/script/GenerateBuildInfo.cmake:56 in 903fc971ed

51+        ERROR_QUIET
52+      )
53+      if(SCRIPT_STATUS_EXITCODE)
54+        set(IS_INSIDE_WORK_TREE 0)
55+      elseif(SCRIPT_STATUS MATCHES "^$|\\?")
56+        set(IS_INSIDE_WORK_TREE 0)

hebasto commented at 12:48 pm on April 4, 2025:

In what real-life scenarios is this branch executed?

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

If any parent directory is a git repository, but the source directory is not

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

It is unnecessary to run git for that. I believe #32220 presents a superior and fully functional alternative to this PR.

sipa commented at 6:10 pm on April 8, 2025:

I disagree somewhat @hebasto, though I don’t think it’s very important.

Sure, the user can disable git entirely if they know they’re not building directly in a git working tree for the repository, but it doesn’t seem unreasonable to me that the code automatically works correctly by not treating it as a git build if it happens to be unpacked tarball in an unrelated git working tree.

That said, if this already doesn’t work correctly it’s not a blocker for 29.x.

hebasto commented at 6:52 pm on April 8, 2025:

@sipa

Sure, the user can disable git entirely if they know they’re not building directly in a git working tree for the repository, but it doesn’t seem unreasonable to me that the code automatically works correctly by not treating it as a git build if it happens to be unpacked tarball in an unrelated git working tree.

I’ve read your comment a few times, and I’m fairly sure I agree with you 100%. Now I’m wondering — which (part) of my comments do you disagree with?

sipa commented at 6:55 pm on April 8, 2025:

You suggest that it is possible to find out, without running git, that a parent directory is a git repository, but the source directory is not. I don’t see how.

You suggest that #32220 is an alternative to this PR, but as far as I can tell, it offers no way to detect the use case this PR is concerned about.

hebasto commented at 7:28 pm on April 8, 2025:

Thank you for clarification!

#32220 handles cases when building from source tarballs: https://github.com/bitcoin/bitcoin/blob/e2c21ba65cdb898d24c344a666c1464abfe8bce0/src/CMakeLists.txt#L82-L86

sipa commented at 7:30 pm on April 8, 2025:

Oh, I see! That only works if it’s a git-created tarball, and not a source tree copied over from somewhere, but perhaps that is enough.

luke-jr commented at 1:00 am on April 10, 2025:

It’s not enough: a git work tree appears similar to “copied over from somewhere” if the build user does not have permissions to access the git repo.

laanwj added the label Build system on Apr 7, 2025

hebasto commented at 12:49 pm on April 7, 2025: member

So this is fixing a CMake regression?

If so, could it be demonstrated that something works in v28.1 but is broken in v29.0rc3?

The PR’s author mentioned only one explicit case where this PR might alter behaviour:

If any parent directory is a git repository, but the source directory is not

Accordingly, I have verified any related behavioural changes in this scenario:

Creating an empty git repository:

0$ mkdir test_tarballs
1$ cd test_tarballs
2$ git init .
3$ git commit --allow-empty -m "Initial commit"
4$ git rev-parse --short=12 HEAD
576e386813b40

Testing the v28.1 tarball:

0$ wget https://bitcoincore.org/bin/bitcoin-core-28.1/bitcoin-28.1.tar.gz
1$ tar xf bitcoin-28.1.tar.gz 
2$ cd bitcoin-28.1
3$ ./autogen.sh 
4$ ./configure 
5$ make -j $(nproc)
6$ cat src/obj/build.h
7#define BUILD_GIT_COMMIT "76e386813b40"
8$ ./src/bitcoind -version | head -1
9Bitcoin Core version v28.1.0

Testing the v29.0rc3 tarball:

0$ wget https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc3/bitcoin-29.0rc3.tar.gz
1$ tar xf bitcoin-29.0rc3.tar.gz 
2$ cd bitcoin-29.0rc3
3$ cmake -B build
4$ cmake --build build -j $(nproc)
5$ cat build/src/bitcoin-build-info.h
6#define BUILD_GIT_COMMIT "76e386813b40"
7$ ./build/bin/bitcoind -version | head -1
8Bitcoin Core daemon version v29.0.0rc3

Testing a pre-release untagged commit from the 28.x branch:

0$ tar xf /home/hebasto/GIT_ARCHIVES/bitcoin-1248d0da2279.tar.gz
1$ cd bitcoin-1248d0da2279
2$ ./autogen.sh 
3$ ./configure 
4$ make -j $(nproc)
5$ cat src/obj/build.h
6#define BUILD_GIT_COMMIT "76e386813b40"
7hebasto@TS-P340:~/dev/test_tarballs/bitcoin-1248d0da2279$ ./src/bitcoind -version | head -1
8Bitcoin Core version v28.99.0-76e386813b40

Testing a pre-release untagged commit from the 29.x branch:

0$ tar xf /home/hebasto/GIT_ARCHIVES/bitcoin-8cb6ab0b971a.tar.gz
1$ cd bitcoin-8cb6ab0b971a
2$ cmake -B build
3$ cmake --build build -j $(nproc)
4$ cat build/src/bitcoin-build-info.h
5#define BUILD_GIT_COMMIT "76e386813b40"
6$ ./build/bin/bitcoind -version | head -1
7Bitcoin Core daemon version v28.99.0-76e386813b40

As demonstrated above, there is no change in behaviour and no indication of a “CMake regression”.

Maybe remove this PR from the 29.0 milestone?

hebasto commented at 1:13 pm on April 7, 2025: member

Tested 903fc971ed6b13da7260f5e5a8e3e9e7372b2bd7 on Ubuntu 24.10.

When building from my local Bitcoin Core repository:

on the master branch @ 8cb6ab0b971a27ba255ad6a48959a8c7b84c00f3:

0$ cmake --build build -t bitcoin_clientversion
1[1/3] Generating bitcoin-build-info.h
2$ cat build/src/bitcoin-build-info.h 
3#define BUILD_GIT_COMMIT "8cb6ab0b971a"

this PR:

0$ cmake --build build -t bitcoin_clientversion
1[3/3] Linking CXX static library lib/libbitcoin_clientversion.a
2$ cat build/src/bitcoin-build-info.h 
3// No build information available

The PR’s behaviour is evidently incorrect.

glozow removed this from the milestone 29.0 on Apr 7, 2025

luke-jr commented at 1:04 am on April 10, 2025: member

As demonstrated above, there is no change in behaviour and no indication of a “CMake regression”.

The regression occurred in #18556 and would have been fixed by #18902

Testing the v29.0rc3 tarball:

0$ wget https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc3/bitcoin-29.0rc3.tar.gz
1$ tar xf bitcoin-29.0rc3.tar.gz 
2$ cd bitcoin-29.0rc3
3$ cmake -B build
4$ cmake --build build -j $(nproc)
5$ cat build/src/bitcoin-build-info.h
6#define BUILD_GIT_COMMIT "76e386813b40"
7$ ./build/bin/bitcoind -version | head -1
8Bitcoin Core daemon version v29.0.0rc3

v29.0rc3 is not 76e386813b40

hebasto commented at 9:44 am on April 10, 2025: member

As demonstrated above, there is no change in behaviour and no indication of a “CMake regression”.

The regression occurred in #18556 and would have been fixed by #18902

So, it’s not caused by the migration from Autotools to CMake.

Testing the v29.0rc3 tarball:

0$ wget https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc3/bitcoin-29.0rc3.tar.gz
1$ tar xf bitcoin-29.0rc3.tar.gz 
2$ cd bitcoin-29.0rc3
3$ cmake -B build
4$ cmake --build build -j $(nproc)
5$ cat build/src/bitcoin-build-info.h
6#define BUILD_GIT_COMMIT "76e386813b40"
7$ ./build/bin/bitcoind -version | head -1
8Bitcoin Core daemon version v29.0.0rc3

v29.0rc3 is not 76e386813b40

Sure it is not. Because the build happens within another “parent” git repository:

If any parent directory is a git repository, but the source directory is not

Accordingly, I have verified any related behavioural changes in this scenario:

Creating an empty git repository:
0$ mkdir test_tarballs
1$ cd test_tarballs
2$ git init .
3$ git commit --allow-empty -m "Initial commit"
4$ git rev-parse --short=12 HEAD
576e386813b40

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

Bugfix: Only use git for build info if the repository is actually the right one #32217

Code Coverage & Benchmarks

Reviews

Conflicts