Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

uv-resolver: fix basic case of overlapping markers #5488

Merged
merged 1 commit into from
Jul 26, 2024
Merged

uv-resolver: fix basic case of overlapping markers #5488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/uv-resolver/src/marker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ pub(crate) fn requires_python_marker(tree: &MarkerTree) -> Option<RequiresPython
///
/// This is useful in cases where creating conjunctions or disjunctions might occur in a non-deterministic
/// order. This routine will attempt to erase the distinction created by such a construction.
///
/// This returns `None` in the case of normalization turning a `MarkerTree`
/// into an expression that is known to match all possible marker
/// environments. Note though that this may return an empty disjunction, e.g.,
/// `MarkerTree::Or(vec![])`, which matches precisely zero marker environments.
pub(crate) fn normalize(
mut tree: MarkerTree,
bound: Option<&RequiresPythonBound>,
Expand Down
80 changes: 23 additions & 57 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2217,7 +2217,7 @@ impl ForkState {
fn with_markers(mut self, markers: MarkerTree) -> Self {
let combined_markers = self.markers.and(markers);
let combined_markers =
normalize(combined_markers, None).expect("Fork markers are universal");
normalize(combined_markers, None).unwrap_or_else(|| MarkerTree::And(vec![]));

// If the fork contains a narrowed Python requirement, apply it.
let python_requirement = requires_python_marker(&combined_markers)
Expand Down Expand Up @@ -2709,25 +2709,20 @@ impl Dependencies {
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)
});
fork.remove_disjoint_packages();
}
new_forks.extend(new_forks_for_remaining_universe);
}
// Each group has a list of packages whose marker expressions are
// guaranteed to be overlapping. So we must union those marker
// expressions and then intersect them with each existing fork.
for group in fork_groups.forks {
let mut new_forks_for_group = forks.clone();
for (index, _) in group.packages {
for fork in &mut new_forks_for_group {
fork.add_forked_package(deps[index].clone());
for fork in &mut new_forks_for_group {
fork.markers.and(group.union());
fork.remove_disjoint_packages();
for &(index, _) in &group.packages {
fork.dependencies.push(deps[index].clone());
}
}
new_forks.extend(new_forks_for_group);
Expand Down Expand Up @@ -2802,39 +2797,6 @@ struct Fork {
}

impl Fork {
/// Add the given dependency to this fork with the assumption that it
/// provoked this fork into existence.
///
/// In particular, the markers given should correspond to the markers
/// associated with that dependency, and they are combined (via
/// conjunction) with the markers on this fork.
///
/// Finally, and critically, any dependencies that are already in this
/// fork that are disjoint with the markers given are removed. This is
/// because a fork provoked by the given marker should not have packages
/// whose markers are disjoint with it. While it might seem harmless, this
/// can cause the resolver to explore otherwise impossible resolutions,
/// and also run into conflicts (and thus a failed resolution) that don't
/// actually exist.
fn add_forked_package(&mut self, dependency: PubGrubDependency) {
// OK because a package without a marker is unconditional and
// thus can never provoke a fork.
let marker = dependency
.package
.marker()
.cloned()
.expect("forked package always has a marker");
self.remove_disjoint_packages(&marker);
self.dependencies.push(dependency);
// Each marker expression in a single fork is,
// by construction, overlapping with at least
// one other marker expression in this fork.
// However, the symmetric differences may be
// non-empty. Therefore, the markers need to be
// combined on the corresponding fork.
self.markers.and(marker);
}

/// Add the given dependency to this fork.
///
/// This works by assuming the given package did *not* provoke a fork.
Expand All @@ -2854,15 +2816,15 @@ impl Fork {
}

/// Removes any dependencies in this fork whose markers are disjoint with
/// the given markers.
fn remove_disjoint_packages(&mut self, fork_marker: &MarkerTree) {
/// its own markers.
fn remove_disjoint_packages(&mut self) {
use crate::marker::is_disjoint;

self.dependencies.retain(|dependency| {
dependency
.package
.marker()
.map_or(true, |pkg_marker| !is_disjoint(pkg_marker, fork_marker))
.map_or(true, |pkg_marker| !is_disjoint(pkg_marker, &self.markers))
});
}

Expand Down Expand Up @@ -3052,12 +3014,16 @@ impl<'a> PossibleFork<'a> {
/// Each marker expression in the union returned is guaranteed to be overlapping
/// with at least one other expression in the same union.
fn union(&self) -> MarkerTree {
MarkerTree::Or(
self.packages
.iter()
.map(|&(_, tree)| (*tree).clone())
.collect(),
)
let mut trees: Vec<MarkerTree> = self
.packages
.iter()
.map(|&(_, tree)| (*tree).clone())
.collect();
if trees.len() == 1 {
trees.pop().unwrap()
} else {
MarkerTree::Or(trees)
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/uv/tests/lock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2902,7 +2902,7 @@ fn lock_python_version_marker_complement() -> Result<()> {
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' 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 = "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')" },
]

Expand Down
12 changes: 6 additions & 6 deletions crates/uv/tests/pip_compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ; python_version < '3.11' or python_version >= '3.12'
astroid==3.1.0
# via pylint
babel==2.14.0
# via sphinx
Expand All @@ -7778,7 +7778,7 @@ fn universal_no_repeated_unconditional_distributions() -> Result<()> {
# via
# pylint
# sphinx
dill==0.3.8 ; python_version < '3.11' or python_version >= '3.12'
dill==0.3.8
# via pylint
docutils==0.20.1
# via sphinx
Expand All @@ -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 ; python_version < '3.11' or python_version >= '3.12'
isort==5.13.2
# via pylint
jinja2==3.1.3
# via sphinx
markupsafe==2.1.5
# via jinja2
mccabe==0.7.0 ; python_version < '3.11' or python_version >= '3.12'
mccabe==0.7.0
# via pylint
packaging==24.0
# via sphinx
platformdirs==4.2.0 ; python_version < '3.11' or python_version >= '3.12'
platformdirs==4.2.0
# via pylint
pygments==2.17.2
# via sphinx
Expand Down Expand Up @@ -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 ; python_version < '3.11' or python_version >= '3.12'
tomlkit==0.12.4
# via pylint
typing-extensions==4.10.0 ; python_version < '3.11'
# via
Expand Down
Loading