Skip to content

Commit

Permalink
Prefer Python executable names that match the request over default na…
Browse files Browse the repository at this point in the history
…mes (#9066)

This restores behavior previously removed in
#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 #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
`[email protected]`, 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.
  • Loading branch information
zanieb authored Nov 13, 2024
1 parent 15ef807 commit 2966471
Show file tree
Hide file tree
Showing 5 changed files with 284 additions and 157 deletions.
124 changes: 106 additions & 18 deletions crates/uv-python/src/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1600,20 +1600,102 @@ impl EnvironmentPreference {
}
}

#[derive(Debug, Clone, Copy)]
#[derive(Debug, Clone, Default, Copy, PartialEq, Eq)]
pub(crate) struct ExecutableName {
name: &'static str,
implementation: Option<ImplementationName>,
major: Option<u8>,
minor: Option<u8>,
patch: Option<u8>,
prerelease: Option<Prerelease>,
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<std::cmp::Ordering> {
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
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -1764,6 +1849,9 @@ impl VersionRequest {
}
}

names.sort_unstable_by_key(|name| name.into_comparator(self, implementation));
names.reverse();

names
}

Expand Down
33 changes: 18 additions & 15 deletions crates/uv-python/src/discovery/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
"[email protected]",
&[
"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"],
);
}
4 changes: 4 additions & 0 deletions crates/uv-python/src/implementation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ impl ImplementationName {
["cpython", "pypy", "graalpy"].into_iter()
}

pub(crate) fn iter_all() -> impl Iterator<Item = Self> {
[Self::CPython, Self::PyPy, Self::GraalPy].into_iter()
}

pub fn pretty(self) -> &'static str {
match self {
Self::CPython => "CPython",
Expand Down
Loading

0 comments on commit 2966471

Please sign in to comment.