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

Fix Python executable installation when multiple patch versions are requested #9607

Merged
merged 1 commit into from
Dec 3, 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
35 changes: 28 additions & 7 deletions crates/uv/src/commands/python/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,11 @@ pub(crate) async fn install(
None
};

let installations: Vec<_> = downloaded.iter().chain(satisfied.iter().copied()).collect();

// Ensure that the installations are _complete_ for both downloaded installations and existing
// installations that match the request
for installation in downloaded.iter().chain(satisfied.iter().copied()) {
for installation in &installations {
installation.ensure_externally_managed()?;
installation.ensure_canonical_executables()?;

Expand Down Expand Up @@ -353,7 +355,13 @@ pub(crate) async fn install(
);

// Figure out what installation it references, if any
let existing = find_matching_bin_link(&existing_installations, &target);
let existing = find_matching_bin_link(
installations
.iter()
.copied()
.chain(existing_installations.iter()),
&target,
);

match existing {
None => {
Expand All @@ -373,7 +381,7 @@ pub(crate) async fn install(
target.simplified_display()
);
}
Some(existing) if existing == installation => {
Some(existing) if existing == *installation => {
// The existing link points to the same installation, so we're done unless
// they requested we reinstall
if !(reinstall || force) {
Expand Down Expand Up @@ -429,6 +437,17 @@ pub(crate) async fn install(

// Replace the existing link
fs_err::remove_file(&to)?;

if let Some(existing) = existing {
// Ensure we do not report installation of this executable for an existing
// key if we undo it
changelog
.installed_executables
.entry(existing.key().clone())
.or_default()
.remove(&target);
}

installation.create_bin_link(&target)?;
debug!(
"Updated executable at `{}` to {}",
Expand Down Expand Up @@ -562,6 +581,10 @@ pub(crate) fn format_executables(
return String::new();
};

if installed.is_empty() {
return String::new();
}

let names = installed
.iter()
.filter_map(|path| path.file_name())
Expand Down Expand Up @@ -612,7 +635,7 @@ fn warn_if_not_on_path(bin: &Path) {
/// Like [`ManagedPythonInstallation::is_bin_link`], but this method will only resolve the
/// given path one time.
fn find_matching_bin_link<'a>(
installations: &'a [ManagedPythonInstallation],
mut installations: impl Iterator<Item = &'a ManagedPythonInstallation>,
path: &Path,
) -> Option<&'a ManagedPythonInstallation> {
let target = if cfg!(unix) {
Expand All @@ -630,7 +653,5 @@ fn find_matching_bin_link<'a>(
unreachable!("Only Windows and Unix are supported")
};

installations
.iter()
.find(|installation| installation.executable() == target)
installations.find(|installation| installation.executable() == target)
}
36 changes: 36 additions & 0 deletions crates/uv/tests/it/python_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,42 @@ fn python_install_preview() {

// The executable should be removed
bin_python.assert(predicate::path::missing());

// Install multiple patch versions
uv_snapshot!(context.filters(), context.python_install().arg("--preview").arg("3.12.7").arg("3.12.6"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Installed 2 versions in [TIME]
+ cpython-3.12.6-[PLATFORM]
+ cpython-3.12.7-[PLATFORM] (python3.12)
"###);

let bin_python = context
.temp_dir
.child("bin")
.child(format!("python3.12{}", std::env::consts::EXE_SUFFIX));

// The link should be for the newer patch version
if cfg!(unix) {
insta::with_settings!({
filters => context.filters(),
}, {
insta::assert_snapshot!(
read_link_path(&bin_python), @"[TEMP_DIR]/managed/cpython-3.12.7-[PLATFORM]/bin/python3.12"
);
});
} else {
insta::with_settings!({
filters => context.filters(),
}, {
insta::assert_snapshot!(
read_link_path(&bin_python), @"[TEMP_DIR]/managed/cpython-3.12.7-[PLATFORM]/python"
);
});
}
}

#[test]
Expand Down
Loading