Skip to content

Commit

Permalink
Add a proxy layer for extras
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Apr 17, 2024
1 parent 59e9f57 commit 647afcf
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 71 deletions.
7 changes: 4 additions & 3 deletions crates/uv-resolver/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand All @@ -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 {
Expand Down
9 changes: 8 additions & 1 deletion crates/uv-resolver/src/pubgrub/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,19 @@ pub enum PubGrubPackage {
/// _after_ a registry variant.
Option<VerbatimUrl>,
),
/// A package with an extra.
Extra(PackageName, ExtraName, Option<VerbatimUrl>),
}

impl PubGrubPackage {
/// Create a [`PubGrubPackage`] from a package name and optional extra name.
pub(crate) fn from_package(name: PackageName, extra: Option<ExtraName>, 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)
}
}
}

Expand All @@ -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}]"),
}
}
}
6 changes: 4 additions & 2 deletions crates/uv-resolver/src/pubgrub/priority.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(),
}
}
Expand Down
78 changes: 37 additions & 41 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand All @@ -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)))
}
}
Expand Down Expand Up @@ -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()),
),
])),
}
}

Expand Down Expand Up @@ -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));
}
Expand Down
38 changes: 14 additions & 24 deletions crates/uv/tests/pip_compile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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 "<string>", line 14, in <module>
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 "<string>", line 3, in <module>
ZeroDivisionError: division by zero
---
Resolved 5 packages in [TIME]
"###);

Ok(())
Expand Down

0 comments on commit 647afcf

Please sign in to comment.