Lazy expand absolute path to avoid including them in the output #4009
+179
−49
Conversation
This file contains 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
|
@AlessandroPatti Tests are failing on macOS, presumably because the wrapper isn't fully transparent. |
fmeum
reviewed
Aug 7, 2024
AlessandroPatti
force-pushed
the
apatti/hermetic-cc
branch
from
f880949
to
22bdd67
Compare
fmeum
approved these changes
Aug 7, 2024
|
Thanks, this is a really clean and well-tested solution! |
fmeum
pushed a commit
that referenced
this pull request
Oct 25, 2024
**What type of PR is this?** > Bug fix **What does this PR do? Why is it needed?** PR #4009 added a mechanism that uses a sentinel value as a placeholder for paths which need to be expanded to absolutes for compile operations. These paths end up embedded in `.a` files and come back to the linker during link operations. The previous PR replaces the `CC` tool with a pointer to `builder cc` which in turn substitutes the placeholder value for actual paths. Unfortunately this wasn't done for the linker, so the linker is receiving parameters like `--sysroot=__GO_BAZEL_CC_PLACEHOLDER__external/_main~_repo_rules~ubuntu_20_04_sysroot/` which is not valid. When a sysroot is configured this means that internally the linker won't be able to find the sysroot in some cases. In almost comically bad luck, this *only* affects the `go tool link` calls to `linkerFlagSupported` because for some reason `linkerFlagSupported` puts the `-extldflags` at the *beginning* of the linker command, while the main host link command has them at the end. So this means the linker detects all flags as unsupported but then proceeds to run the linker "correctly" which results in really confusing linker errors. In my case this resulted in the linker trying to run with `-rdynamic` incorrectly. **Which issues(s) does this PR fix?** Fixes #3886 It could be reasonable to open a bug against `go tool link` describing the difference between the host link and the flag checks. I suspect it's not really intended behavior and was just sort of happenstance that things ended up that way. I also really have no idea how to write a test for this. I'm willing to give it a shot but would really appreciate a suggestion as to where/how.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
It fixes reproducibility issues reported in #3994 by lazy-expanding paths to absolute paths
Which issues(s) does this PR fix?
Fixes #3994