-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Pass rustflags to artifacts built with implicit targets when using target-applies-to-host #13900
Merged
bors
merged 3 commits into
rust-lang:master
from
gmorenz:target_applies_to_host_rustflags
Jul 4, 2024
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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 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 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 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 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 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 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 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 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 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 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 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
Oops, something went wrong.
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.
This looks correct.
Would you mind sharing why we need the entire infra change (
Arc
and move out rustflags from targetinfo), instead of just the change here?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.
This question is basically what "How it is implemented" tries to answer in the original comment on this PR. To take another stab at explaining it here:
The
CompileKind
for artifactUnit
starts out asCompileKind::Target(host_target)
(assuming we're not cross compiling). TheCompileKind
for hostUnit
starts out asCompileKind::Host
This change sets up
target_info
so that lookups forCompileKind::Target(host_target)
get the correct rustflags (as well as units withCompileKind::Host
, which need different rustflags). Which is great provided we lookup artifactUnit
's with their originalCompileKind
.rebuild_unit_graph_shared
changes artifactUnit
s withCompileKind::Target(host_target)
to haveCompileKind::Host
. It does this so that we can deduplicate identical otherwise identical units and only compile them once*.The current infrastructure attempts to look up
rustflags
viakind
afterrebuild_unit_graph_shared
. Thus even though we have correctly set up thetarget_info
forCompileKind::Target(host_target)
we're going to look up the flags inhost_info
, which are wrong. The infrastructure change is to push this lookup forwards to before we rebuild the unit with a differentkind
. It is also necessary because we deduplicateUnit
s based on their hash, so even if we could somehow lookup the correct rustflags later, there wouldn't necessarily be a unique set of them (in the case where host and target artifacts have different rustflags, and a shared dependency).* To draw your attention to the table at the top. This is why
no --target flag
,target_applies_to_host=false
has "Without rustflags, built with 5 invocations" rather than 6. A dependency shared between the host artifacts and the target artifacts only has to be built once because it is built the exact same way for both of them.However this question caused me to double check that my understanding was correct by... trying it. I believe it is, however it turns out my test is broken and will (incorrectly) pass (fail to compile) because passing an invalid rustflag will cause
TargetInfo::new(host_target)
to fail because that command calls rustc with rustflags to query file-names/sysroot/... even though we never build a target with those rustflags.I'll figure out how to write a version of that test without the false-pass on the partial change and push it later tonight.
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.
Thanks. I like the idea that
Unit
holds the exact rustflags it is going to invoke. It opens a new door that we might be able to use this information to create the acutal “build plans” for external systems.There's only one nit left and I'll merge this :)