-
Notifications
You must be signed in to change notification settings - Fork 637
Remap the exec root out of build script rustflags #4202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
cpcwood
wants to merge
1
commit into
bazelbuild:main
Choose a base branch
from
cpcwood:fix/build-script-remap-path-prefix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+7
−2
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is mapping pwd to
.correct? Won't this be theCARGO_MANIFEST_DIRwhich is not going to be correct outside of this action? Shouldn't there be some other mapping that the build script determines based on theOUT_DIRpath location relative to the execroot for the action?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for taking a look.
From my understanding
--remap-path-prefixshould only be changing what rustc writes into its own output, so it doesn't participate in the token substitution that makes build script output portable between actions.${pwd}is resolved to the exec root on the way in, before the script runs, and the.env/.depenvfiles are built only fromcargo:-prefixed lines on the script's stdout, which this shouldn't touch.The thing being fixed is that
OUT_DIRis a TreeArtifact hashed into every downstream rustc cache key. rustix's feature probe writes a--emit=metadatafile in there and only reads the exit code, but rustc folds its working directory into the metadata hash, so the file's bytes differ between checkouts and re-key everything downstream. It forwardsCARGO_ENCODED_RUSTFLAGSto that rustc, which is how the remap reaches it.re: deriving the prefix from
OUT_DIR- I'm unsure that'd work,OUT_DIRand the script's cwd are siblings rather than nested:The path that actually leaks is the working directory, and an OUT_DIR-derived prefix diverges from it at that last segment. The exec root is their only common ancestor, and since
--remap-path-prefixmatches on prefix it covers both..is the same valuerustc.bzluses for non-build-script compiles.While looking at this I realised there's another alternative if you'd prefer to avoid touching rustflags.
remove_nondeterministic_out_dir_filesalready targets this class of issue, so addingrustix_test_can_compiletoout_dir_volatile_file_basenamesshould fix this specific crate. I went with the remap because it'll cover any build script that forwards rustflags to rustc rather than needing an entry per crate, but happy to do that version instead if you want.