diff --git a/crates/uv-resolver/src/pubgrub/dependencies.rs b/crates/uv-resolver/src/pubgrub/dependencies.rs index 65d923fa3..ea39304dd 100644 --- a/crates/uv-resolver/src/pubgrub/dependencies.rs +++ b/crates/uv-resolver/src/pubgrub/dependencies.rs @@ -1,5 +1,6 @@ use itertools::Itertools; use pubgrub::range::Range; +use rustc_hash::FxHashSet; use tracing::warn; use distribution_types::Verbatim; @@ -30,70 +31,20 @@ impl PubGrubDependencies { env: &MarkerEnvironment, ) -> Result { let mut dependencies = Vec::default(); + let mut seen = FxHashSet::default(); - // Iterate over all declared requirements. - for requirement in overrides.apply(requirements) { - // If the requirement isn't relevant for the current platform, skip it. - if let Some(extra) = source_extra { - if !requirement.evaluate_markers(env, std::slice::from_ref(extra)) { - continue; - } - } else if !requirement.evaluate_markers(env, &[]) { - continue; - } - - // Add the package, plus any extra variants. - for result in std::iter::once(to_pubgrub(requirement, None, urls, locals)).chain( - requirement - .extras - .clone() - .into_iter() - .map(|extra| to_pubgrub(requirement, Some(extra), urls, locals)), - ) { - let (package, version) = result?; - - // Detect self-dependencies. - if let PubGrubPackage::Package(name, extra, ..) = &package { - if source_name.is_some_and(|source_name| source_name == name) { - // Allow, e.g., `black` to depend on `black[colorama]`. - if source_extra == extra.as_ref() { - warn!("{name} has a dependency on itself"); - continue; - } - } - } - - dependencies.push((package.clone(), version.clone())); - - // If the requirement was constrained, add those constraints. - for constraint in constraints.get(&requirement.name).into_iter().flatten() { - // If the requirement isn't relevant for the current platform, skip it. - if let Some(extra) = source_extra { - if !constraint.evaluate_markers(env, std::slice::from_ref(extra)) { - continue; - } - } else if !constraint.evaluate_markers(env, &[]) { - continue; - } - - // Add the package. - let (package, version) = to_pubgrub(constraint, None, urls, locals)?; - - // Detect self-dependencies. - if let PubGrubPackage::Package(name, extra, ..) = &package { - if source_name.is_some_and(|source_name| source_name == name) { - // Allow, e.g., `black` to depend on `black[colorama]`. - if source_extra == extra.as_ref() { - warn!("{name} has a dependency on itself"); - continue; - } - } - } - - dependencies.push((package.clone(), version.clone())); - } - } - } + add_requirements( + requirements, + constraints, + overrides, + source_name, + source_extra, + urls, + locals, + env, + &mut dependencies, + &mut seen, + )?; Ok(Self(dependencies)) } @@ -109,6 +60,117 @@ impl PubGrubDependencies { } } +/// Add a set of requirements to a list of dependencies. +#[allow(clippy::too_many_arguments)] +fn add_requirements( + requirements: &[Requirement], + constraints: &Constraints, + overrides: &Overrides, + source_name: Option<&PackageName>, + source_extra: Option<&ExtraName>, + urls: &Urls, + locals: &Locals, + env: &MarkerEnvironment, + dependencies: &mut Vec<(PubGrubPackage, Range)>, + seen: &mut FxHashSet, +) -> Result<(), ResolveError> { + // Iterate over all declared requirements. + for requirement in overrides.apply(requirements) { + // If the requirement isn't relevant for the current platform, skip it. + match source_extra { + Some(source_extra) => { + if !requirement.evaluate_markers(env, std::slice::from_ref(source_extra)) { + continue; + } + } + None => { + if !requirement.evaluate_markers(env, &[]) { + continue; + } + } + } + + // Add the package, plus any extra variants. + for result in std::iter::once(to_pubgrub(requirement, None, urls, locals)).chain( + requirement + .extras + .clone() + .into_iter() + .map(|extra| to_pubgrub(requirement, Some(extra), urls, locals)), + ) { + let (package, version) = result?; + + match &package { + PubGrubPackage::Package(name, ..) => { + // Detect self-dependencies. + if source_name.is_some_and(|source_name| source_name == name) { + warn!("{name} has a dependency on itself"); + continue; + } + + dependencies.push((package.clone(), version.clone())); + } + PubGrubPackage::Extra(name, extra, ..) => { + // Recursively add the dependencies of the current package (e.g., `black` depending on + // `black[colorama]`). + if source_name.is_some_and(|source_name| source_name == name) { + if seen.insert(extra.clone()) { + add_requirements( + requirements, + constraints, + overrides, + source_name, + Some(extra), + urls, + locals, + env, + dependencies, + seen, + )?; + } + } else { + dependencies.push((package.clone(), version.clone())); + } + } + _ => {} + } + + // If the requirement was constrained, add those constraints. + for constraint in constraints.get(&requirement.name).into_iter().flatten() { + // If the requirement isn't relevant for the current platform, skip it. + match source_extra { + Some(source_extra) => { + if !constraint.evaluate_markers(env, std::slice::from_ref(source_extra)) { + continue; + } + } + None => { + if !constraint.evaluate_markers(env, &[]) { + continue; + } + } + } + + // Add the package. + let (package, version) = to_pubgrub(constraint, None, urls, locals)?; + + // Ignore self-dependencies. + if let PubGrubPackage::Package(name, ..) = &package { + // Detect self-dependencies. + if source_name.is_some_and(|source_name| source_name == name) { + warn!("{name} has a dependency on itself"); + continue; + } + } + + dependencies.push((package.clone(), version.clone())); + } + } + } + + Ok(()) +} + /// Convert a [`PubGrubDependencies`] to a [`DependencyConstraints`]. impl From for Vec<(PubGrubPackage, Range)> { fn from(dependencies: PubGrubDependencies) -> Self { diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index 23b3ac153..99ca35757 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -3985,6 +3985,56 @@ fn avoid_irrelevant_extras() -> Result<()> { Ok(()) } +/// `extras==0.0.2` fails to build (i.e., it always throws). `extras==0.0.1` is the only version +/// that resolves the constraints, but if we don't visit `example[test]` prior to `extras==0.0.2`, +/// we'll end up with a broken build. +#[test] +fn avoid_irrelevant_recursive_extras() -> Result<()> { + let context = TestContext::new("3.12"); + + // Create an editable package with an optional URL dependency. + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#"[project] +name = "example" +version = "0.0.0" +dependencies = [] +requires-python = '>=3.8' + +[project.optional-dependencies] +test = ["extras<0.0.2"] +coverage = ["example[test]", "extras>=0.0.1,<=0.0.2"] +"#, + )?; + + // Write to a requirements file. + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str("-e .[test,coverage]")?; + + uv_snapshot!(context.compile() + .arg("requirements.in") + .arg("--find-links") + .arg(context.workspace_root.join("scripts").join("links")), @r###" + 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 + -e . + extras==0.0.1 + # via example + iniconfig==2.0.0 + # via extras + + ----- stderr ----- + Built 1 editable in [TIME] + Resolved 3 packages in [TIME] + "### + ); + + Ok(()) +} + /// Use an existing resolution for `black==23.10.1`, with stale versions of `click` and `pathspec`. /// Nothing should change. #[test]