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.

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

Remove same-graph merging in resolver #6077

Merged
merged 1 commit into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Loading