Skip to content

Commit

Permalink
uv-resolver: fix marker propagation
Browse files Browse the repository at this point in the history 
This PR represents a different approach to marker propagation in an
attempt to unblock #4640. In particular, instead of propagating markers
when forks are created, we wait until resolution is complete to
propagate all markers to all dependencies in each fork. This ends up
being both more robust (we should never miss anything) and simpler to
implement because it doesn't require mutating a `PubGrubPackage` (which
was pretty annoying). I think the main downside here is that this can
sometimes add markers where they aren't needed.

This actually winds up making quite a few snapshot changes. I went
through each of them. Some of them look like legitimate bug fixes. Some
of them look like superfluous additions. And some of them look like they
would be removed if we had perfect marker normalization. But I don't
think any of the changes are _wrong_.
  • Loading branch information
@BurntSushi
BurntSushi committed Jul 30, 2024
1 parent 750b3a7 commit 4e74836
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 142 deletions.
5 changes: 0 additions & 5 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ 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,
Expand All @@ -30,10 +29,6 @@ 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>,
Expand Down
72 changes: 0 additions & 72 deletions crates/uv-resolver/src/pubgrub/package.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::mem;
use std::ops::Deref;
use std::sync::Arc;

Expand Down Expand Up @@ -121,77 +120,6 @@ 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 {
Expand Down
49 changes: 30 additions & 19 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2301,7 +2301,36 @@ impl ForkState {
to_url: to_url.cloned(),
to_extra: dependency_extra.clone(),
to_dev: dependency_dev.clone(),
marker: None,
// This propagates markers from the fork to
// packages without any markers. These might wind
// up be duplicative (and are even further merged
// via disjunction when a ResolutionGraph is
// constructed), but normalization should simplify
// most such cases.
//
// In a previous implementation of marker
// propagation, markers were propagated at the
// time a fork was created. But this was crucially
// missing a key detail: the specific version of
// a package outside of a fork can be determined
// by the forks of its dependencies, even when
// that package is not part of a fork at the time
// the forks were created. In that case, it was
// possible for two versions of the same package
// to be unconditionally included in a resolution,
// which must never be.
//
// See https://github.com/astral-sh/uv/pull/5583
// for an example of where this occurs with
// `Sphinx`.
//
// Here, instead, we do the marker propagation
// after resolution has completed. This relies
// on the fact that the markers aren't otherwise
// needed during resolution (which I believe is
// true), but is a more robust approach that should
// capture all cases.
marker: self.markers.fork_markers().cloned(),
};
edges.insert(edge);
}
Expand Down Expand Up @@ -2736,9 +2765,6 @@ impl Dependencies {
forks = new_forks;
diverging_packages.push(name.clone());
}
for fork in &mut forks {
fork.propagate_markers();
}
ForkedDependencies::Forked {
forks,
diverging_packages,
Expand Down Expand Up @@ -2833,21 +2859,6 @@ impl Fork {
.map_or(true, |pkg_marker| !is_disjoint(pkg_marker, &self.markers))
});
}

/// 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
Expand Down
24 changes: 12 additions & 12 deletions crates/uv/tests/branching_urls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,8 @@ fn root_package_splits_transitive_too() -> Result<()> {
"python_version < '3.12'",
]
dependencies = [
{ name = "idna" },
{ name = "sniffio" },
{ name = "idna", marker = "python_version < '3.12'" },
{ name = "sniffio", marker = "python_version < '3.12'" },
]
sdist = { url = "https://files.pythonhosted.org/packages/2d/b8/7333d87d5f03247215d86a86362fd3e324111788c6cdd8d2e6196a6ba833/anyio-4.2.0.tar.gz", hash = "sha256:e1875bb4b4e2de1669f4bc7869b6d3f54231cdced71605e6e64c9be77e3be50f", size = 158770 }
wheels = [
Expand All @@ -248,8 +248,8 @@ fn root_package_splits_transitive_too() -> Result<()> {
"python_version >= '3.12'",
]
dependencies = [
{ name = "idna" },
{ name = "sniffio" },
{ name = "idna", marker = "python_version >= '3.12'" },
{ name = "sniffio", marker = "python_version >= '3.12'" },
]
sdist = { url = "https://files.pythonhosted.org/packages/db/4d/3970183622f0330d3c23d9b8a5f52e365e50381fd484d08e3285104333d3/anyio-4.3.0.tar.gz", hash = "sha256:f75253795a87df48568485fd18cdd2a3fa5c4f7c5be8e5e36637733fce06fed6", size = 159642 }
wheels = [
Expand All @@ -270,15 +270,15 @@ fn root_package_splits_transitive_too() -> Result<()> {
version = "0.1.0"
source = { directory = "../b1" }
dependencies = [
{ name = "iniconfig", version = "1.1.1", source = { url = "https://files.pythonhosted.org/packages/9b/dd/b3c12c6d707058fa947864b67f0c4e0c39ef8610988d7baea9578f3c48f3/iniconfig-1.1.1-py2.py3-none-any.whl" } },
{ name = "iniconfig", version = "1.1.1", source = { url = "https://files.pythonhosted.org/packages/9b/dd/b3c12c6d707058fa947864b67f0c4e0c39ef8610988d7baea9578f3c48f3/iniconfig-1.1.1-py2.py3-none-any.whl" }, marker = "python_version < '3.12'" },
]
[[distribution]]
name = "b2"
version = "0.1.0"
source = { directory = "../b2" }
dependencies = [
{ name = "iniconfig", version = "2.0.0", source = { url = "https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl" } },
{ name = "iniconfig", version = "2.0.0", source = { url = "https://files.pythonhosted.org/packages/ef/a6/62565a6e1cf69e10f5727360368e451d4b7f58beeac6173dc9db836a5b46/iniconfig-2.0.0-py3-none-any.whl" }, marker = "python_version >= '3.12'" },
]
[[distribution]]
Expand Down Expand Up @@ -407,8 +407,8 @@ fn root_package_splits_other_dependencies_too() -> Result<()> {
"python_version < '3.12'",
]
dependencies = [
{ name = "idna" },
{ name = "sniffio" },
{ name = "idna", marker = "python_version < '3.12'" },
{ name = "sniffio", marker = "python_version < '3.12'" },
]
sdist = { url = "https://files.pythonhosted.org/packages/2d/b8/7333d87d5f03247215d86a86362fd3e324111788c6cdd8d2e6196a6ba833/anyio-4.2.0.tar.gz", hash = "sha256:e1875bb4b4e2de1669f4bc7869b6d3f54231cdced71605e6e64c9be77e3be50f", size = 158770 }
wheels = [
Expand All @@ -423,8 +423,8 @@ fn root_package_splits_other_dependencies_too() -> Result<()> {
"python_version >= '3.12'",
]
dependencies = [
{ name = "idna" },
{ name = "sniffio" },
{ name = "idna", marker = "python_version >= '3.12'" },
{ name = "sniffio", marker = "python_version >= '3.12'" },
]
sdist = { url = "https://files.pythonhosted.org/packages/db/4d/3970183622f0330d3c23d9b8a5f52e365e50381fd484d08e3285104333d3/anyio-4.3.0.tar.gz", hash = "sha256:f75253795a87df48568485fd18cdd2a3fa5c4f7c5be8e5e36637733fce06fed6", size = 159642 }
wheels = [
Expand All @@ -436,15 +436,15 @@ fn root_package_splits_other_dependencies_too() -> Result<()> {
version = "0.1.0"
source = { directory = "b1" }
dependencies = [
{ name = "iniconfig", version = "1.1.1", source = { registry = "https://pypi.org/simple" } },
{ name = "iniconfig", version = "1.1.1", source = { registry = "https://pypi.org/simple" }, marker = "python_version < '3.12'" },
]
[[distribution]]
name = "b2"
version = "0.1.0"
source = { directory = "b2" }
dependencies = [
{ name = "iniconfig", version = "2.0.0", source = { registry = "https://pypi.org/simple" } },
{ name = "iniconfig", version = "2.0.0", source = { registry = "https://pypi.org/simple" }, marker = "python_version >= '3.12'" },
]
[[distribution]]
Expand Down
18 changes: 9 additions & 9 deletions crates/uv/tests/lock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2921,9 +2921,9 @@ fn lock_python_version_marker_complement() -> Result<()> {
version = "0.1.0"
source = { editable = "." }
dependencies = [
{ 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') 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 = "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')" },
{ name = "attrs" },
{ name = "iniconfig" },
{ name = "typing-extensions", marker = "python_full_version <= '3.10' or python_full_version > '3.10'" },
]
[[distribution]]
Expand Down Expand Up @@ -4063,8 +4063,8 @@ fn lock_same_version_multiple_urls() -> Result<()> {
"sys_platform != 'darwin'",
]
dependencies = [
{ name = "idna" },
{ name = "sniffio" },
{ name = "idna", marker = "sys_platform != 'darwin'" },
{ name = "sniffio", marker = "sys_platform != 'darwin'" },
]
sdist = { url = "https://files.pythonhosted.org/packages/99/0d/65165f99e5f4f3b4c43a5ed9db0fb7aa655f5a58f290727a30528a87eb45/anyio-3.0.0.tar.gz", hash = "sha256:b553598332c050af19f7d41f73a7790142f5bc3d5eb8bd82f5e515ec22019bd9", size = 116952 }
wheels = [
Expand All @@ -4079,8 +4079,8 @@ fn lock_same_version_multiple_urls() -> Result<()> {
"sys_platform == 'darwin'",
]
dependencies = [
{ name = "idna" },
{ name = "sniffio" },
{ name = "idna", marker = "sys_platform == 'darwin'" },
{ name = "sniffio", marker = "sys_platform == 'darwin'" },
]
sdist = { url = "https://files.pythonhosted.org/packages/c6/b3/fefbf7e78ab3b805dec67d698dc18dd505af7a18a8dd08868c9b4fa736b5/anyio-3.7.0.tar.gz", hash = "sha256:275d9973793619a5374e1c89a4f4ad3f4b0a5510a2b5b939444bee8f4c4d37ce", size = 142737 }
wheels = [
Expand All @@ -4095,7 +4095,7 @@ fn lock_same_version_multiple_urls() -> Result<()> {
"sys_platform == 'darwin'",
]
dependencies = [
{ name = "anyio", version = "3.7.0", source = { registry = "https://pypi.org/simple" } },
{ name = "anyio", version = "3.7.0", source = { registry = "https://pypi.org/simple" }, marker = "sys_platform == 'darwin'" },
]
[[distribution]]
Expand All @@ -4106,7 +4106,7 @@ fn lock_same_version_multiple_urls() -> Result<()> {
"sys_platform != 'darwin'",
]
dependencies = [
{ name = "anyio", version = "3.0.0", source = { registry = "https://pypi.org/simple" } },
{ name = "anyio", version = "3.0.0", source = { registry = "https://pypi.org/simple" }, marker = "sys_platform != 'darwin'" },
]
[[distribution]]
Expand Down
Loading

0 comments on commit 4e74836

Please sign in to comment.