feat(build-std): Add implicit builtin dependencies - #17397
Conversation
rustbot
added
A-crate-dependencies
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
6481a69 to
e7c0f2c
Compare
This comment has been minimized.
This comment has been minimized.
|
This push fixed conflicts caused by #17401 by constructing a RustcTargetData earlier and then discarding it. This isn't optimal and we have an item in our work plan to reduce rustc invocations once the implementation has settled a bit. As part of that I want to improve |
e7c0f2c to
b69af00
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. |
|
I've reworked this by dropping this commit which had changes to ResolverOpts. This revision moves the injected deps to be essentially a single flag for the entire resolve which simplifies things. This does means that the resolve contains slightly more unneeded packages, such as builtin deps on packages only reachable via I suspect it was broken as the resolver caches activations, and if the first encounter with a package ends up not injecting builtins then the resolver won't revisit it to inject them later if needed. I think this would be fixable fairly easily, but ultimately given we already need to drop some packages from the resolve during Unit generation when the target does't default to std (see https://github.com/rust-lang/cargo/pull/16675/changes#diff-8011e2789b524940bae5b965595c4bd762e81f29f35ded30f1afd12e38d6d233R146) this mechanism didn't achieve much. |
|
|
||
| if !candidate.source_id().is_builtin() { | ||
| for dep in self.implicit_builtin_deps { | ||
| deps.push((dep.clone(), Rc::new(BTreeSet::default()))); |
There was a problem hiding this comment.
Existing comment from @epage regarding where exactly we should be injecting builtin deps: #16675 (comment)
Since the original comment this has moved to build_deps, which is the original point where the resolver discovers dependencies. Moving this lower means modifying data structures which are intended to be immutable, and moving it higher means adding to already huge functions and moves it outside the cache.
| } | ||
| } | ||
|
|
||
| pub fn new_implicit_builtin(name: InternedString, path: &Path) -> CargoResult<Dependency> { |
There was a problem hiding this comment.
Existing comment from @epage regarding the role of the constructor: #16675 (comment)
Since then the constructor has changed a little to be more like the other constructors. I feel the use case is clearly distinct.
| &version_prefs, | ||
| ResolveVersion::with_rust_version(ws.lowest_rust_version()), | ||
| ws.gctx(), | ||
| implicit_builtin_deps, |
There was a problem hiding this comment.
Existing concern from @epage regarding this being tunnelled through to the resolver #16675 (comment)
Since then this no longer needs to be tunnelled from outside of ops/resolve.rs from other modules in opts as whether to inject builtins can be determined from within this function.
| version_prefs: &VersionPreferences, | ||
| resolve_version: ResolveVersion, | ||
| gctx: &GlobalContext, | ||
| implicit_builtin_deps: &[Dependency], |
There was a problem hiding this comment.
Existing concern from @epage regarding usage of bools in parameter lists #16675 (comment)
Since then this has been changed to a more descriptive slice of Dependencys.
b69af00 to
28f2f00
Compare
3 participants
What does this PR try to resolve?
This PR, part of a stack split off from #16675, allows Cargo to inject implicit builtin dependencies as mentioned in the explicit builtin dependencies. The code, though tested, is currently dead pending later a later PR which will gate them on
-Zbuild-std, and then eventually enabled by default regardless of the setting ofbuild-std.This PR has diverged slightly from the RFC, which writes that the set of builtin dependencies will be "that target’s default set of standard library crates". However, resolves are target-independent, so this PR defaults to introducing a dependency on the full set of std crates if no list is provided. A later PR which implements unit generation will discard any unnecessary builtin dependencies.
The scope of this PR is to not introduce any unexpected changes compared with the existing -Zbuild-std implementation.
How to test and review this PR?
The PR introduces unit tests for all new behaviour and I recommend reading the commits in order. This PR cannot be manually tested.
There is a very large amount of context behind build-std. Please feel free to spam me with questions and I can link to relevant bits of this context than reviewers needing to reread this context every time you need to review a build-std PR if you prefer.
This PR is part of a stack: