fix(build-std): include artifact dependency targets#17074
Conversation
rustbot
added
A-cfg-expr
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| } | ||
|
|
||
| #[cargo_test(build_std_mock)] | ||
| fn artifact_dep_target_builds_std_for_unrequested_target() { |
There was a problem hiding this comment.
Please see https://epage.github.io/dev/pr-style/#c-test (note our contrib guide says this as policy but that goes into more detail)
|
@davidtwco or @adamgemmell would either of you be up for doing a first pass on this? Note the issue is in the triage state. If we are not ready yet for this, including design discussions, we can shift the focus to the issue by moving this to a draft or closing it. See also https://doc.crates.io/contrib/issues.html#issue-status-labels |
| .any(CompileKind::is_host); | ||
| let mut std_kinds = build_config.requested_kinds.clone(); | ||
| std_kinds.extend(target_data.target_kinds().filter(|kind| { | ||
| !(host_kind_requested && target_data.rustc.host == target_data.short_name(kind)) |
There was a problem hiding this comment.
There's probably a simpler way to express the logic in this block. Is there actually a case where a compilekind is in target_kinds() but not requested_kinds?
There was a problem hiding this comment.
That case is covered now by per_package_target_builds_std_for_manifest_target.
| let mut std_kinds = build_config.requested_kinds.clone(); | ||
| std_kinds.extend( | ||
| data.target_kinds() | ||
| .filter(|kind| !(host_kind_requested && data.rustc.host == data.short_name(kind))), |
There was a problem hiding this comment.
In addition to the above, this is quite complex and weird in general, and is now duplicated. Could you put it in a helper function with a comment explaining why the filter is necessary?
There was a problem hiding this comment.
Addressed by moving this into standard_lib::resolve_std_kinds with a comment explaining the host-target filter.
npmccallum
force-pushed
the
issue-10444-15365-bindeps-build-std
branch
from
e04e305 to
cebeb7b
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
Thanks for revisiting this for per-pkg-target, I think that should be all we need to support mixed-target builds for rust-lang/rust#155363. I'm happy with the functionality and code at a low-level, a cargo team member will need to check for higher-level concerns 🙂
| .target_host() | ||
| .with_stderr_data(str![[r#" | ||
| ... | ||
| [RUNNING] `[..]rustc --crate-name std [..]--target [ALT_TARGET][..]` |
There was a problem hiding this comment.
Could you add an extra check that the host std is also built?
| .iter() | ||
| .copied() | ||
| .chain(ws.members().flat_map(|p| { | ||
| let mut manifest_target_kinds = ws |
There was a problem hiding this comment.
It's a bit unfortunate that the real target of a package is only figured out during unit generation. As is, this leads to std being resolved with more targets than required if a package has both default and forced targets set.
Right now it would be feasible to do the main resolve, generate its units, then resolve std for the right targets and attach those units. But with #16675, unit generation happens in one step and so will require both resolves ahead of time.
I think that's fine for now as std doesn't actually get built more times than required. per-pkg-target is unstable (as is build-std), and I'd prefer if #16675 is merged before looking for improvements here
5 participants
Artifact dependencies can introduce target kinds that were not requested on the command line. When
-Zbuild-stdis active, generate std roots for those discovered targets as well so unit construction does not panic when attaching std dependencies.The regression covers a bindeps artifact built for an alternate target while the root package is checked for the host target.
Closes #10444.
Refs #15365.