WIP: Builtin/opaque dependencies#16675
Conversation
rustbot
added
A-cli
|
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
| .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
| 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?
| } | ||
| } | ||
|
|
||
| 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?
| ) | ||
| })?; | ||
|
|
||
| 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.
| // 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
| ); | ||
| if name == "compiler_builtins" { | ||
| Self::from_url(&format!( | ||
| "builtin+{path}compiler-builtins/compiler-builtins" |
| 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
| 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
4 participants
This WIP PR intends to evaluate an approach to rust-lang/rfcs#3875 by adding support for
builtinandopaquedependencies and lets the current-Zbuild-stdimplementation use them. I'm working on this now as it's a fairly wide-reaching change and was worried that some necessary changes might invalidate other work on the build-std RFCs.A
builtindependency is one which Cargo innately knows how to find, such as one included with the toolchain. The RFC above adds a newbuiltinpackage source for standard library dependencies. Anopaquedependency isn't yet fully agreed upon, but is a possible future feature that "hides" transitive dependencies from the main resolve in order to reduce rebuilds.This PR does this by:
dep_cache(which every call to activate in the resolver goes through), which are identified as opaque.This PR mostly works end to end for various packages I've tried, though some (like tokio) fail right now, maybe because it's a workspace. Removing the hardcoded hacks will be needed to check what tests break.
-Zbuild-std-featuresstill works as I haven't touched the main std resolve and take Units from that.What would be great is some feedback on whether this approach seems sustainable or risks breaking too many things. Do you have any thoughts at this stage @ehuss @epage?