-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix: generate std roots based on all collected targets #14480
fix: generate std roots based on all collected targets #14480
Conversation
fixes rust-lang#10444 by using the same `CompileKind`s that artifact dependencies produces
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
@@ -948,6 +948,7 @@ impl<'gctx> RustcTargetData<'gctx> { | |||
})); | |||
for kind in all_kinds { | |||
res.merge_compile_kind(kind)?; | |||
res.requested_kinds.push(kind); |
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 only collects target triples from workspace members. It doesn't walk through the entire dependency graph. I believe if there is a transitive artifact dependency requiring an extra target triple it will fail. We need a test case to verify it.
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 for putting effort on a fix. This seems incomplete and lacks tests. Could you follow the atomic commit pattern that the first commit contains tests and passes. The second shows the actual fix and test change.
https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request
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.
I think we are looking for something like ehuss suggesting: #10444 (comment), though I personally have no time working on a proper refactor 😞
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.
Yeah, this had originally fixed my issue but after some more testing it appears to also cause more issues in other places :/ It appears you are correct and a proper fix is somewhat more complex. Gonna close this for now
fixes #10444 by using the same
CompileKind
s that artifact dependencies produces