Skip to content

Commit

Permalink
Add support for extras in editable requirements (#1531)
Browse files Browse the repository at this point in the history
## Summary

If you're developing on a package like `attrs` locally, and it has a
recursive extra like `attrs[dev]`, it turns out that we then try to find
the `attrs` in `attrs[dev]` from the registry, rather than recognizing
that it's part of the editable.

This PR fixes the issue by making editables slightly more first-class
throughout the resolver. Instead of mocking metadata, we explicitly
check for extras in various places. Part of the problem here is that we
treated editables as URL dependencies, but when we saw an _extra_ like
`attrs[dev]`, we didn't map that back to the URL. So now, we treat them
as registry dependencies, but with the appropriate guardrails
throughout.

Closes #1447.

## Test Plan

- Cloned `attrs`.
- Ran `cargo run venv && cargo run pip install -e ".[dev]" -v`.
  • Loading branch information
charliermarsh authored Feb 16, 2024
1 parent 4e0b6f8 commit 6392961
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 37 deletions.
54 changes: 35 additions & 19 deletions crates/uv-resolver/src/resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,13 @@ impl ResolutionGraph {
match package {
PubGrubPackage::Package(package_name, None, None) => {
// Create the distribution.
let pinned_package = pins
.get(package_name, version)
.expect("Every package should be pinned")
.clone();
let pinned_package = if let Some((editable, _)) = editables.get(package_name) {
Dist::from_editable(package_name.clone(), editable.clone())?
} else {
pins.get(package_name, version)
.expect("Every package should be pinned")
.clone()
};

// Add its hashes to the index.
if let Some(versions_response) = packages.get(package_name) {
Expand All @@ -87,9 +90,7 @@ impl ResolutionGraph {
}
PubGrubPackage::Package(package_name, None, Some(url)) => {
// Create the distribution.
let pinned_package = if let Some((editable, _)) = editables.get(package_name) {
Dist::from_editable(package_name.clone(), editable.clone())?
} else {
let pinned_package = {
let url = redirects.get(url).map_or_else(
|| url.clone(),
|url| VerbatimUrl::unknown(url.value().clone()),
Expand All @@ -115,20 +116,35 @@ impl ResolutionGraph {
PubGrubPackage::Package(package_name, Some(extra), None) => {
// Validate that the `extra` exists.
let dist = PubGrubDistribution::from_registry(package_name, version);
let metadata = distributions
.get(&dist.package_id())
.expect("Every package should have metadata");

if !metadata.provides_extras.contains(extra) {
let pinned_package = pins
.get(package_name, version)
.expect("Every package should be pinned")
.clone();
if let Some((_, metadata)) = editables.get(package_name) {
if !metadata.provides_extras.contains(extra) {
let pinned_package = pins
.get(package_name, version)
.expect("Every package should be pinned")
.clone();

diagnostics.push(Diagnostic::MissingExtra {
dist: pinned_package,
extra: extra.clone(),
});
diagnostics.push(Diagnostic::MissingExtra {
dist: pinned_package,
extra: extra.clone(),
});
}
} else {
let metadata = distributions
.get(&dist.package_id())
.expect("Every package should have metadata");

if !metadata.provides_extras.contains(extra) {
let pinned_package = pins
.get(package_name, version)
.expect("Every package should be pinned")
.clone();

diagnostics.push(Diagnostic::MissingExtra {
dist: pinned_package,
extra: extra.clone(),
});
}
}
}
PubGrubPackage::Package(package_name, Some(extra), Some(url)) => {
Expand Down
65 changes: 49 additions & 16 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,16 +160,8 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
// Determine all the editable requirements.
let mut editables = FxHashMap::default();
for (editable_requirement, metadata) in &manifest.editables {
// Convert the editable requirement into a distribution.
let dist = Dist::from_editable(metadata.name.clone(), editable_requirement.clone())
.expect("This is a valid distribution");

// Mock editable responses.
let package_id = dist.package_id();
index.distributions.register(package_id.clone());
index.distributions.done(package_id, metadata.clone());
editables.insert(
dist.name().clone(),
metadata.name.clone(),
(editable_requirement.clone(), metadata.clone()),
);
}
Expand Down Expand Up @@ -633,6 +625,16 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
}

PubGrubPackage::Package(package_name, extra, None) => {
// If the dist is an editable, return the version from the editable metadata.
if let Some((_local, metadata)) = self.editables.get(package_name) {
let version = metadata.version.clone();
return if range.contains(&version) {
Ok(Some(ResolverVersion::Available(version)))
} else {
Ok(None)
};
}

// Wait for the metadata to be available.
let versions_response = self
.index
Expand Down Expand Up @@ -798,19 +800,15 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
// Add a dependency on each editable.
for (editable, metadata) in self.editables.values() {
constraints.insert(
PubGrubPackage::Package(
metadata.name.clone(),
None,
Some(editable.url().clone()),
),
PubGrubPackage::Package(metadata.name.clone(), None, None),
Range::singleton(metadata.version.clone()),
);
for extra in &editable.extras {
constraints.insert(
PubGrubPackage::Package(
metadata.name.clone(),
Some(extra.clone()),
Some(editable.url().clone()),
None,
),
Range::singleton(metadata.version.clone()),
);
Expand All @@ -830,7 +828,37 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
return Ok(Dependencies::Available(DependencyConstraints::default()));
}

// Determine the distribution to lookup
// Determine if the distribution is editable.
if let Some((_local, metadata)) = self.editables.get(package_name) {
let mut constraints = PubGrubDependencies::from_requirements(
&metadata.requires_dist,
&self.constraints,
&self.overrides,
Some(package_name),
extra.as_ref(),
self.markers,
)?;

for (package, version) in constraints.iter() {
debug!("Adding transitive dependency: {package}{version}");

// Emit a request to fetch the metadata for this package.
self.visit_package(package, priorities, request_sink)
.await?;
}

// If a package has an extra, insert a constraint on the base package.
if extra.is_some() {
constraints.insert(
PubGrubPackage::Package(package_name.clone(), None, None),
Range::singleton(version.clone()),
);
}

return Ok(Dependencies::Available(constraints.into()));
}

// Determine the distribution to lookup.
let dist = match url {
Some(url) => PubGrubDistribution::from_url(package_name, url),
None => PubGrubDistribution::from_registry(package_name, version),
Expand Down Expand Up @@ -984,6 +1012,11 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {

// Pre-fetch the package and distribution metadata.
Request::Prefetch(package_name, range) => {
// Ignore editables.
if self.editables.contains_key(&package_name) {
return Ok(None);
}

// Wait for the package metadata to become available.
let versions_response = self
.index
Expand Down
6 changes: 4 additions & 2 deletions crates/uv/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2075,7 +2075,7 @@ fn compile_editable() -> Result<()> {
requirements_in.write_str(indoc! {r"
-e ../../scripts/editable-installs/poetry_editable
-e ${PROJECT_ROOT}/../../scripts/editable-installs/maturin_editable
-e file://../../scripts/editable-installs/black_editable[d]
-e file://../../scripts/editable-installs/black_editable[dev]
boltons # normal dependency for comparison
"
})?;
Expand Down Expand Up @@ -2122,12 +2122,14 @@ fn compile_editable() -> Result<()> {
# yarl
numpy==1.26.2
# via poetry-editable
uvloop==0.19.0
# via black
yarl==1.9.2
# via aiohttp
----- stderr -----
Built 3 editables in [TIME]
Resolved 12 packages in [TIME]
Resolved 13 packages in [TIME]
"###);

Ok(())
Expand Down
4 changes: 4 additions & 0 deletions scripts/editable-installs/black_editable/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ jupyter = [
"ipython>=7.8.0",
"tokenize-rt>=3.2.0",
]
dev = [
"black[d]",
"black[uvloop]",
]

[build-system]
requires = ["flit_core>=3.4,<4"]
Expand Down

0 comments on commit 6392961

Please sign in to comment.