-
Notifications
You must be signed in to change notification settings - Fork 2.9k
WIP: Builtin/opaque dependencies #16675
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,10 @@ struct Inner { | |
| // This dependency should be used only for this platform. | ||
| // `None` means *all platforms*. | ||
| platform: Option<Platform>, | ||
|
|
||
| // Opaque dependencies should be resolved with a separate resolver run, and handled | ||
| // by unit generation. | ||
| opaque: bool, | ||
| } | ||
|
|
||
| #[derive(Serialize)] | ||
|
|
@@ -162,6 +166,30 @@ impl Dependency { | |
| platform: None, | ||
| explicit_name_in_toml: None, | ||
| artifact: None, | ||
| opaque: false, | ||
| }), | ||
| } | ||
| } | ||
|
|
||
| pub fn new_injected_builtin(name: InternedString) -> Dependency { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you see as the role of this compared to the other |
||
| assert!(!name.is_empty()); | ||
| Dependency { | ||
| inner: Arc::new(Inner { | ||
| name, | ||
| source_id: SourceId::new_builtin(&name).expect("package name is valid url"), | ||
| registry_id: None, | ||
| req: OptVersionReq::Any, | ||
| kind: DepKind::Normal, | ||
| only_match_name: true, | ||
| optional: false, | ||
| public: false, | ||
| features: Vec::new(), | ||
| default_features: true, | ||
| specified_req: false, | ||
| platform: None, | ||
| explicit_name_in_toml: None, | ||
| artifact: None, | ||
| opaque: true, | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -455,6 +483,10 @@ impl Dependency { | |
| pub(crate) fn maybe_lib(&self) -> bool { | ||
| self.artifact().map(|a| a.is_lib).unwrap_or(true) | ||
| } | ||
|
|
||
| pub fn is_opaque(&self) -> bool { | ||
| self.inner.opaque | ||
| } | ||
| } | ||
|
|
||
| /// The presence of an artifact turns an ordinary dependency into an Artifact dependency. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| //! The former is just one kind of source, | ||
| //! while the latter involves operations on the registry Web API. | ||
|
|
||
| use std::collections::{HashMap, HashSet}; | ||
| use std::collections::{BTreeMap, HashMap, HashSet}; | ||
| use std::task::{Poll, ready}; | ||
|
|
||
| use crate::core::{Dependency, PackageId, PackageSet, Patch, SourceId, Summary}; | ||
|
|
@@ -24,6 +24,7 @@ use crate::util::{CanonicalUrl, GlobalContext}; | |
| use annotate_snippets::Level; | ||
| use anyhow::Context as _; | ||
| use itertools::Itertools; | ||
| use semver::Version; | ||
| use tracing::{debug, trace}; | ||
| use url::Url; | ||
|
|
||
|
|
@@ -724,6 +725,36 @@ impl<'gctx> Registry for PackageRegistry<'gctx> { | |
| ) | ||
| })?; | ||
|
|
||
| if dep.is_opaque() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // Currently, all opaque dependencies are builtins. | ||
| // Create a dummy Summary that can be replaced with a real package during | ||
| // unit generation | ||
| trace!( | ||
| "Injecting package to satisfy builtin dependency on {}", | ||
| dep.package_name() | ||
| ); | ||
| let ver = if dep.package_name() == "compiler_builtins" { | ||
| //TODO: hack | ||
| Version::new(0, 1, 160) | ||
| } else { | ||
| Version::new(0, 0, 0) | ||
| }; | ||
| let pkg_id = PackageId::new( | ||
| dep.package_name(), | ||
| ver, | ||
| SourceId::new_builtin(&dep.package_name()).expect("SourceId ok"), | ||
| ); | ||
|
|
||
| let summary = Summary::new( | ||
| pkg_id, | ||
| vec![], | ||
| &BTreeMap::new(), // TODO: bodge | ||
| Option::<String>::None, | ||
| None, | ||
| )?; | ||
| f(IndexSummary::Candidate(summary)); | ||
| return Poll::Ready(Ok(())); | ||
| } | ||
| let source = self.sources.get_mut(dep.source_id()); | ||
| match (override_summary, source) { | ||
| (Some(_), None) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,21 +48,42 @@ pub struct RegistryQueryer<'a> { | |
| >, | ||
| /// all the cases we ended up using a supplied replacement | ||
| used_replacements: HashMap<PackageId, Summary>, | ||
| /// Cached builtin dependencies that should be injected. Empty implies that builtins shouldn't | ||
| /// be injected | ||
| builtins: Vec<Dependency>, | ||
| } | ||
|
|
||
| impl<'a> RegistryQueryer<'a> { | ||
| pub fn new( | ||
| registry: &'a mut dyn Registry, | ||
| replacements: &'a [(PackageIdSpec, Dependency)], | ||
| version_prefs: &'a VersionPreferences, | ||
| inject_builtins: bool, | ||
| ) -> Self { | ||
| let builtins = if inject_builtins { | ||
| [ | ||
| "std", | ||
| "alloc", | ||
| "core", | ||
| "panic_unwind", | ||
| "proc_macro", | ||
| "compiler_builtins", | ||
| ] | ||
| .iter() | ||
| .map(|&krate| Dependency::new_injected_builtin(krate.into())) | ||
| .collect() | ||
| } else { | ||
| vec![] | ||
| }; | ||
|
|
||
| RegistryQueryer { | ||
| registry, | ||
| replacements, | ||
| version_prefs, | ||
| registry_cache: HashMap::new(), | ||
| summary_cache: HashMap::new(), | ||
| used_replacements: HashMap::new(), | ||
| builtins, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -238,10 +259,11 @@ impl<'a> RegistryQueryer<'a> { | |
| { | ||
| return Ok(out.0.clone()); | ||
| } | ||
|
|
||
| // First, figure out our set of dependencies based on the requested set | ||
| // of features. This also calculates what features we're going to enable | ||
| // for our own dependencies. | ||
| let (used_features, deps) = resolve_features(parent, candidate, opts)?; | ||
| let (used_features, deps) = resolve_features(parent, candidate, opts, &self.builtins)?; | ||
|
|
||
| // Next, transform all dependencies into a list of possible candidates | ||
| // which can satisfy that dependency. | ||
|
|
@@ -291,18 +313,28 @@ pub fn resolve_features<'b>( | |
| parent: Option<PackageId>, | ||
| s: &'b Summary, | ||
| opts: &'b ResolveOpts, | ||
| builtins: &[Dependency], | ||
| ) -> ActivateResult<(HashSet<InternedString>, Vec<(Dependency, FeaturesSet)>)> { | ||
| // First, filter by dev-dependencies. | ||
| let deps = s.dependencies(); | ||
| let deps = deps.iter().filter(|d| d.is_transitive() || opts.dev_deps); | ||
|
|
||
| let deps = deps | ||
| .into_iter() | ||
| .filter(|d| d.is_transitive() || opts.dev_deps); | ||
| let builtin_deps = if s.source_id().is_builtin() { | ||
| // Don't add builtin deps to dummy builtin packages | ||
| None | ||
| } else { | ||
| Some(builtins.iter()) | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this is for implicit builtins. Is there a reason you chose to do this here? |
||
|
|
||
| let reqs = build_requirements(parent, s, opts)?; | ||
| let mut ret = Vec::new(); | ||
| let default_dep = BTreeSet::new(); | ||
| let mut valid_dep_names = HashSet::new(); | ||
|
|
||
| // Next, collect all actually enabled dependencies and their features. | ||
| for dep in deps { | ||
| for dep in deps.chain(builtin_deps.into_iter().flatten()) { | ||
| // Skip optional dependencies, but not those enabled through a | ||
| // feature | ||
| if dep.is_optional() && !reqs.deps.contains_key(&dep.name_in_toml()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,6 +140,21 @@ impl SourceId { | |
| } | ||
| } | ||
|
|
||
| pub fn new_builtin(name: &str) -> CargoResult<SourceId> { | ||
| // 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/" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the special case? |
||
| )) | ||
| } else { | ||
| Self::from_url(&format!("builtin+{path}{name}")) | ||
| } | ||
| } | ||
|
|
||
| /// Parses a source URL and returns the corresponding ID. | ||
| /// | ||
| /// ## Example | ||
|
|
@@ -176,6 +191,10 @@ impl SourceId { | |
| let url = url.into_url()?; | ||
| SourceId::new(SourceKind::Path, url, None) | ||
| } | ||
| "builtin" => { | ||
| let url = url.into_url()?; | ||
| SourceId::new(SourceKind::Builtin, url, None) | ||
| } | ||
| kind => Err(anyhow::format_err!("unsupported source protocol: {}", kind)), | ||
| } | ||
| } | ||
|
|
@@ -387,6 +406,10 @@ impl SourceId { | |
| matches!(self.inner.kind, SourceKind::Git(_)) | ||
| } | ||
|
|
||
| pub fn is_builtin(self) -> bool { | ||
| matches!(self.inner.kind, SourceKind::Builtin) | ||
| } | ||
|
|
||
| /// Creates an implementation of `Source` corresponding to this ID. | ||
| /// | ||
| /// * `yanked_whitelist` --- Packages allowed to be used, even if they are yanked. | ||
|
|
@@ -433,6 +456,14 @@ impl SourceId { | |
| .expect("path sources cannot be remote"); | ||
| Ok(Box::new(DirectorySource::new(&path, self, gctx))) | ||
| } | ||
| SourceKind::Builtin => { | ||
| let path = self | ||
| .inner | ||
| .url | ||
| .to_file_path() | ||
| .expect("builtin sources should not be remote"); | ||
| Ok(Box::new(PathSource::new(&path, self, gctx))) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will using a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This particularly could have interesting design questions |
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -679,6 +710,7 @@ impl fmt::Display for SourceId { | |
| } | ||
| SourceKind::LocalRegistry => write!(f, "registry `{}`", url_display(&self.inner.url)), | ||
| SourceKind::Directory => write!(f, "dir {}", url_display(&self.inner.url)), | ||
| SourceKind::Builtin => write!(f, "builtin {}", url_display(&self.inner.url)), | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will impact the unique identifier for the packages from this source in cargo's json output when compiling,
cargo metadata,cargo <cmd> -p, etcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
builtindoesn't actually appear in Units - they're all Path dependencies by that point.An interesting point on
cargo metadatais 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.