-
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 all 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()) { | ||
|
|
||
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, etc