depends: Do not consider CC environment variable for detecting system
#29963
+1
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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. |
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 25, 2024
A workaround for a bug fixed in bitcoin#29963
Member
|
Where/why is this an issue? |
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 25, 2024
A workaround for a bug fixed in bitcoin#29963
Member
Author
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 |
Member
|
Surely this isn't the only place env vars like this would cause issues? Feels like a cat-and-mouse game. |
Member
Can you link to the exact lines in the log that show "no cross-compiling". |
Member
Author
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 |
Member
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
Member
Author
I agree. Implemented. Thanks! |
CC environment variable for detecting systemCC_FOR_BUILD for config.guess
Member
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. |
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 30, 2024
A workaround for a bug fixed in bitcoin#29963
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
May 2, 2024
A workaround for a bug fixed in bitcoin#29963
Member
|
Is this still an issue given recent CMake changes? |
Member
Author
Yes. Tested with the master branch @ 80bdd4b. And this PR still fixes it. |
Member
|
@hebasto can you rebase this? |
2f66415 to
707d65b
Compare
Member
Author
Rebased. |
Contributor
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
Member
Author
|
My Guix build: |
Member
Author
There are testing environments, such as OSS-Fuzz, that rely on a project's build system to support environment variables. |
707d65b to
38d13e6
Compare
Member
Author
|
Rebased to resolve the conflict with the merged #30584. |
Member
Author
|
My Guix build: |
Member
|
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 ? |
Member
Author
Member
At this point I can't remember. |
Otherwise, the build system fails to detect cross-compiling mode properly in some cases when `CC` is set.
38d13e6 to
a8cdc58
Compare
Member
Author
Reverted to the initial variant. |
Contributor
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use] |
CC_FOR_BUILD for config.guess CC environment variable for detecting system
Member
Author
|
My Guix build: |
Contributor
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.