From 77b005244d3550b3788e583da9819d2dae128094 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Wed, 17 Jul 2024 13:55:48 -0400 Subject: [PATCH] uv-resolver: propagate markers to sibling dependencies in forks When a fork occurs, we divide not just the dependencies that provoked a fork into distinct groups, but we also add the corresponding sibling dependencies to each fork. Previously, while we track markers on the fork itself, the individual dependencies that had markers only corresponded to markers written from the dependency specification. This meant that the sibling dependencies that got added to each fork would not themselves have markers attached to them. This in turn meant they would not have markers associated with them in the lock file. In many cases, this is actually okay, because the resolver will pick a version that is "universal" across all forks in most cases. But in some cases, this just simply isn't possible as the marker expressions in the fork can and do influence resolution. In which case, it is possible for the same package with different versions to show up in the lock file unconditionally. Which is a big no-no. So in this commit, after we determine the forks, we intersect the markers on each fork with each of its dependencies. This does seem to balloon the marker expressions in some cases. I plucked one low hanging fruit to avoid doing `x and x` in trivial cases. (And this eliminated a portion of the snapshot diffs.) But some pretty gnarly diffs remain. This commit also fixes another bug: previously, when we created a fork to capture the "remaining" universe of an incomplete set of markers, we left out dependencies that should be included in that fork. We rectify that here. Fixes #5086 Partially addresses #4732 --- .../uv-resolver/src/pubgrub/dependencies.rs | 5 ++ crates/uv-resolver/src/pubgrub/package.rs | 75 +++++++++++++++++++ crates/uv-resolver/src/resolver/mod.rs | 38 +++++++++- crates/uv/tests/lock.rs | 6 +- crates/uv/tests/lock_scenarios.rs | 8 +- crates/uv/tests/pip_compile.rs | 24 +++--- 6 files changed, 133 insertions(+), 23 deletions(-) diff --git a/crates/uv-resolver/src/pubgrub/dependencies.rs b/crates/uv-resolver/src/pubgrub/dependencies.rs index 97a1a1e4a..ab9243213 100644 --- a/crates/uv-resolver/src/pubgrub/dependencies.rs +++ b/crates/uv-resolver/src/pubgrub/dependencies.rs @@ -5,6 +5,7 @@ use pubgrub::range::Range; use tracing::warn; use pep440_rs::{Version, VersionSpecifiers}; +use pep508_rs::MarkerTree; use pypi_types::{ ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement, RequirementSource, VerbatimParsedUrl, @@ -29,6 +30,10 @@ pub(crate) struct PubGrubDependency { } impl PubGrubDependency { + pub(crate) fn and_markers(&mut self, marker: &MarkerTree) { + self.package.and_markers(marker); + } + pub(crate) fn from_requirement<'a>( requirement: &'a Requirement, source_name: Option<&'a PackageName>, diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs index e677b4d0d..82fe2eb3e 100644 --- a/crates/uv-resolver/src/pubgrub/package.rs +++ b/crates/uv-resolver/src/pubgrub/package.rs @@ -1,3 +1,4 @@ +use std::mem; use std::ops::Deref; use std::sync::Arc; @@ -42,6 +43,9 @@ pub enum PubGrubPackageInner { /// A Python version. Python(PubGrubPython), /// A Python package. + /// + /// Note that it is guaranteed that `extra` and `dev` are never both + /// `Some`. That is, if one is `Some` then the other must be `None`. Package { name: PackageName, extra: Option, @@ -117,6 +121,77 @@ impl PubGrubPackage { } } + /// Modifies this package in place such that it is associated with the + /// given markers by intersecting them any pre-existing markers. + /// + /// That is, this causes the package to only be applicable to marker + /// environments corresponding to the intersection of what it previously + /// matched and the given markers. + /// + /// This is useful when one needs to propagate markers from a fork to each + /// of its constituent dependencies. This is necessary because within a + /// fork, the resolver makes decisions based on the markers that created + /// that fork. While in many cases these decisions are universal, some may + /// not be! And so the markers from the fork must be propagated out to the + /// individual packages. + pub(crate) fn and_markers(&mut self, fork_marker: &MarkerTree) { + // We do a little dance here to pluck out as much existing + // information from this package as we can to avoid allocs. + // It is possible that the `Arc::make_mut` will do a deep + // clone here, thereby defeating our efforts, but I think it + // is likely that in most cases it will not. This is because + // this routine should be called almost immediately after a + // `PubGrubPackage` is created and _before_ it is passed to + // PubGrub itself. + match Arc::make_mut(&mut self.0) { + PubGrubPackageInner::Root(_) | PubGrubPackageInner::Python(_) => {} + // In this case, we may or may not already have a marker. + // If we don't, then this implies we will, and thus we may + // need to switch `Package` to some other representation. + PubGrubPackageInner::Package { + ref mut name, + ref mut extra, + ref dev, + ref mut marker, + } => { + // This case *should* never happen, I believe, because + // a `Package` with a non-None `dev` can only happen as + // a result of a `Dev` package, which we should have + // processed already by the time we get this package. + if dev.is_some() { + return; + } + + let mut and = fork_marker.clone(); + if let Some(marker) = marker.take() { + and.and(marker); + } + *self = PubGrubPackage::from_package(mem::take(name), extra.take(), Some(and)); + } + // These cases are easy, because we can just modify the marker + // in place. + PubGrubPackageInner::Extra { ref mut marker, .. } => { + let mut and = fork_marker.clone(); + if let Some(marker) = marker.take() { + and.and(marker); + } + *marker = Some(and); + } + PubGrubPackageInner::Dev { ref mut marker, .. } => { + let mut and = fork_marker.clone(); + if let Some(marker) = marker.take() { + and.and(marker); + } + *marker = Some(and); + } + PubGrubPackageInner::Marker { ref mut marker, .. } => { + let mut and = fork_marker.clone(); + and.and(mem::replace(marker, MarkerTree::And(vec![]))); + *marker = and; + } + } + } + /// Returns the name of this PubGrub package, if it has one. pub(crate) fn name(&self) -> Option<&PackageName> { match &**self { diff --git a/crates/uv-resolver/src/resolver/mod.rs b/crates/uv-resolver/src/resolver/mod.rs index 19cb4be6c..8d94fcef7 100644 --- a/crates/uv-resolver/src/resolver/mod.rs +++ b/crates/uv-resolver/src/resolver/mod.rs @@ -2735,10 +2735,22 @@ impl Dependencies { let mut new_forks: Vec = vec![]; if let Some(markers) = fork_groups.remaining_universe() { trace!("Adding split to cover possibly incomplete markers: {markers}"); - new_forks.push(Fork { - dependencies: vec![], - markers, - }); + let mut new_forks_for_remaining_universe = forks.clone(); + for fork in &mut new_forks_for_remaining_universe { + fork.markers.and(markers.clone()); + fork.dependencies.retain(|dep| { + let Some(dep_marker) = dep.package.marker() else { + return true; + }; + // After we constrain the markers on an existing + // fork, we should ensure that any existing + // dependencies that are no longer possible in this + // fork are removed. This mirrors the check we do in + // `add_nonfork_package`. + !crate::marker::is_disjoint(&fork.markers, dep_marker) + }); + } + new_forks.extend(new_forks_for_remaining_universe); } for group in fork_groups.forks { let mut new_forks_for_group = forks.clone(); @@ -2752,6 +2764,9 @@ impl Dependencies { forks = new_forks; diverging_packages.push(name.clone()); } + for fork in &mut forks { + fork.propagate_markers(); + } ForkedDependencies::Forked { forks, diverging_packages, @@ -2879,6 +2894,21 @@ impl Fork { .map_or(true, |pkg_marker| !is_disjoint(pkg_marker, fork_marker)) }); } + + /// This attaches the marker in this fork to each of its dependencies. + /// + /// In effect, this "propagates" the markers to each individual dependency + /// that was spawned as the result of a fork. While in many cases the + /// markers will be combined when multiple forks choose the same version of + /// a dependency, in some cases, the version chosen can be specific to a + /// particular set of marker environments. In this case, the dependencies + /// will be platform specific and thus require marker expressions to appear + /// in the lock file. + fn propagate_markers(&mut self) { + for dependency in &mut self.dependencies { + dependency.and_markers(&self.markers); + } + } } /// Intermediate state that represents a *possible* grouping of forks diff --git a/crates/uv/tests/lock.rs b/crates/uv/tests/lock.rs index ffd99710c..0fcc3a58a 100644 --- a/crates/uv/tests/lock.rs +++ b/crates/uv/tests/lock.rs @@ -2901,9 +2901,9 @@ fn lock_python_version_marker_complement() -> Result<()> { version = "0.1.0" source = { editable = "." } dependencies = [ - { name = "attrs" }, - { name = "iniconfig" }, - { name = "typing-extensions", marker = "python_full_version <= '3.10' or python_full_version > '3.10'" }, + { name = "attrs", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version > '3.10')" }, + { name = "iniconfig", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version > '3.10')" }, + { name = "typing-extensions", marker = "(python_full_version <= '3.10' and python_version == '3.10') or (python_full_version <= '3.10' and python_version < '3.10') or (python_full_version <= '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version <= '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version == '3.10') or (python_full_version > '3.10' and python_version < '3.10') or (python_full_version > '3.10' and python_version < '3.10' and python_version > '3.10') or (python_full_version > '3.10' and python_version > '3.10')" }, ] [[distribution]] diff --git a/crates/uv/tests/lock_scenarios.rs b/crates/uv/tests/lock_scenarios.rs index 5b7cf9871..553ab3136 100644 --- a/crates/uv/tests/lock_scenarios.rs +++ b/crates/uv/tests/lock_scenarios.rs @@ -723,7 +723,7 @@ fn fork_incomplete_markers() -> Result<()> { dependencies = [ { name = "package-a", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "python_version < '3.10'" }, { name = "package-a", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "python_version >= '3.11'" }, - { name = "package-b" }, + { name = "package-b", marker = "python_version < '3.10' or python_version >= '3.11' or (python_version < '3.11' and python_version >= '3.10')" }, ] "### ); @@ -1804,7 +1804,7 @@ fn fork_marker_limited_inherit() -> Result<()> { dependencies = [ { name = "package-a", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'darwin'" }, { name = "package-a", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'linux'" }, - { name = "package-b" }, + { name = "package-b", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or (sys_platform != 'darwin' and sys_platform != 'linux')" }, ] "### ); @@ -1919,7 +1919,7 @@ fn fork_marker_selection() -> Result<()> { version = "0.1.0" source = { editable = "." } dependencies = [ - { name = "package-a" }, + { name = "package-a", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or (sys_platform != 'darwin' and sys_platform != 'linux')" }, { name = "package-b", version = "1.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'darwin'" }, { name = "package-b", version = "2.0.0", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'linux'" }, ] @@ -2059,7 +2059,7 @@ fn fork_marker_track() -> Result<()> { version = "0.1.0" source = { editable = "." } dependencies = [ - { name = "package-a" }, + { name = "package-a", marker = "sys_platform == 'darwin' or sys_platform == 'linux' or (sys_platform != 'darwin' and sys_platform != 'linux')" }, { name = "package-b", version = "2.7", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'darwin'" }, { name = "package-b", version = "2.8", source = { registry = "https://astral-sh.github.io/packse/0.3.30/simple-html/" }, marker = "sys_platform == 'linux'" }, ] diff --git a/crates/uv/tests/pip_compile.rs b/crates/uv/tests/pip_compile.rs index d6fe40a6a..2fc3b5072 100644 --- a/crates/uv/tests/pip_compile.rs +++ b/crates/uv/tests/pip_compile.rs @@ -7298,7 +7298,7 @@ fn universal_nested_overlapping_local_requirement() -> Result<()> { # uv pip compile --cache-dir [CACHE_DIR] requirements.in --universal cmake==3.28.4 ; platform_machine == 'x86_64' and platform_system == 'Linux' # via triton - . ; os_name == 'Linux' + . ; (os_name == 'Linux' and platform_machine == 'x86_64') or (os_name == 'Linux' and platform_machine != 'x86_64') # via -r requirements.in filelock==3.13.1 # via @@ -7766,7 +7766,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # uv pip compile --cache-dir [CACHE_DIR] requirements.in -p 3.8 --universal alabaster==0.7.13 # via sphinx - astroid==3.1.0 + astroid==3.1.0 ; python_version < '3.11' or python_version >= '3.12' # via pylint babel==2.14.0 # via sphinx @@ -7778,7 +7778,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # via # pylint # sphinx - dill==0.3.8 + dill==0.3.8 ; python_version < '3.11' or python_version >= '3.12' # via pylint docutils==0.20.1 # via sphinx @@ -7788,17 +7788,17 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # via sphinx importlib-metadata==7.1.0 ; python_version < '3.10' # via sphinx - isort==5.13.2 + isort==5.13.2 ; python_version < '3.11' or python_version >= '3.12' # via pylint jinja2==3.1.3 # via sphinx markupsafe==2.1.5 # via jinja2 - mccabe==0.7.0 + mccabe==0.7.0 ; python_version < '3.11' or python_version >= '3.12' # via pylint packaging==24.0 # via sphinx - platformdirs==4.2.0 + platformdirs==4.2.0 ; python_version < '3.11' or python_version >= '3.12' # via pylint pygments==2.17.2 # via sphinx @@ -7826,7 +7826,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> { # via sphinx tomli==2.0.1 ; python_version < '3.11' # via pylint - tomlkit==0.12.4 + tomlkit==0.12.4 ; python_version < '3.11' or python_version >= '3.12' # via pylint typing-extensions==4.10.0 ; python_version < '3.11' # via @@ -7916,7 +7916,7 @@ fn universal_marker_propagation() -> Result<()> { # via requests filelock==3.13.1 # via torch - fsspec==2024.3.1 + fsspec==2024.3.1 ; platform_machine != 'x86_64' # via torch idna==3.6 # via requests @@ -7936,17 +7936,17 @@ fn universal_marker_propagation() -> Result<()> { # via torchvision sympy==1.12 # via torch - torch==2.0.0 + torch==2.0.0 ; platform_machine == 'x86_64' # via # -r requirements.in # torchvision - torch==2.2.0 + torch==2.2.0 ; platform_machine != 'x86_64' # via # -r requirements.in # torchvision - torchvision==0.15.1+rocm5.4.2 + torchvision==0.15.1+rocm5.4.2 ; platform_machine == 'x86_64' # via -r requirements.in - torchvision==0.17.0+rocm5.7 + torchvision==0.17.0+rocm5.7 ; platform_machine != 'x86_64' # via -r requirements.in typing-extensions==4.10.0 # via torch