From 647afcf28e43aa7ea848909ee923e6c39f01413c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 17 Apr 2024 12:00:08 -0400 Subject: [PATCH] Add a proxy layer for extras --- crates/uv-resolver/src/error.rs | 7 +- crates/uv-resolver/src/pubgrub/package.rs | 9 ++- crates/uv-resolver/src/pubgrub/priority.rs | 6 +- crates/uv-resolver/src/resolver/mod.rs | 78 ++++++++++------------ crates/uv/tests/pip_compile.rs | 38 ++++------- 5 files changed, 67 insertions(+), 71 deletions(-) diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index 8391ad6aae26..d40d93986537 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -208,7 +208,8 @@ impl NoSolutionError { BTreeSet::from([python_requirement.target().deref().clone()]), ); } - PubGrubPackage::Package(name, ..) => { + PubGrubPackage::Extra(_, _, _) => {} + PubGrubPackage::Package(name, _, _) => { // Avoid including available versions for packages that exist in the derivation // tree, but were never visited during resolution. We _may_ have metadata for // these packages, but it's non-deterministic, and omitting them ensures that @@ -256,7 +257,7 @@ impl NoSolutionError { ) -> Self { let mut new = FxHashMap::default(); for package in self.derivation_tree.packages() { - if let PubGrubPackage::Package(name, ..) = package { + if let PubGrubPackage::Package(name, _, _) = package { if let Some(entry) = unavailable_packages.get(name) { let reason = entry.value(); new.insert(name.clone(), reason.clone()); @@ -275,7 +276,7 @@ impl NoSolutionError { ) -> Self { let mut new = FxHashMap::default(); for package in self.derivation_tree.packages() { - if let PubGrubPackage::Package(name, ..) = package { + if let PubGrubPackage::Package(name, _, _) = package { if let Some(entry) = incomplete_packages.get(name) { let versions = entry.value(); for entry in versions { diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs index 922b2b54bcb5..47f93303aed2 100644 --- a/crates/uv-resolver/src/pubgrub/package.rs +++ b/crates/uv-resolver/src/pubgrub/package.rs @@ -61,13 +61,19 @@ pub enum PubGrubPackage { /// _after_ a registry variant. Option, ), + /// A package with an extra. + Extra(PackageName, ExtraName, Option), } impl PubGrubPackage { /// Create a [`PubGrubPackage`] from a package name and optional extra name. pub(crate) fn from_package(name: PackageName, extra: Option, urls: &Urls) -> Self { let url = urls.get(&name).cloned(); - Self::Package(name, extra, url) + if let Some(extra) = extra { + Self::Extra(name, extra, url) + } else { + Self::Package(name, extra, url) + } } } @@ -94,6 +100,7 @@ impl std::fmt::Display for PubGrubPackage { Self::Package(name, Some(extra), ..) => { write!(f, "{name}[{extra}]") } + Self::Extra(name, extra, ..) => write!(f, "{name}[{extra}]"), } } } diff --git a/crates/uv-resolver/src/pubgrub/priority.rs b/crates/uv-resolver/src/pubgrub/priority.rs index bb6b1e329e7e..3f4753bd79e1 100644 --- a/crates/uv-resolver/src/pubgrub/priority.rs +++ b/crates/uv-resolver/src/pubgrub/priority.rs @@ -27,7 +27,8 @@ impl PubGrubPriorities { match package { PubGrubPackage::Root(_) => {} PubGrubPackage::Python(_) => {} - PubGrubPackage::Package(name, _, None) => { + + PubGrubPackage::Extra(name, _, None) | PubGrubPackage::Package(name, _, None) => { match self.0.entry(name.clone()) { std::collections::hash_map::Entry::Occupied(mut entry) => { // Preserve the original index. @@ -65,7 +66,7 @@ impl PubGrubPriorities { } } } - PubGrubPackage::Package(name, _, Some(_)) => { + PubGrubPackage::Extra(name, _, Some(_)) | PubGrubPackage::Package(name, _, Some(_)) => { match self.0.entry(name.clone()) { std::collections::hash_map::Entry::Occupied(mut entry) => { // Preserve the original index. @@ -99,6 +100,7 @@ impl PubGrubPriorities { match package { PubGrubPackage::Root(_) => Some(PubGrubPriority::Root), PubGrubPackage::Python(_) => Some(PubGrubPriority::Root), + PubGrubPackage::Extra(name, _, _) => self.0.get(name).copied(), PubGrubPackage::Package(name, _, _) => self.0.get(name).copied(), } } diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index e7a998cf2f32..305248b1978e 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -525,6 +525,7 @@ impl< match package { PubGrubPackage::Root(_) => {} PubGrubPackage::Python(_) => {} + PubGrubPackage::Extra(_, _, _) => {} PubGrubPackage::Package(name, _extra, None) => { // Verify that the package is allowed under the hash-checking policy. if !self.hasher.allows_package(name) { @@ -561,7 +562,7 @@ impl< // Iterate over the potential packages, and fetch file metadata for any of them. These // represent our current best guesses for the versions that we _might_ select. for (package, range) in packages { - let PubGrubPackage::Package(package_name, _extra, None) = package else { + let PubGrubPackage::Package(package_name, None, None) = package else { continue; }; request_sink @@ -604,16 +605,9 @@ impl< } } - PubGrubPackage::Package(package_name, extra, Some(url)) => { - if let Some(extra) = extra { - debug!( - "Searching for a compatible version of {package_name}[{extra}] @ {url} ({range})", - ); - } else { - debug!( - "Searching for a compatible version of {package_name} @ {url} ({range})" - ); - } + PubGrubPackage::Extra(package_name, _, Some(url)) + | PubGrubPackage::Package(package_name, _, Some(url)) => { + debug!("Searching for a compatible version of {package} @ {url} ({range})"); // If the dist is an editable, return the version from the editable metadata. if let Some((_local, metadata)) = self.editables.get(package_name) { @@ -702,7 +696,8 @@ impl< Ok(Some(ResolverVersion::Available(version.clone()))) } - PubGrubPackage::Package(package_name, extra, None) => { + PubGrubPackage::Extra(package_name, _, None) + | PubGrubPackage::Package(package_name, _, None) => { // Wait for the metadata to be available. let versions_response = self .index @@ -732,13 +727,7 @@ impl< } }; - if let Some(extra) = extra { - debug!( - "Searching for a compatible version of {package_name}[{extra}] ({range})", - ); - } else { - debug!("Searching for a compatible version of {package_name} ({range})"); - } + debug!("Searching for a compatible version of {package} ({range})"); // Find a version. let Some(candidate) = self.selector.select( @@ -770,22 +759,13 @@ impl< } ResolvedDistRef::Installed(_) => Cow::Borrowed("installed"), }; - if let Some(extra) = extra { - debug!( - "Selecting: {}[{}]=={} ({})", - candidate.name(), - extra, - candidate.version(), - filename, - ); - } else { - debug!( - "Selecting: {}=={} ({})", - candidate.name(), - candidate.version(), - filename, - ); - } + + debug!( + "Selecting: {}=={} ({})", + package, + candidate.version(), + filename, + ); // We want to return a package pinned to a specific version; but we _also_ want to // store the exact file that we selected to satisfy that version. @@ -794,13 +774,16 @@ impl< let version = candidate.version().clone(); // Emit a request to fetch the metadata for this version. - if self.index.distributions.register(candidate.version_id()) { - let request = match dist.for_resolution() { - ResolvedDistRef::Installable(dist) => Request::Dist(dist.clone()), - ResolvedDistRef::Installed(dist) => Request::Installed(dist.clone()), - }; - request_sink.send(request).await?; + if matches!(package, PubGrubPackage::Package(_, _, _)) { + if self.index.distributions.register(candidate.version_id()) { + let request = match dist.for_resolution() { + ResolvedDistRef::Installable(dist) => Request::Dist(dist.clone()), + ResolvedDistRef::Installed(dist) => Request::Installed(dist.clone()), + }; + request_sink.send(request).await?; + } } + Ok(Some(ResolverVersion::Available(version))) } } @@ -1044,6 +1027,18 @@ impl< Ok(Dependencies::Available(constraints.into())) } + + // Add a dependency on both the extra and base package. + PubGrubPackage::Extra(package_name, extra, url) => Ok(Dependencies::Available(vec![ + ( + PubGrubPackage::Package(package_name.clone(), None, url.clone()), + Range::singleton(version.clone()), + ), + ( + PubGrubPackage::Package(package_name.clone(), Some(extra.clone()), url.clone()), + Range::singleton(version.clone()), + ), + ])), } } @@ -1251,6 +1246,7 @@ impl< match package { PubGrubPackage::Root(_) => {} PubGrubPackage::Python(_) => {} + PubGrubPackage::Extra(_, _, _) => {} PubGrubPackage::Package(package_name, _extra, Some(url)) => { reporter.on_progress(package_name, &VersionOrUrl::Url(url)); } diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 6f61bf85e6c8..c893d9fdb660 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -3904,8 +3904,6 @@ fn find_links_requirements_txt() -> Result<()> { /// `extras==0.0.2` fails to build (i.e., it always throws). Since `extras==0.0.1` is pinned, we /// should never even attempt to build `extras==0.0.2`, despite an unpinned `extras[dev]` /// requirement. -/// -/// This resolution should succeed, but currently fails. #[test] fn avoid_irrelevant_extras() -> Result<()> { let context = TestContext::new("3.12"); @@ -3919,31 +3917,23 @@ fn avoid_irrelevant_extras() -> Result<()> { .arg("requirements.in") .arg("--find-links") .arg(context.workspace_root.join("scripts").join("links")), @r###" - success: false - exit_code: 2 + success: true + exit_code: 0 ----- stdout ----- + # This file was autogenerated by uv via the following command: + # uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z requirements.in + anyio==4.3.0 + # via extras + extras==0.0.1 + idna==3.6 + # via anyio + iniconfig==2.0.0 + # via extras + sniffio==1.3.1 + # via anyio ----- stderr ----- - error: Failed to download and build: extras==0.0.2 - Caused by: Failed to build: extras==0.0.2 - Caused by: Build backend failed to determine extra requires with `build_wheel()` with exit status: 1 - --- stdout: - - --- stderr: - Traceback (most recent call last): - File "", line 14, in - File "[CACHE_DIR]/[TMP]/build_meta.py", line 325, in get_requires_for_build_wheel - return self._get_build_requires(config_settings, requirements=['wheel']) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - File "[CACHE_DIR]/[TMP]/build_meta.py", line 295, in _get_build_requires - self.run_setup() - File "[CACHE_DIR]/[TMP]/build_meta.py", line 487, in run_setup - super().run_setup(setup_script=setup_script) - File "[CACHE_DIR]/[TMP]/build_meta.py", line 311, in run_setup - exec(code, locals()) - File "", line 3, in - ZeroDivisionError: division by zero - --- + Resolved 5 packages in [TIME] "###); Ok(())