Build-std: Add builtin dependencies - #16675
Conversation
|
We aren't expecting that this will land prior to those RFCs being approved (unless Cargo want it to), the intent with this is just to get some feedback on the general direction for the implementation, we're happy to adjust as much or as little as required :) @adamgemmell will also be away for a couple of weeks starting next week, so we might not respond to feedback immediately. |
| /// A directory-based registry. | ||
| Directory, | ||
| /// Package sources distributed with the rust toolchain | ||
| Builtin, |
There was a problem hiding this comment.
https://github.com/rust-lang/cargo/blob/master/crates/cargo-util-schemas/src/core/package_id_spec.rs is at least one other place that would need updating
There was a problem hiding this comment.
This will impact the unique identifier for the packages from this source in cargo's json output when compiling, cargo metadata, cargo <cmd> -p, etc
There was a problem hiding this comment.
I'll modify there too, and add a note to check the stdout in various use cases. The RFCs often make notes on what the output of various commands will be. Note that builtin doesn't actually appear in Units - they're all Path dependencies by that point.
An interesting point on cargo metadata is that we decided that we have an unresolved question regarding if deps of builtins should be shown on output, which will be a little hard here as they're not attached until unit generation.
There was a problem hiding this comment.
I've opted to implement pkg spec input/output in a later PR. I've added tests for common output commands like metadata/tree. json output (from build --message-format=json) isn't impacted in this PR - builtins do not exist in the unit graph (see this test which isn't changed in the rest of the branch c1f8964). I plan to address that in a future PR but I'm not sure on the best way to do that at this stage.
| .url | ||
| .to_file_path() | ||
| .expect("builtin sources should not be remote"); | ||
| Ok(Box::new(PathSource::new(&path, self, gctx))) |
There was a problem hiding this comment.
Will using a PathSsource directly like this work?
There was a problem hiding this comment.
This particularly could have interesting design questions
There was a problem hiding this comment.
Moved this to a builtin source
There was a problem hiding this comment.
A builtin source now wraps a RecursivePathSource, and there's only one source for all builtin packages. see 4832d04
| None | ||
| } else { | ||
| Some(builtins.iter()) | ||
| }; |
There was a problem hiding this comment.
I assume this is for implicit builtins. Is there a reason you chose to do this here?
There was a problem hiding this comment.
Source replacements happen in dep_cache (see query) so it seemed simpler just to put this here for now. After digging a little more, it might just be the deprecated [replace] section though, in which case I can probably lift this out of the cache to a more appropriate place
There was a problem hiding this comment.
This is in build_deps() now (see 1588221), which is a bit more appropriate. It could be moved up to the resolver activate function, but that's already massive, and moving it to PackageRegistry::query would involve modifying Summarys which are intended to be immutable.
| } | ||
| } | ||
|
|
||
| pub fn new_injected_builtin(name: InternedString) -> Dependency { |
There was a problem hiding this comment.
what do you see as the role of this compared to the other news?
There was a problem hiding this comment.
The difference is that opaque is true (making a SourceId was also more complicated in a previous iteration of this patch). public should also be true now that I look closer.
There was a problem hiding this comment.
| ) | ||
| })?; | ||
|
|
||
| if dep.is_opaque() { |
There was a problem hiding this comment.
I'd like to find a way to ask the source for the opaque variant of the summary. The tricky thing will be that we need to work with both variants.
There was a problem hiding this comment.
This pattern works well, see the new Summary::to_opaque_builtin_summary()
There was a problem hiding this comment.
Reworked again to make this more like the other constructors: 4832d04
| // Injecting builtins earlier (somewhere with access to RustcTargetData) is needed instead of this | ||
| let home = std::env::var("HOME").expect("HOME is set"); | ||
| let path = format!( | ||
| "file://{home}/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/" |
There was a problem hiding this comment.
The URL in source ids is public facing. I think we'll need something more generic and then a new Source
There was a problem hiding this comment.
I definitely tried something more generic but found that it had to be valid at various points. Will experiment more with this.
There was a problem hiding this comment.
I have hidden the URL in pkgids as per https://rust-lang.github.io/rfcs/3875-build-std-explicit-dependencies.html#cargo-subcommands:~:text=builtin%3A%2F%2F%2E%23core, but it may still leak out in various places. I plan to address this in future PRs that test individual subcommands' behaviour.
| has_dev_units, | ||
| crate::core::resolver::features::ForceAllTargets::No, | ||
| dry_run, | ||
| true, |
There was a problem hiding this comment.
not really a fan of bool constants being used in parameter lists. Makes it a lot harder to figure what what is going on here
There was a problem hiding this comment.
I've moved this specific bool, but there's probably scope to improve the passing around of the builtins path and dependencies set
There was a problem hiding this comment.
I added this information to ResolveOpts, see 707ac6b
| keep_previous: Option<Keep<'_>>, | ||
| specs: &[PackageIdSpec], | ||
| register_patches: bool, | ||
| inject_builtins: bool, |
There was a problem hiding this comment.
Why do we need to tunnel this through? Not thrilled with this
There was a problem hiding this comment.
This bool or the required logic behind it could probably be packaged into the workspace or it's gctx
There was a problem hiding this comment.
I added this information to ResolveOpts, see 707ac6b
This comment has been minimized.
This comment has been minimized.
16dc1ab to
ab7d8b0
Compare
| None | ||
| } else { | ||
| Some(builtins.iter()) | ||
| }; |
There was a problem hiding this comment.
Source replacements happen in dep_cache (see query) so it seemed simpler just to put this here for now. After digging a little more, it might just be the deprecated [replace] section though, in which case I can probably lift this out of the cache to a more appropriate place
| } | ||
| } | ||
|
|
||
| pub fn new_injected_builtin(name: InternedString) -> Dependency { |
There was a problem hiding this comment.
The difference is that opaque is true (making a SourceId was also more complicated in a previous iteration of this patch). public should also be true now that I look closer.
| // Injecting builtins earlier (somewhere with access to RustcTargetData) is needed instead of this | ||
| let home = std::env::var("HOME").expect("HOME is set"); | ||
| let path = format!( | ||
| "file://{home}/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/" |
There was a problem hiding this comment.
I definitely tried something more generic but found that it had to be valid at various points. Will experiment more with this.
| keep_previous: Option<Keep<'_>>, | ||
| specs: &[PackageIdSpec], | ||
| register_patches: bool, | ||
| inject_builtins: bool, |
There was a problem hiding this comment.
This bool or the required logic behind it could probably be packaged into the workspace or it's gctx
This comment has been minimized.
This comment has been minimized.
ab7d8b0 to
8607d8c
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. |
9766054 to
ee827d1
Compare
This commit is currently an unused API and will be tested in later commits
This commit is currently an unused API and will be tested in later commits
generation for them The old unit generation used to insert dependencies to every package on the set of roots std was resolved with. The new unit generation replaces dependencies with the relevant root of std, or ignores them if the dependency is not needed. This commit enables tests for the previous set of commits.
This test's assertion was completely broken because of the leading spaces and use of `...` which doesn't work with stderr_does_not_contain. See the comments in this test for additional details of shared dependencies on why the test cannot be fixed right now. It is feasibly possible for Cargo to share the user dep_test dependency and the build-std dep_test, but this is complex as written as dep_test gains an implicit std dependency while the mock-std depends on it. It may be easier to share them with explicit builtin dependencies, but the situation is complex due to -Zforce-unstable-if-unmarked and prone to breakage due to RUSTC_BOOTSTRAP=1.
The ideal behaviour is probably to emit all packages, but currently only the roots of std are emitted. A future PR will change this.
ee827d1 to
0b54da3
Compare
|
@rustbot ready |
View all comments
What does this PR try to resolve?
This PR internally introduces:
It moves the -Zbuild-std implementation to use them as part of rust-lang/rfcs#3875. While not strictly required for build-std=always which is our immediate goal, I wanted to ensure the implementation moving forward had explicit dependencies in mind.
The main behaviour change in this PR is that host dependencies like build-scripts and proc_macros no longer use build-std. If this is desired we can re-introduce the behaviour in an unstable manner, but it never worked for -Zbuild-std in cross-compile mode.
This PR implements this as a whole by:
std_crates, and injects them to every eligible crate during the resolve.This PR has a very large surface area and will inevitably change some behaviour of -Zbuild-std. In particular we need to be careful about where we expose builtin SourceIds and package specs. The intent here is to avoid disruption as build-std is a very commonly used unstable feature, but some is to be expected and will be fixed in future PRs according to the agreed RFC. The non-build-std route is well tested already and I do not expect to find regressions there as the majority of this code is gated on -Zbuild-std.
How to test and review this PR?
The PR is best read commit-by-commit - all commits are atomic and try to introduce features piecemeal where possible, though somewhat inevitably there's a big "waterfall" enabling commit in the middle. A commit 5bfab35 early in the branch adds a test showing that the unit-graph does not change compared to the old
-Zbuild-stdbehaviour.The behaviour can be manually tested locally by providing
-Zbuild-stdas a flag to any cargo command.