From 2966471db2dfafa71230cdedfa7f0cd124de02b7 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Wed, 13 Nov 2024 10:00:23 -0600 Subject: [PATCH] Prefer Python executable names that match the request over default names (#9066) This restores behavior previously removed in https://github.com/astral-sh/uv/pull/7649. I thought it'd be clearer (and simpler) to have a consistent Python executable name ordering. However, we've seen some cases where this can be surprising and, in combination with #8481, can result in incorrect behavior. For example, see https://github.com/astral-sh/uv/issues/9046 where we prefer `python3` over `python3.12` in the same directory even though `python3.12` was requested. While `python3` and `python3.12` both point to valid Python 3.12 environments there, the expectation is that when `python3.12` is requested that the `python3.12` executable is preferred. This expectation may be less obvious if the user requests `python@3.12`, but uv does not distinguish between these request forms. Similarly, this may be surprising as by default uv prefers `python` over `python3` but when requesting `python3.12` the preference will be swapped. --- crates/uv-python/src/discovery.rs | 124 ++++++++++++-- crates/uv-python/src/discovery/tests.rs | 33 ++-- crates/uv-python/src/implementation.rs | 4 + crates/uv-python/src/tests.rs | 208 +++++++++++++++--------- crates/uv/tests/it/python_pin.rs | 72 +++----- 5 files changed, 284 insertions(+), 157 deletions(-) diff --git a/crates/uv-python/src/discovery.rs b/crates/uv-python/src/discovery.rs index 2195b18bd980..00b015c887b8 100644 --- a/crates/uv-python/src/discovery.rs +++ b/crates/uv-python/src/discovery.rs @@ -1600,9 +1600,9 @@ impl EnvironmentPreference { } } -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Default, Copy, PartialEq, Eq)] pub(crate) struct ExecutableName { - name: &'static str, + implementation: Option, major: Option, minor: Option, patch: Option, @@ -1610,10 +1610,92 @@ pub(crate) struct ExecutableName { variant: PythonVariant, } +#[derive(Debug, Clone, PartialEq, Eq)] +struct ExecutableNameComparator<'a> { + name: ExecutableName, + request: &'a VersionRequest, + implementation: Option<&'a ImplementationName>, +} + +impl Ord for ExecutableNameComparator<'_> { + /// Note the comparison returns a reverse priority ordering. + /// + /// Higher priority items are "Greater" than lower priority items. + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + // Prefer the default name over a specific implementation, unless an implementation was + // requested + let name_ordering = if self.implementation.is_some() { + std::cmp::Ordering::Greater + } else { + std::cmp::Ordering::Less + }; + if self.name.implementation.is_none() && other.name.implementation.is_some() { + return name_ordering.reverse(); + } + if self.name.implementation.is_some() && other.name.implementation.is_none() { + return name_ordering; + } + // Otherwise, use the names in supported order + let ordering = self.name.implementation.cmp(&other.name.implementation); + if ordering != std::cmp::Ordering::Equal { + return ordering; + } + let ordering = self.name.major.cmp(&other.name.major); + let is_default_request = + matches!(self.request, VersionRequest::Any | VersionRequest::Default); + if ordering != std::cmp::Ordering::Equal { + return if is_default_request { + ordering.reverse() + } else { + ordering + }; + } + let ordering = self.name.minor.cmp(&other.name.minor); + if ordering != std::cmp::Ordering::Equal { + return if is_default_request { + ordering.reverse() + } else { + ordering + }; + } + let ordering = self.name.patch.cmp(&other.name.patch); + if ordering != std::cmp::Ordering::Equal { + return if is_default_request { + ordering.reverse() + } else { + ordering + }; + } + let ordering = self.name.prerelease.cmp(&other.name.prerelease); + if ordering != std::cmp::Ordering::Equal { + return if is_default_request { + ordering.reverse() + } else { + ordering + }; + } + let ordering = self.name.variant.cmp(&other.name.variant); + if ordering != std::cmp::Ordering::Equal { + return if is_default_request { + ordering.reverse() + } else { + ordering + }; + } + ordering + } +} + +impl PartialOrd for ExecutableNameComparator<'_> { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + impl ExecutableName { #[must_use] - fn with_name(mut self, name: &'static str) -> Self { - self.name = name; + fn with_implementation(mut self, implementation: ImplementationName) -> Self { + self.implementation = Some(implementation); self } @@ -1646,24 +1728,27 @@ impl ExecutableName { self.variant = variant; self } -} -impl Default for ExecutableName { - fn default() -> Self { - Self { - name: "python", - major: None, - minor: None, - patch: None, - prerelease: None, - variant: PythonVariant::Default, + fn into_comparator<'a>( + self, + request: &'a VersionRequest, + implementation: Option<&'a ImplementationName>, + ) -> ExecutableNameComparator<'a> { + ExecutableNameComparator { + name: self, + request, + implementation, } } } impl std::fmt::Display for ExecutableName { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.name)?; + if let Some(implementation) = self.implementation { + write!(f, "{implementation}")?; + } else { + f.write_str("python")?; + } if let Some(major) = self.major { write!(f, "{major}")?; if let Some(minor) = self.minor { @@ -1741,15 +1826,15 @@ impl VersionRequest { // Add all the implementation-specific names if let Some(implementation) = implementation { for i in 0..names.len() { - let name = names[i].with_name(implementation.into()); + let name = names[i].with_implementation(*implementation); names.push(name); } } else { // When looking for all implementations, include all possible names if matches!(self, Self::Any) { for i in 0..names.len() { - for implementation in ImplementationName::long_names() { - let name = names[i].with_name(implementation); + for implementation in ImplementationName::iter_all() { + let name = names[i].with_implementation(implementation); names.push(name); } } @@ -1764,6 +1849,9 @@ impl VersionRequest { } } + names.sort_unstable_by_key(|name| name.into_comparator(self, implementation)); + names.reverse(); + names } diff --git a/crates/uv-python/src/discovery/tests.rs b/crates/uv-python/src/discovery/tests.rs index 9dbd688f367e..97057a3816ad 100644 --- a/crates/uv-python/src/discovery/tests.rs +++ b/crates/uv-python/src/discovery/tests.rs @@ -477,49 +477,52 @@ fn executable_names_from_request() { case( "any", &[ - "python", "python3", "cpython", "pypy", "graalpy", "cpython3", "pypy3", "graalpy3", + "python", "python3", "cpython", "cpython3", "pypy", "pypy3", "graalpy", "graalpy3", ], ); case("default", &["python", "python3"]); - case("3", &["python", "python3"]); + case("3", &["python3", "python"]); - case("4", &["python", "python4"]); + case("4", &["python4", "python"]); - case("3.13", &["python", "python3", "python3.13"]); + case("3.13", &["python3.13", "python3", "python"]); + + case("pypy", &["pypy", "pypy3", "python", "python3"]); case( "pypy@3.10", &[ - "python", - "python3", - "python3.10", - "pypy", - "pypy3", "pypy3.10", + "pypy3", + "pypy", + "python3.10", + "python3", + "python", ], ); case( "3.13t", &[ - "python", - "python3", + "python3.13t", "python3.13", - "pythont", "python3t", - "python3.13t", + "python3", + "pythont", + "python", ], ); + case("3t", &["python3t", "python3", "pythont", "python"]); case( "3.13.2", - &["python", "python3", "python3.13", "python3.13.2"], + &["python3.13.2", "python3.13", "python3", "python"], ); case( "3.13rc2", - &["python", "python3", "python3.13", "python3.13rc2"], + &["python3.13rc2", "python3.13", "python3", "python"], ); } diff --git a/crates/uv-python/src/implementation.rs b/crates/uv-python/src/implementation.rs index 7d405d48deb6..ffc61dac7ffa 100644 --- a/crates/uv-python/src/implementation.rs +++ b/crates/uv-python/src/implementation.rs @@ -33,6 +33,10 @@ impl ImplementationName { ["cpython", "pypy", "graalpy"].into_iter() } + pub(crate) fn iter_all() -> impl Iterator { + [Self::CPython, Self::PyPy, Self::GraalPy].into_iter() + } + pub fn pretty(self) -> &'static str { match self { Self::CPython => "CPython", diff --git a/crates/uv-python/src/tests.rs b/crates/uv-python/src/tests.rs index 2c8a6475f742..df92db2a89f0 100644 --- a/crates/uv-python/src/tests.rs +++ b/crates/uv-python/src/tests.rs @@ -2087,86 +2087,61 @@ fn find_python_graalpy_request_ignores_cpython() -> Result<()> { } #[test] -fn find_python_prefers_generic_executable_over_implementation_name() -> Result<()> { +fn find_python_executable_name_preference() -> Result<()> { let mut context = TestContext::new()?; - - // We prefer `python` executables over `graalpy` executables in the same directory - // if they are both GraalPy TestContext::create_mock_interpreter( - &context.tempdir.join("python"), + &context.tempdir.join("pypy3.10"), &PythonVersion::from_str("3.10.0").unwrap(), - ImplementationName::GraalPy, + ImplementationName::PyPy, true, false, )?; TestContext::create_mock_interpreter( - &context.tempdir.join("graalpy"), + &context.tempdir.join("pypy"), &PythonVersion::from_str("3.10.1").unwrap(), - ImplementationName::GraalPy, + ImplementationName::PyPy, true, false, )?; context.add_to_search_path(context.tempdir.to_path_buf()); - let python = context.run(|| { - find_python_installation( - &PythonRequest::parse("graalpy@3.10"), - EnvironmentPreference::Any, - PythonPreference::OnlySystem, - &context.cache, - ) - })??; + let python = context + .run(|| { + find_python_installation( + &PythonRequest::parse("pypy@3.10"), + EnvironmentPreference::Any, + PythonPreference::OnlySystem, + &context.cache, + ) + }) + .unwrap() + .unwrap(); assert_eq!( python.interpreter().python_full_version().to_string(), "3.10.0", + "We should prefer the versioned one when a version is requested" ); - // And `python` executables earlier in the search path will take precedence - context.reset_search_path(); - context.add_python_interpreters(&[ - (true, ImplementationName::GraalPy, "python", "3.10.2"), - (true, ImplementationName::GraalPy, "graalpy", "3.10.3"), - ])?; - let python = context.run(|| { - find_python_installation( - &PythonRequest::parse("graalpy@3.10"), - EnvironmentPreference::Any, - PythonPreference::OnlySystem, - &context.cache, - ) - })??; - assert_eq!( - python.interpreter().python_full_version().to_string(), - "3.10.2", - ); - - // But `graalpy` executables earlier in the search path will take precedence - context.reset_search_path(); - context.add_python_interpreters(&[ - (true, ImplementationName::GraalPy, "graalpy", "3.10.3"), - (true, ImplementationName::GraalPy, "python", "3.10.2"), - ])?; - let python = context.run(|| { - find_python_installation( - &PythonRequest::parse("graalpy@3.10"), - EnvironmentPreference::Any, - PythonPreference::OnlySystem, - &context.cache, - ) - })??; + let python = context + .run(|| { + find_python_installation( + &PythonRequest::parse("pypy"), + EnvironmentPreference::Any, + PythonPreference::OnlySystem, + &context.cache, + ) + }) + .unwrap() + .unwrap(); assert_eq!( python.interpreter().python_full_version().to_string(), - "3.10.3", + "3.10.1", + "We should prefer the generic one when no version is requested" ); - Ok(()) -} - -#[test] -fn find_python_prefers_generic_executable_over_one_with_version() -> Result<()> { let mut context = TestContext::new()?; TestContext::create_mock_interpreter( - &context.tempdir.join("pypy3.10"), + &context.tempdir.join("python3.10"), &PythonVersion::from_str("3.10.0").unwrap(), ImplementationName::PyPy, true, @@ -2179,51 +2154,126 @@ fn find_python_prefers_generic_executable_over_one_with_version() -> Result<()> true, false, )?; + TestContext::create_mock_interpreter( + &context.tempdir.join("python"), + &PythonVersion::from_str("3.10.2").unwrap(), + ImplementationName::PyPy, + true, + false, + )?; context.add_to_search_path(context.tempdir.to_path_buf()); - let python = context.run(|| { - find_python_installation( - &PythonRequest::parse("pypy@3.10"), - EnvironmentPreference::Any, - PythonPreference::OnlySystem, - &context.cache, - ) - })??; + let python = context + .run(|| { + find_python_installation( + &PythonRequest::parse("pypy@3.10"), + EnvironmentPreference::Any, + PythonPreference::OnlySystem, + &context.cache, + ) + }) + .unwrap() + .unwrap(); assert_eq!( python.interpreter().python_full_version().to_string(), "3.10.1", - "We should prefer the generic executable over one with the version number" + "We should prefer the implementation name over the generic name" + ); + + let python = context + .run(|| { + find_python_installation( + &PythonRequest::parse("default"), + EnvironmentPreference::Any, + PythonPreference::OnlySystem, + &context.cache, + ) + }) + .unwrap() + .unwrap(); + assert_eq!( + python.interpreter().python_full_version().to_string(), + "3.10.2", + "We should prefer the generic name over the implementation name, but not the versioned name" ); + // We prefer `python` executables over `graalpy` executables in the same directory + // if they are both GraalPy let mut context = TestContext::new()?; TestContext::create_mock_interpreter( - &context.tempdir.join("python3.10"), + &context.tempdir.join("python"), &PythonVersion::from_str("3.10.0").unwrap(), - ImplementationName::PyPy, + ImplementationName::GraalPy, true, false, )?; TestContext::create_mock_interpreter( - &context.tempdir.join("pypy"), + &context.tempdir.join("graalpy"), &PythonVersion::from_str("3.10.1").unwrap(), - ImplementationName::PyPy, + ImplementationName::GraalPy, true, false, )?; context.add_to_search_path(context.tempdir.to_path_buf()); - let python = context.run(|| { - find_python_installation( - &PythonRequest::parse("pypy@3.10"), - EnvironmentPreference::Any, - PythonPreference::OnlySystem, - &context.cache, - ) - })??; + let python = context + .run(|| { + find_python_installation( + &PythonRequest::parse("graalpy@3.10"), + EnvironmentPreference::Any, + PythonPreference::OnlySystem, + &context.cache, + ) + }) + .unwrap() + .unwrap(); assert_eq!( python.interpreter().python_full_version().to_string(), - "3.10.0", - "We should prefer the generic name with a version over one the implementation name" + "3.10.1", + ); + + // And `python` executables earlier in the search path will take precedence + context.reset_search_path(); + context.add_python_interpreters(&[ + (true, ImplementationName::GraalPy, "python", "3.10.2"), + (true, ImplementationName::GraalPy, "graalpy", "3.10.3"), + ])?; + let python = context + .run(|| { + find_python_installation( + &PythonRequest::parse("graalpy@3.10"), + EnvironmentPreference::Any, + PythonPreference::OnlySystem, + &context.cache, + ) + }) + .unwrap() + .unwrap(); + assert_eq!( + python.interpreter().python_full_version().to_string(), + "3.10.2", + ); + + // And `graalpy` executables earlier in the search path will take precedence + context.reset_search_path(); + context.add_python_interpreters(&[ + (true, ImplementationName::GraalPy, "graalpy", "3.10.3"), + (true, ImplementationName::GraalPy, "python", "3.10.2"), + ])?; + let python = context + .run(|| { + find_python_installation( + &PythonRequest::parse("graalpy@3.10"), + EnvironmentPreference::Any, + PythonPreference::OnlySystem, + &context.cache, + ) + }) + .unwrap() + .unwrap(); + assert_eq!( + python.interpreter().python_full_version().to_string(), + "3.10.3", ); Ok(()) diff --git a/crates/uv/tests/it/python_pin.rs b/crates/uv/tests/it/python_pin.rs index 2faf19c3e45c..67a5ff255cf3 100644 --- a/crates/uv/tests/it/python_pin.rs +++ b/crates/uv/tests/it/python_pin.rs @@ -456,14 +456,14 @@ fn python_pin_resolve_no_python() { #[test] fn python_pin_resolve() { - let context: TestContext = TestContext::new_with_versions(&["3.11", "3.12"]); + let context: TestContext = TestContext::new_with_versions(&["3.12", "3.13"]); // We pin the first interpreter on the path uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("any"), @r###" success: true exit_code: 0 ----- stdout ----- - Pinned `.python-version` to `[PYTHON-3.11]` + Pinned `.python-version` to `[PYTHON-3.12]` ----- stderr ----- "###); @@ -472,17 +472,15 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.11] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.12]"); }); - // Request Python 3.12 - uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("3.12"), @r###" + // Request Python 3.13 + uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("3.13"), @r###" success: true exit_code: 0 ----- stdout ----- - Updated `.python-version` from `[PYTHON-3.11]` -> `[PYTHON-3.12]` + Updated `.python-version` from `[PYTHON-3.12]` -> `[PYTHON-3.13]` ----- stderr ----- "###); @@ -491,17 +489,15 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.12] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.13]"); }); - // Request Python 3.11 - uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("3.11"), @r###" + // Request Python 3.13 + uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("3.13"), @r###" success: true exit_code: 0 ----- stdout ----- - Updated `.python-version` from `[PYTHON-3.12]` -> `[PYTHON-3.11]` + Pinned `.python-version` to `[PYTHON-3.13]` ----- stderr ----- "###); @@ -510,9 +506,7 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.11] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.13]"); }); // Request CPython @@ -520,7 +514,7 @@ fn python_pin_resolve() { success: true exit_code: 0 ----- stdout ----- - Pinned `.python-version` to `[PYTHON-3.11]` + Updated `.python-version` from `[PYTHON-3.13]` -> `[PYTHON-3.12]` ----- stderr ----- "###); @@ -529,17 +523,15 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.11] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.12]"); }); - // Request CPython 3.12 - uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("cpython@3.12"), @r###" + // Request CPython 3.13 + uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("cpython@3.13"), @r###" success: true exit_code: 0 ----- stdout ----- - Updated `.python-version` from `[PYTHON-3.11]` -> `[PYTHON-3.12]` + Updated `.python-version` from `[PYTHON-3.12]` -> `[PYTHON-3.13]` ----- stderr ----- "###); @@ -548,17 +540,15 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.12] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.13]"); }); - // Request CPython 3.12 via partial key syntax - uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("cpython-3.12"), @r###" + // Request CPython 3.13 via partial key syntax + uv_snapshot!(context.filters(), context.python_pin().arg("--resolved").arg("cpython-3.13"), @r###" success: true exit_code: 0 ----- stdout ----- - Pinned `.python-version` to `[PYTHON-3.12]` + Pinned `.python-version` to `[PYTHON-3.13]` ----- stderr ----- "###); @@ -567,22 +557,20 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.12] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.13]"); }); - // Request CPython 3.12 for the current platform + // Request CPython 3.13 for the current platform let os = Os::from_env(); let arch = Arch::from_env(); uv_snapshot!(context.filters(), context.python_pin().arg("--resolved") - .arg(format!("cpython-3.12-{os}-{arch}")) + .arg(format!("cpython-3.13-{os}-{arch}")) , @r###" success: true exit_code: 0 ----- stdout ----- - Pinned `.python-version` to `[PYTHON-3.12]` + Pinned `.python-version` to `[PYTHON-3.13]` ----- stderr ----- "###); @@ -591,9 +579,7 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.12] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.13]"); }); // Request an implementation that is not installed @@ -612,9 +598,7 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.12] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.13]"); }); // Request a version that is not installed @@ -633,9 +617,7 @@ fn python_pin_resolve() { insta::with_settings!({ filters => context.filters(), }, { - assert_snapshot!(python_version, @r###" - [PYTHON-3.12] - "###); + assert_snapshot!(python_version, @"[PYTHON-3.13]"); }); }