Add support for extras in editable requirements (#1531)

## 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 https://github.com/astral-sh/uv/issues/1447.

## Test Plan

- Cloned `attrs`.
- Ran `cargo run venv && cargo run pip install -e ".[dev]" -v`.
This commit is contained in:
Charlie Marsh 2024-02-16 18:48:35 -05:00 committed by GitHub
parent 4e0b6f8f84
commit 6392961f44
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 92 additions and 37 deletions

View file

@ -65,10 +65,13 @@ impl ResolutionGraph {
match package { match package {
PubGrubPackage::Package(package_name, None, None) => { PubGrubPackage::Package(package_name, None, None) => {
// Create the distribution. // Create the distribution.
let pinned_package = pins let pinned_package = if let Some((editable, _)) = editables.get(package_name) {
.get(package_name, version) Dist::from_editable(package_name.clone(), editable.clone())?
.expect("Every package should be pinned") } else {
.clone(); pins.get(package_name, version)
.expect("Every package should be pinned")
.clone()
};
// Add its hashes to the index. // Add its hashes to the index.
if let Some(versions_response) = packages.get(package_name) { if let Some(versions_response) = packages.get(package_name) {
@ -87,9 +90,7 @@ impl ResolutionGraph {
} }
PubGrubPackage::Package(package_name, None, Some(url)) => { PubGrubPackage::Package(package_name, None, Some(url)) => {
// Create the distribution. // Create the distribution.
let pinned_package = if let Some((editable, _)) = editables.get(package_name) { let pinned_package = {
Dist::from_editable(package_name.clone(), editable.clone())?
} else {
let url = redirects.get(url).map_or_else( let url = redirects.get(url).map_or_else(
|| url.clone(), || url.clone(),
|url| VerbatimUrl::unknown(url.value().clone()), |url| VerbatimUrl::unknown(url.value().clone()),
@ -115,20 +116,35 @@ impl ResolutionGraph {
PubGrubPackage::Package(package_name, Some(extra), None) => { PubGrubPackage::Package(package_name, Some(extra), None) => {
// Validate that the `extra` exists. // Validate that the `extra` exists.
let dist = PubGrubDistribution::from_registry(package_name, version); 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) { if let Some((_, metadata)) = editables.get(package_name) {
let pinned_package = pins if !metadata.provides_extras.contains(extra) {
.get(package_name, version) let pinned_package = pins
.expect("Every package should be pinned") .get(package_name, version)
.clone(); .expect("Every package should be pinned")
.clone();
diagnostics.push(Diagnostic::MissingExtra { diagnostics.push(Diagnostic::MissingExtra {
dist: pinned_package, dist: pinned_package,
extra: extra.clone(), 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)) => { PubGrubPackage::Package(package_name, Some(extra), Some(url)) => {

View file

@ -160,16 +160,8 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
// Determine all the editable requirements. // Determine all the editable requirements.
let mut editables = FxHashMap::default(); let mut editables = FxHashMap::default();
for (editable_requirement, metadata) in &manifest.editables { 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( editables.insert(
dist.name().clone(), metadata.name.clone(),
(editable_requirement.clone(), metadata.clone()), (editable_requirement.clone(), metadata.clone()),
); );
} }
@ -633,6 +625,16 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
} }
PubGrubPackage::Package(package_name, extra, None) => { 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. // Wait for the metadata to be available.
let versions_response = self let versions_response = self
.index .index
@ -798,11 +800,7 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
// Add a dependency on each editable. // Add a dependency on each editable.
for (editable, metadata) in self.editables.values() { for (editable, metadata) in self.editables.values() {
constraints.insert( constraints.insert(
PubGrubPackage::Package( PubGrubPackage::Package(metadata.name.clone(), None, None),
metadata.name.clone(),
None,
Some(editable.url().clone()),
),
Range::singleton(metadata.version.clone()), Range::singleton(metadata.version.clone()),
); );
for extra in &editable.extras { for extra in &editable.extras {
@ -810,7 +808,7 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
PubGrubPackage::Package( PubGrubPackage::Package(
metadata.name.clone(), metadata.name.clone(),
Some(extra.clone()), Some(extra.clone()),
Some(editable.url().clone()), None,
), ),
Range::singleton(metadata.version.clone()), Range::singleton(metadata.version.clone()),
); );
@ -830,7 +828,37 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
return Ok(Dependencies::Available(DependencyConstraints::default())); 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 { let dist = match url {
Some(url) => PubGrubDistribution::from_url(package_name, url), Some(url) => PubGrubDistribution::from_url(package_name, url),
None => PubGrubDistribution::from_registry(package_name, version), None => PubGrubDistribution::from_registry(package_name, version),
@ -984,6 +1012,11 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
// Pre-fetch the package and distribution metadata. // Pre-fetch the package and distribution metadata.
Request::Prefetch(package_name, range) => { 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. // Wait for the package metadata to become available.
let versions_response = self let versions_response = self
.index .index

View file

@ -2075,7 +2075,7 @@ fn compile_editable() -> Result<()> {
requirements_in.write_str(indoc! {r" requirements_in.write_str(indoc! {r"
-e ../../scripts/editable-installs/poetry_editable -e ../../scripts/editable-installs/poetry_editable
-e ${PROJECT_ROOT}/../../scripts/editable-installs/maturin_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 boltons # normal dependency for comparison
" "
})?; })?;
@ -2122,12 +2122,14 @@ fn compile_editable() -> Result<()> {
# yarl # yarl
numpy==1.26.2 numpy==1.26.2
# via poetry-editable # via poetry-editable
uvloop==0.19.0
# via black
yarl==1.9.2 yarl==1.9.2
# via aiohttp # via aiohttp
----- stderr ----- ----- stderr -----
Built 3 editables in [TIME] Built 3 editables in [TIME]
Resolved 12 packages in [TIME] Resolved 13 packages in [TIME]
"###); "###);
Ok(()) Ok(())

View file

@ -20,6 +20,10 @@ jupyter = [
"ipython>=7.8.0", "ipython>=7.8.0",
"tokenize-rt>=3.2.0", "tokenize-rt>=3.2.0",
] ]
dev = [
"black[d]",
"black[uvloop]",
]
[build-system] [build-system]
requires = ["flit_core>=3.4,<4"] requires = ["flit_core>=3.4,<4"]