Skip to content

Commit

Permalink
Remove same-graph merging in resolver
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Aug 14, 2024
1 parent 4a902a7 commit 44e880e
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 54 deletions.
38 changes: 0 additions & 38 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,29 +400,6 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
);
}

// If another fork had the same resolution, merge into that fork instead.
if let Some(existing_resolution) = resolutions
.iter_mut()
.find(|existing_resolution| resolution.same_graph(existing_resolution))
{
let ResolverMarkers::Fork(existing_markers) = &existing_resolution.markers
else {
panic!("A non-forking resolution exists in forking mode")
};
let mut new_markers = existing_markers.clone();
new_markers.or(resolution
.markers
.fork_markers()
.expect("A non-forking resolution exists in forking mode")
.clone());
existing_resolution.markers = if new_markers.is_true() {
ResolverMarkers::universal(vec![])
} else {
ResolverMarkers::Fork(new_markers)
};
continue 'FORK;
}

Self::trace_resolution(&resolution);
resolutions.push(resolution);
continue 'FORK;
Expand Down Expand Up @@ -2516,21 +2493,6 @@ pub(crate) struct ResolutionDependencyEdge {
pub(crate) marker: MarkerTree,
}

impl Resolution {
/// Whether we got two identical resolutions in two separate forks.
///
/// Ignores pins since the which distribution we prioritized for each version doesn't matter.
fn same_graph(&self, other: &Self) -> bool {
// TODO(konsti): The edges being equal is not a requirement for the graph being equal. While
// an exact solution is too much here, we should ignore different in edges that point to
// nodes that are always installed. Example: root requires foo, root requires bar. bar
// forks, and one for the branches has bar -> foo while the other doesn't. The resolution
// is still the same graph since the presence or absence of the bar -> foo edge cannot
// change which packages and versions are installed.
self.nodes == other.nodes && self.edges == other.edges
}
}

impl ResolutionPackage {
pub(crate) fn is_base(&self) -> bool {
self.extra.is_none() && self.dev.is_none()
Expand Down
4 changes: 1 addition & 3 deletions crates/uv/tests/ecosystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ fn packse() -> Result<()> {
// Source: https://github.com/konstin/github-wikidata-bot/blob/8218d20985eb480cb8633026f9dabc9e5ec4b5e3/pyproject.toml
#[test]
fn github_wikidata_bot() -> Result<()> {
// TODO(charlie): This test became non-deterministic in https://github.com/astral-sh/uv/pull/6065.
// However, that fix is _correct_, and the non-determinism itself is an existing bug.
lock_ecosystem_package_non_deterministic("3.12", "github-wikidata-bot")
lock_ecosystem_package("3.12", "github-wikidata-bot")
}

// Source: https://github.com/psf/black/blob/9ff047a9575f105f659043f28573e1941e9cdfb3/pyproject.toml
Expand Down
24 changes: 11 additions & 13 deletions crates/uv/tests/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3626,19 +3626,17 @@ fn lock_python_version_marker_complement() -> Result<()> {
);
});

// TODO(charlie): This test became non-deterministic in https://github.com/astral-sh/uv/pull/6065.
// But that fix is correct, and the non-determinism itself is a bug.
// // Re-run with `--locked`.
// uv_snapshot!(context.filters(), context.lock().arg("--locked"), @r###"
// success: false
// exit_code: 2
// ----- stdout -----
//
// ----- stderr -----
// warning: `uv lock` is experimental and may change without warning
// Resolved 4 packages in [TIME]
// error: The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`.
// "###);
// Re-run with `--locked`.
uv_snapshot!(context.filters(), context.lock().arg("--locked"), @r###"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
warning: `uv lock` is experimental and may change without warning
Resolved 4 packages in [TIME]
error: The lockfile at `uv.lock` needs to be updated, but `--locked` was provided. To update the lockfile, run `uv lock`.
"###);

Ok(())
}
Expand Down

0 comments on commit 44e880e

Please sign in to comment.