From 1ac9672b95f6d2109705e956e116d7a72a70d97e Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Tue, 2 Apr 2024 13:48:33 -0500 Subject: [PATCH] Resolve non-determistic behavior in preferences due to site-packages ordering (#2780) Originally a regression test for #2779 but we found out that there's some weird behavior where different `anyio` versions were preferred based on the platform. --- crates/uv-installer/src/site_packages.rs | 9 ++- crates/uv-resolver/src/candidate_selector.rs | 8 +- crates/uv-resolver/src/preferences.rs | 3 + crates/uv/tests/pip_install.rs | 83 +++++++++++--------- 4 files changed, 60 insertions(+), 43 deletions(-) diff --git a/crates/uv-installer/src/site_packages.rs b/crates/uv-installer/src/site_packages.rs index d8912b97f..9ec723ea6 100644 --- a/crates/uv-installer/src/site_packages.rs +++ b/crates/uv-installer/src/site_packages.rs @@ -46,7 +46,13 @@ impl<'a> SitePackages<'a> { for site_packages in venv.site_packages() { // Read the site-packages directory. let site_packages = match fs::read_dir(site_packages) { - Ok(site_packages) => site_packages, + Ok(site_packages) => { + let mut entries = site_packages.collect::, std::io::Error>>()?; + // TODO(zanieb): Consider filtering to just directories to reduce the size of the sort + // Sort for determinism, `read_dir` is different per-platform + entries.sort_by_key(fs_err::DirEntry::path); + entries + } Err(err) if err.kind() == std::io::ErrorKind::NotFound => { return Ok(Self { venv, @@ -60,7 +66,6 @@ impl<'a> SitePackages<'a> { // Index all installed packages by name. for entry in site_packages { - let entry = entry?; if entry.file_type()?.is_dir() { let path = entry.path(); diff --git a/crates/uv-resolver/src/candidate_selector.rs b/crates/uv-resolver/src/candidate_selector.rs index 6ce986505..eff292bd9 100644 --- a/crates/uv-resolver/src/candidate_selector.rs +++ b/crates/uv-resolver/src/candidate_selector.rs @@ -100,7 +100,9 @@ impl CandidateSelector { } // We do not consider installed distributions with multiple versions because // during installation these must be reinstalled from the remote - _ => {} + _ => { + debug!("Ignoring installed versions of {package_name}: multiple distributions found"); + } } } @@ -130,7 +132,9 @@ impl CandidateSelector { } // We do not consider installed distributions with multiple versions because // during installation these must be reinstalled from the remote - _ => {} + _ => { + debug!("Ignoring installed versions of {package_name}: multiple distributions found"); + } } } diff --git a/crates/uv-resolver/src/preferences.rs b/crates/uv-resolver/src/preferences.rs index 36e20770f..95bccb464 100644 --- a/crates/uv-resolver/src/preferences.rs +++ b/crates/uv-resolver/src/preferences.rs @@ -79,6 +79,9 @@ impl Preferences { markers: &MarkerEnvironment, ) -> Self { Self( + // TODO(zanieb): We should explicitly ensure that when a package name is seen multiple times + // that the newest or oldest version is preferred dependning on the resolution strategy; + // right now, the order is dependent on the given iterator. preferences .into_iter() .filter_map(|preference| { diff --git a/crates/uv/tests/pip_install.rs b/crates/uv/tests/pip_install.rs index 168ca5842..ee987fb0b 100644 --- a/crates/uv/tests/pip_install.rs +++ b/crates/uv/tests/pip_install.rs @@ -3517,53 +3517,38 @@ fn already_installed_local_version_of_remote_package() { #[test] #[cfg(unix)] fn already_installed_multiple_versions() -> Result<()> { - use crate::common::copy_dir_all; + fn prepare(context: &TestContext) -> Result<()> { + use crate::common::copy_dir_all; - // Install a version - let context1 = TestContext::new("3.12"); - uv_snapshot!(context1.filters(), context1.install() - .arg("anyio==3.7.0"), @r###" - success: true - exit_code: 0 - ----- stdout ----- + // Install into the base environment + context.install().arg("anyio==3.7.0").assert().success(); - ----- stderr ----- - Resolved 3 packages in [TIME] - Downloaded 3 packages in [TIME] - Installed 3 packages in [TIME] - + anyio==3.7.0 - + idna==3.6 - + sniffio==1.3.1 - "### - ); + // Install another version into another environment + let context_duplicate = TestContext::new("3.12"); + context_duplicate + .install() + .arg("anyio==4.0.0") + .assert() + .success(); - // Install another version - let context2 = TestContext::new("3.12"); - uv_snapshot!(context2.filters(), context2.install() - .arg("anyio==4.0.0"), @r###" - success: true - exit_code: 0 - ----- stdout ----- + // Copy the second version into the first environment + copy_dir_all( + context_duplicate + .site_packages() + .join("anyio-4.0.0.dist-info"), + context.site_packages().join("anyio-4.0.0.dist-info"), + )?; - ----- stderr ----- - Resolved 3 packages in [TIME] - Downloaded 3 packages in [TIME] - Installed 3 packages in [TIME] - + anyio==4.0.0 - + idna==3.6 - + sniffio==1.3.1 - "### - ); + Ok(()) + } - // Copy the second version into the first environment - copy_dir_all( - context2.site_packages().join("anyio-4.0.0.dist-info"), - context1.site_packages().join("anyio-4.0.0.dist-info"), - )?; + let context = TestContext::new("3.12"); + + prepare(&context)?; // Request the second anyio version again // Should remove both previous versions and reinstall the second one - uv_snapshot!(context1.filters(), context1.install().arg("anyio==4.0.0"), @r###" + uv_snapshot!(context.filters(), context.install().arg("anyio==4.0.0"), @r###" success: true exit_code: 0 ----- stdout ----- @@ -3578,6 +3563,26 @@ fn already_installed_multiple_versions() -> Result<()> { "### ); + // Reset the test context + prepare(&context)?; + + // Request the anyio without a version specifier + // This is loosely a regression test for the ordering of the installation preferences + // from existing site-packages + uv_snapshot!(context.filters(), context.install().arg("anyio"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 3 packages in [TIME] + Installed 1 package in [TIME] + - anyio==3.7.0 + - anyio==4.0.0 + + anyio==4.0.0 + "### + ); + Ok(()) }