Skip to content

Commit

Permalink
uv-resolver: conditionally include the base package dependency
Browse files Browse the repository at this point in the history
This _partially_ unwinds the optimization in #9540 by adding back the
base package dependency as a sibling to the extra package dependency
in some cases. Specifically, this occurs when _any_ of the extras are
declared as conflicting.

This is believed to be necessary (until another method is found) to
handle the forking logic based on conflicts. Namely, the forking logic
depends on the base and extra packages being sibling dependencies. If
only the extra is present, then it won't be included in the fork that
excludes all conflicting extras. And that means the base package won't
either, even though it should be included in that fork in some cases. If
the base package dependency is deferred, then it will never be reached.

This also adds another test and updates the snapshots that would have
caught the regression in #9540 if the conflict tests had been enabled.
  • Loading branch information
BurntSushi committed Dec 2, 2024
1 parent 8bcb440 commit 81569c4
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 8 deletions.
32 changes: 29 additions & 3 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use tracing::warn;
use uv_normalize::{ExtraName, GroupName, PackageName};
use uv_pep440::{Version, VersionSpecifiers};
use uv_pypi_types::{
ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement,
RequirementSource, VerbatimParsedUrl,
Conflicts, ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl,
Requirement, RequirementSource, VerbatimParsedUrl,
};

use crate::pubgrub::{PubGrubPackage, PubGrubPackageInner};
Expand All @@ -26,14 +26,40 @@ pub(crate) struct PubGrubDependency {

impl PubGrubDependency {
pub(crate) fn from_requirement<'a>(
conflicts: &Conflicts,
requirement: &'a Requirement,
dev: Option<&'a GroupName>,
source_name: Option<&'a PackageName>,
) -> impl Iterator<Item = Self> + 'a {
let iter = if requirement.extras.is_empty() {
Either::Left(iter::once(None))
} else {
Either::Right(requirement.extras.clone().into_iter().map(Some))
// This is crazy subtle, but if any of the extras in the
// requirement are part of a declared conflict, then we
// specifically need (at time of writing) to include the
// base package as a dependency. This results in both
// the base package and the extra package being sibling
// dependencies at the point in which forks are created
// base on conflicting extras. If the base package isn't
// present at that point, then it's impossible for the
// fork that excludes all conflicting extras to reach
// the non-extra dependency, which may be necessary for
// correctness.
//
// But why do we not include the base package in the first
// place? Well, that's part of an optimization[1].
//
// [1]: https://github.com/astral-sh/uv/pull/9540
let base = if requirement
.extras
.iter()
.any(|extra| conflicts.contains(&requirement.name, extra))
{
Either::Left(iter::once(None))
} else {
Either::Right(iter::empty())
};
Either::Right(base.chain(requirement.extras.clone().into_iter().map(Some)))
};

// Add the package, plus any extra variants.
Expand Down
14 changes: 12 additions & 2 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1272,7 +1272,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
requirements
.iter()
.flat_map(|requirement| {
PubGrubDependency::from_requirement(requirement, None, None)
PubGrubDependency::from_requirement(
&self.conflicts,
requirement,
None,
None,
)
})
.collect()
}
Expand Down Expand Up @@ -1460,7 +1465,12 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let mut dependencies: Vec<_> = requirements
.iter()
.flat_map(|requirement| {
PubGrubDependency::from_requirement(requirement, dev.as_ref(), Some(name))
PubGrubDependency::from_requirement(
&self.conflicts,
requirement,
dev.as_ref(),
Some(name),
)
})
.collect();

Expand Down
101 changes: 98 additions & 3 deletions crates/uv/tests/it/lock_conflict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ fn extra_multiple_independent() -> Result<()> {
----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because project[extra1] depends on sortedcontainers==2.3.0 and project[extra2] depends on sortedcontainers==2.4.0, we can conclude that project[extra1] and project[extra2] are incompatible.
╰─▶ Because project[extra2] depends on sortedcontainers==2.4.0 and project[extra1] depends on sortedcontainers==2.3.0, we can conclude that project[extra1] and project[extra2] are incompatible.
And because your project requires project[extra1] and project[extra2], we can conclude that your projects's requirements are unsatisfiable.
"###);

Expand Down Expand Up @@ -1321,8 +1321,8 @@ fn extra_nested_across_workspace() -> Result<()> {
× No solution found when resolving dependencies:
╰─▶ Because dummy[extra2] depends on proxy1[extra2] and only proxy1[extra2]==0.1.0 is available, we can conclude that dummy[extra2] depends on proxy1[extra2]==0.1.0. (1)
Because proxy1[extra1]==0.1.0 depends on anyio==4.1.0 and proxy1[extra2]==0.1.0 depends on anyio==4.2.0, we can conclude that proxy1[extra1]==0.1.0 and proxy1[extra2]==0.1.0 are incompatible.
And because we know from (1) that dummy[extra2] depends on proxy1[extra2]==0.1.0, we can conclude that proxy1[extra1]==0.1.0 and dummy[extra2] are incompatible.
Because proxy1[extra2]==0.1.0 depends on anyio==4.2.0 and proxy1[extra1]==0.1.0 depends on anyio==4.1.0, we can conclude that proxy1[extra1]==0.1.0 and proxy1[extra2]==0.1.0 are incompatible.
And because we know from (1) that dummy[extra2] depends on proxy1[extra2]==0.1.0, we can conclude that dummy[extra2] and proxy1[extra1]==0.1.0 are incompatible.
And because only proxy1[extra1]==0.1.0 is available and dummysub[extra1] depends on proxy1[extra1], we can conclude that dummysub[extra1] and dummy[extra2] are incompatible.
And because your workspace requires dummy[extra2] and dummysub[extra1], we can conclude that your workspace's requirements are unsatisfiable.
"###);
Expand Down Expand Up @@ -2647,3 +2647,98 @@ fn multiple_sources_index_disjoint_extras_with_marker() -> Result<()> {

Ok(())
}

/// Tests that forks excluding both conflicting extras are handled correctly.
///
/// This previously failed where running `uv sync` wouldn't install anything,
/// despite `sniffio` being an unconditional dependency.
#[test]
fn non_optional_dependency_extra() -> Result<()> {
let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"
[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.11"
dependencies = [
"sniffio>=1",
]
[project.optional-dependencies]
x1 = ["idna==3.5"]
x2 = ["idna==3.6"]
[tool.uv]
conflicts = [
[
{package = "project", extra = "x1"},
{package = "project", extra = "x2"},
],
]
"#,
)?;

uv_snapshot!(context.filters(), context.sync(), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Resolved 4 packages in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ sniffio==1.3.1
"###);

Ok(())
}

/// Like `non_optional_dependency_extra`, but for groups.
///
/// This test never regressed, but we added it here to ensure it doesn't.
#[test]
fn non_optional_dependency_group() -> Result<()> {
let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"
[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.11"
dependencies = [
"sniffio>=1",
]
[dependency-groups]
g1 = ["idna==3.5"]
g2 = ["idna==3.6"]
[tool.uv]
conflicts = [
[
{package = "project", group = "g1"},
{package = "project", group = "g2"},
],
]
"#,
)?;

uv_snapshot!(context.filters(), context.sync(), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Resolved 4 packages in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ sniffio==1.3.1
"###);

Ok(())
}

0 comments on commit 81569c4

Please sign in to comment.