depends: Use CC_FOR_BUILD for config.guess
#29963
Conversation
|
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. 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. |
deb57e3 to
2f66415
Compare
I agree. Implemented. Thanks! |
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
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? |
2f66415 to
707d65b
Compare
Rebased. |
|
My Guix build: |
There are testing environments, such as OSS-Fuzz, that rely on a project's build system to support environment variables. |
Co-authored-by: Michael Ford <fanquake@gmail.com>
707d65b to
38d13e6
Compare
|
Rebased to resolve the conflict with the merged #30584. |
|
My Guix build: |
|
I don't understand this change. It seems setting CC_FOR_BUILD to CC is exactly the opposite of what we want to do? @hebasto's original change makes more sense to me: That ignores what's set in CC when detecting the native platform, which seems like the correct behavior to me. What was your intention with this patch, @fanquake ? |
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.