depends: Do not consider CC environment variable for detecting system#29963
depends: Do not consider CC environment variable for detecting system#29963hebasto wants to merge 1 commit intobitcoin:masterfrom
CC environment variable for detecting system#29963Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29963. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. |
A workaround for a bug fixed in bitcoin#29963
|
Where/why is this an issue? |
A workaround for a bug fixed in bitcoin#29963
I faced this issue during my work on the CMake staging branch when the The log shows no cross-compiling, but it is cross-compiling to |
|
Surely this isn't the only place env vars like this would cause issues? Feels like a cat-and-mouse game. |
Can you link to the exact lines in the log that show "no cross-compiling". |
https://github.com/hebasto/bitcoin/actions/runs/8822538616/job/24220973382#step:14:245: Please note that that was a PR branch from the CMake migration project. That branch detects cross-compiling basing on |
Yea, as-is this doesn't seem like a great fix, and may just break other things? A better diff might be something like: diff --git a/depends/Makefile b/depends/Makefile
index 005d9696fb..091511758d 100644
--- a/depends/Makefile
+++ b/depends/Makefile
@@ -51,7 +51,7 @@ FALLBACK_DOWNLOAD_PATH ?= https://bitcoincore.org/depends-sources
C_STANDARD ?= c11
CXX_STANDARD ?= c++20
-BUILD = $(shell ./config.guess)
+BUILD = $(shell CC_FOR_BUILD=$CC ./config.guess)
HOST ?= $(BUILD)
PATCHES_PATH = $(BASEDIR)/patches
BASEDIR = $(CURDIR)which would at least be using the option that is meant to be used for this. |
hebasto
force-pushed
the
240425-guess-cc
branch
from
deb57e3 to
2f66415
Compare
I agree. Implemented. Thanks! |
hebasto
changed the title
CC environment variable for detecting systemCC_FOR_BUILD for config.guess
Arguably that's because this wasn't intended to be supported :) I suppose this fix is reasonable, though supporting env vars like this seems quite brittle. |
A workaround for a bug fixed in bitcoin#29963
A workaround for a bug fixed in bitcoin#29963
|
Is this still an issue given recent CMake changes? |
Yes. Tested with the master branch @ 80bdd4b. And this PR still fixes it. |
|
@hebasto can you rebase this? |
hebasto
force-pushed
the
240425-guess-cc
branch
from
2f66415 to
707d65b
Compare
Rebased. |
At this point I can't remember. |
hebasto
force-pushed
the
240425-guess-cc
branch
from
38d13e6 to
a8cdc58
Compare
Reverted to the initial variant. |
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
hebasto
changed the title
CC_FOR_BUILD for config.guess CC environment variable for detecting system
|
My Guix build: |
|
Current version of this PR is ok but I do think fanquake's suggested change #29963 (comment) passing along the requested compiler to I think his change had a typo because it suggests using Lines 126 to 130 in 13891a8 |
Otherwise, the build system fails to detect cross-compiling mode properly in some cases when `CC` is set.
hebasto
force-pushed
the
240425-guess-cc
branch
from
a8cdc58 to
b149a28
Compare
I agree that using |
|
My Guix build: |
d79249d ci: add chimera Linux LTO CI job (fanquake) Pull request description: Adds a CI config based on using [Chimera Linux](https://chimera-linux.org/). This might be interesting for any of the following: * Chimera is based on LLVM & musl libc - we test both of these in isolation, but not together. * No GNU components. I don't think we have an existing Linux CI job that doesn't have a gcc/stdlibc++ install. This exercises the depends logic for a fully LLVM/Clang/lld only build, including building the native tools (related to #33902). * We don't currently have a job with LTO enabled (here using CMakes `CMAKE_INTERPROCEDURAL_OPTIMIZATION`, which is `-flto=thin` for LLVM/Clang). I think this is worth having generally (we do use LTO in some other places, like oss-fuzz). If runtime is too much of an issue, then it could also be dropped. (Chimera itself is also compiled with LTO). QT in depends doesn't build (#32744), so is excluded for now. Chimera has pointed out at least a few quirks, i.e #34390, #34408 and #29963 (comment). ACKs for top commit: maflcko: lgtm ACK d79249d hebasto: ACK d79249d. Tree-SHA512: 1174a7462bf2e7433a2c27a6cf398e94b05db42bb414629c71cf9f9a297ca269e173ae1b7517b30510b494b4397f918eef706d3c75c4286767c5557aeb6db4c7
| CXX_STANDARD ?= c++20 | ||
|
|
||
| BUILD = $(shell ./config.guess) | ||
| BUILD = $(shell unset CC && ./config.guess) |
There was a problem hiding this comment.
Do we really need to use a utility for this? Can't we set CC=?
There was a problem hiding this comment.
While CC= might work (I haven't test it), it relies on specific behavior in the configure script's implementation, making it a brittle solution.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK b149a28. env --unset was replaced with unset && since last review. I still think it could be better to write CC=$(build_CC) here even if build_CC may not be defined at this point (#29963 (comment)) because it makes intent of the code more obvious, but current PR is already an improvement
7 participants
On the master branch @ 3c88eac, consider the following commands in the
dependssubdirectory:The printed variable values are expected.
However, switching the
CCvariable context from Makefile to the shell environment breaks expectations:This PR fixes this issue.
UPDATE 2026-01-20
On the master branch @ 7f5ebef: