Bugfix: Only use git for build info if the repository is actually the right one by luke-jr · Pull Request #32217 · bitcoin/bitcoin

luke-jr · 2025-04-03T23:21:21Z

cmake port of fix originally fixed for autotools in #7522

DrahtBot · 2025-04-03T23:21:23Z

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.

Type	Reviewers
Concept NACK	hebasto

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

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 · 2025-04-03T23:57:44Z

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

whitslack · 2025-04-04T01:27:56Z

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

This comment is still unaddressed.

hebasto · 2025-04-04T12:46:11Z

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?

hebasto · 2025-04-04T12:48:40Z

In what real-life scenarios is this branch executed?

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

hebasto · 2025-04-04T12:48:50Z

In what real-life scenarios is this branch executed?

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

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

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.

@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?

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.

Thank you for clarification!

#32220 handles cases when building from source tarballs:

bitcoin/src/CMakeLists.txt

Lines 82 to 86 in e2c21ba

set(IS_SOURCE_TARBALL FALSE)

# git will put "set(IS_SOURCE_TARBALL TRUE)" on the next line inside archives. $Format:%nset(IS_SOURCE_TARBALL TRUE)$

if(NOT IS_SOURCE_TARBALL AND PROJECT_IS_TOP_LEVEL)

find_package(Git QUIET)

endif()

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.

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.

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.

This case is handled well in the master branch, as git rev-parse --is-inside-work-tree 2> /dev/null will be empty.

hebasto · 2025-04-07T12:49:50Z

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:

$ mkdir test_tarballs
$ cd test_tarballs
$ git init .
$ git commit --allow-empty -m "Initial commit"
$ git rev-parse --short=12 HEAD
76e386813b40

Testing the v28.1 tarball:

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

Testing the v29.0rc3 tarball:

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

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

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

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

$ tar xf /home/hebasto/GIT_ARCHIVES/bitcoin-8cb6ab0b971a.tar.gz
$ cd bitcoin-8cb6ab0b971a
$ cmake -B build
$ cmake --build build -j $(nproc)
$ cat build/src/bitcoin-build-info.h
#define BUILD_GIT_COMMIT "76e386813b40"
$ ./build/bin/bitcoind -version | head -1
Bitcoin 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 · 2025-04-07T13:13:00Z

Tested 903fc971ed6b13da7260f5e5a8e3e9e7372b2bd7 on Ubuntu 24.10.

When building from my local Bitcoin Core repository:

on the master branch @ 8cb6ab0:

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

this PR:

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

The PR's behaviour is evidently incorrect.

luke-jr · 2025-04-10T01:04:09Z

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:

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

v29.0rc3 is not 76e386813b40

hebasto · 2025-04-10T09:44:58Z

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:

$ wget https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc3/bitcoin-29.0rc3.tar.gz
$ tar xf bitcoin-29.0rc3.tar.gz 
$ cd bitcoin-29.0rc3
$ cmake -B build
$ cmake --build build -j $(nproc)
$ cat build/src/bitcoin-build-info.h
#define BUILD_GIT_COMMIT "76e386813b40"
$ ./build/bin/bitcoind -version | head -1
Bitcoin 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:
$ mkdir test_tarballs
$ cd test_tarballs
$ git init .
$ git commit --allow-empty -m "Initial commit"
$ git rev-parse --short=12 HEAD
76e386813b40

DrahtBot · 2025-04-18T15:12:08Z

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit `247e9de` (master)	commit f52244298d0fb27e0157d02f3cb880a1e8ae1546 (pull/32217/merge)
*-aarch64-linux-gnu-debug.tar.gz	`22a79eff8e36e1bf...`	`b3e21214b511ef7a...`
*-aarch64-linux-gnu.tar.gz	`cbb887b1068cbcb2...`	`e8825e5f7d37959d...`
*-arm-linux-gnueabihf-debug.tar.gz	`d5b1583e9e776bdb...`	`a03c7376d91b5972...`
*-arm-linux-gnueabihf.tar.gz	`067ec12478462110...`	`aeb3332f0a3bca1e...`
*-arm64-apple-darwin-codesigning.tar.gz	`dd98a2df7a886aa2...`	`1af1a7d882467be5...`
*-arm64-apple-darwin-unsigned.tar.gz	`578d53467f1964e4...`	`4719ea1916451108...`
*-arm64-apple-darwin-unsigned.zip	`f7324f4fbf04094a...`	`9c733079ec211594...`
*-powerpc64-linux-gnu-debug.tar.gz	`4d1e9e946f74f4de...`	`4c96544b8853269a...`
*-powerpc64-linux-gnu.tar.gz	`95073776ba9e496f...`	`20d63349f5dd76f1...`
*-riscv64-linux-gnu-debug.tar.gz	`b9aa3f6b161a9a22...`	`0ffe8d08b0910dd9...`
*-riscv64-linux-gnu.tar.gz	`4359303e00e93dd6...`	`cbb67519840ea3eb...`
*-x86_64-apple-darwin-codesigning.tar.gz	`796e72140cca465e...`	`ae967d56916991b6...`
*-x86_64-apple-darwin-unsigned.tar.gz	`cd0a846310165114...`	`29831386b8bba237...`
*-x86_64-apple-darwin-unsigned.zip	`3f81d9630b346346...`	`0a7a72f25c12f5a4...`
*-x86_64-linux-gnu-debug.tar.gz	`b9600b433f60e512...`	`73ac0808ca92b7fe...`
*-x86_64-linux-gnu.tar.gz	`051c8ae1c97f699f...`	`a80382fcd4330a85...`
*.tar.gz	`a4f9396cd4278eca...`	`bc990272f05a6cc4...`
SHA256SUMS.part	`e34ac75fc6f95298...`	`56c8d00bb5600f7d...`
guix_build.log	`29922df57c21719c...`	`50d73edb3996ffee...`
guix_build.log.diff		`511d4dc5d4512587...`

… right one Original-Github-Pull: bitcoin#7522

hebasto · 2025-10-13T16:44:33Z

Also seems like the obvious time to document BITCOIN_GENBUILD_NO_GIT (#31999)

This is still unaddressed.

fanquake · 2026-02-17T10:24:24Z

This is still unaddressed.

@luke-jr are you going to add documentation here?

hebasto · 2026-02-19T13:08:49Z

NACK, for the following reasons:

Removing an undocumented environment variable is strictly better than the approach taken in this PR.
The PR appears to be abandoned by the author, as comments have remained unaddressed for an extended period.

fanquake · 2026-02-27T15:46:35Z

Closing for now. Testing #32220, it seems to accomplish what is needed, and resolves #31999.

whitslack reviewed Apr 4, 2025

View reviewed changes

fanquake added this to the 29.0 milestone Apr 4, 2025

hebasto reviewed Apr 4, 2025

View reviewed changes

laanwj added the Build system label Apr 7, 2025

glozow removed this from the 29.0 milestone Apr 7, 2025

DrahtBot mentioned this pull request Apr 8, 2025

cmake: Get rid of undocumented BITCOIN_GENBUILD_NO_GIT environment variable #32220

Merged

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

DrahtBot removed the DrahtBot Guix build requested label Apr 18, 2025

DrahtBot added CI failed and removed CI failed labels Apr 29, 2025

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

3eb619f

… right one Original-Github-Pull: bitcoin#7522

luke-jr force-pushed the fix_gitdir_foreign branch from 903fc97 to 3eb619f Compare May 13, 2025 22:53

fanquake closed this Feb 27, 2026

	set(IS_SOURCE_TARBALL FALSE)
	# git will put "set(IS_SOURCE_TARBALL TRUE)" on the next line inside archives. $Format:%nset(IS_SOURCE_TARBALL TRUE)$
	if(NOT IS_SOURCE_TARBALL AND PROJECT_IS_TOP_LEVEL)
	find_package(Git QUIET)
	endif()

Conversation

luke-jr commented Apr 3, 2025

Uh oh!

DrahtBot commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

fanquake commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 7, 2025

Uh oh!

luke-jr commented Apr 10, 2025

Uh oh!

hebasto commented Apr 10, 2025

Uh oh!

DrahtBot commented Apr 18, 2025

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

Uh oh!

hebasto commented Oct 13, 2025

Uh oh!

fanquake commented Feb 17, 2026

Uh oh!

hebasto commented Feb 19, 2026

Uh oh!

fanquake commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Apr 3, 2025 •

edited

Loading

hebasto Apr 7, 2025 •

edited

Loading

sipa Apr 8, 2025 •

edited

Loading

hebasto commented Apr 7, 2025 •

edited

Loading