Build-std: Add builtin dependencies#16675
Build-std: Add builtin dependencies#16675adamgemmell wants to merge 9 commits intorust-lang:masterfrom
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.
| .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
| 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, 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.
| ) | ||
| })?; | ||
|
|
||
| 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()
| // 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.
| ); | ||
| if name == "compiler_builtins" { | ||
| Self::from_url(&format!( | ||
| "builtin+{path}compiler-builtins/compiler-builtins" |
There was a problem hiding this comment.
It's the only direct dependency that isn't directly in the library folder. We need a better way of discovering packages for sure.
| 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
| 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
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.
| ); | ||
| if name == "compiler_builtins" { | ||
| Self::from_url(&format!( | ||
| "builtin+{path}compiler-builtins/compiler-builtins" |
There was a problem hiding this comment.
It's the only direct dependency that isn't directly in the library folder. We need a better way of discovering packages for sure.
| 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
| .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.
Moved this to a builtin source
| ) | ||
| })?; | ||
|
|
||
| if dep.is_opaque() { |
There was a problem hiding this comment.
This pattern works well, see the new Summary::to_opaque_builtin_summary()
| None | ||
| } else { | ||
| Some(builtins.iter()) | ||
| }; |
There was a problem hiding this comment.
This is in build_deps() now, 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.
| has_dev_units, | ||
| crate::core::resolver::features::ForceAllTargets::No, | ||
| dry_run, | ||
| true, |
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
|
r? @epage rustbot has assigned @epage. Use Why was this reviewer chosen?The reviewer was selected based on:
|
8607d8c to
65e2e17
Compare
|
Something I've noticed is that we may need to resolve std multiple times - if one supports std, the requested std (or test) crate in the resolve will enable features (like the proposed |
View all comments
This introduces a builtin source, the concept of opaque dependencies and implicit builtin dependencies and moves the -Zbuild-std implementation to use them, as part of rust-lang/rfcs#3875. While not strictly required for build-std=always, resolver support for builtin dependencies is required for the explicit builtin dependencies RFC and I wanted to ensure the implementation moving forward had that RFC in mind.
This PR does this by:
This PR will inevitably break some behaviour of -Zbuild-std, and fixing that will come in future PRs (as will adding tests for it). A fair enough of that behaviour probably worked by accident. -Zbuild-std-features still works as I haven't touched the main std resolve and take Units directly from that. 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.
At the time of writing, I'd particularly like to check behaviour with multiple targets and no_std targets before merging.