-
Notifications
You must be signed in to change notification settings - Fork 790
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
Improve interactions with existing Python executables during install #8733
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ use futures::StreamExt; | |
use itertools::{Either, Itertools}; | ||
use owo_colors::OwoColorize; | ||
use rustc_hash::{FxHashMap, FxHashSet}; | ||
use same_file::is_same_file; | ||
use tracing::{debug, trace}; | ||
|
||
use uv_client::Connectivity; | ||
|
@@ -20,6 +19,7 @@ use uv_python::managed::{ | |
}; | ||
use uv_python::{PythonDownloads, PythonInstallationKey, PythonRequest, PythonVersionFile}; | ||
use uv_shell::Shell; | ||
use uv_trampoline_builder::{Launcher, LauncherKind}; | ||
use uv_warnings::warn_user; | ||
|
||
use crate::commands::python::{ChangeEvent, ChangeEventKind}; | ||
|
@@ -73,7 +73,6 @@ struct Changelog { | |
installed: FxHashSet<PythonInstallationKey>, | ||
uninstalled: FxHashSet<PythonInstallationKey>, | ||
installed_executables: FxHashMap<PythonInstallationKey, Vec<PathBuf>>, | ||
uninstalled_executables: FxHashSet<PathBuf>, | ||
} | ||
|
||
impl Changelog { | ||
|
@@ -104,10 +103,12 @@ impl Changelog { | |
} | ||
|
||
/// Download and install Python versions. | ||
#[allow(clippy::fn_params_excessive_bools)] | ||
pub(crate) async fn install( | ||
project_dir: &Path, | ||
targets: Vec<String>, | ||
reinstall: bool, | ||
force: bool, | ||
python_downloads: PythonDownloads, | ||
native_tls: bool, | ||
connectivity: Connectivity, | ||
|
@@ -281,7 +282,7 @@ pub(crate) async fn install( | |
Ok(()) => { | ||
debug!( | ||
"Installed executable at {} for {}", | ||
target.user_display(), | ||
target.simplified_display(), | ||
installation.key(), | ||
); | ||
changelog.installed.insert(installation.key().clone()); | ||
|
@@ -291,42 +292,102 @@ pub(crate) async fn install( | |
.or_default() | ||
.push(target.clone()); | ||
} | ||
Err(uv_python::managed::Error::LinkExecutable { from, to, err }) | ||
Err(uv_python::managed::Error::LinkExecutable { from: _, to, err }) | ||
if err.kind() == ErrorKind::AlreadyExists => | ||
{ | ||
// TODO(zanieb): Add `--force` | ||
if reinstall { | ||
fs_err::remove_file(&to)?; | ||
installation.create_bin_link(&target)?; | ||
debug!( | ||
"Updated executable at {} to {}", | ||
target.user_display(), | ||
installation.key(), | ||
); | ||
changelog.installed.insert(installation.key().clone()); | ||
changelog | ||
.installed_executables | ||
.entry(installation.key().clone()) | ||
.or_default() | ||
.push(target.clone()); | ||
changelog.uninstalled_executables.insert(target); | ||
} else { | ||
if !is_same_file(&to, &from).unwrap_or_default() { | ||
errors.push(( | ||
debug!( | ||
"Inspecting existing executable at {}", | ||
target.simplified_display() | ||
); | ||
|
||
// Figure out what installation it references, if any | ||
let existing = find_matching_bin_link(&existing_installations, &target); | ||
|
||
match existing { | ||
None => { | ||
// There's an existing executable we don't manage, require `--force` | ||
if !force { | ||
errors.push(( | ||
installation.key(), | ||
anyhow::anyhow!( | ||
"Executable already exists at `{}` but is not managed by uv; use `--force` to replace it", | ||
to.simplified_display() | ||
), | ||
)); | ||
continue; | ||
} | ||
debug!( | ||
"Replacing existing executable at `{}` due to `--force`", | ||
target.simplified_display() | ||
); | ||
} | ||
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) { | ||
debug!( | ||
"Executable at `{}` is already for `{}`", | ||
target.simplified_display(), | ||
installation.key(), | ||
); | ||
continue; | ||
} | ||
debug!( | ||
"Replacing existing executable for `{}` at `{}`", | ||
installation.key(), | ||
anyhow::anyhow!( | ||
"Executable already exists at `{}`. Use `--reinstall` to force replacement.", | ||
to.user_display() | ||
), | ||
)); | ||
target.simplified_display(), | ||
); | ||
} | ||
Some(existing) => { | ||
// The existing link points to a different installation, check if it | ||
// is reasonable to replace | ||
if force { | ||
debug!( | ||
"Replacing existing executable for `{}` at `{}` with executable for `{}` due to `--force` flag", | ||
existing.key(), | ||
target.simplified_display(), | ||
installation.key(), | ||
); | ||
} else { | ||
if installation.is_upgrade_of(existing) { | ||
debug!( | ||
"Replacing existing executable for `{}` at `{}` with executable for `{}` since it is an upgrade", | ||
existing.key(), | ||
target.simplified_display(), | ||
installation.key(), | ||
); | ||
} else { | ||
debug!( | ||
"Executable already exists at `{}` for `{}`. Use `--force` to replace it.", | ||
existing.key(), | ||
to.simplified_display() | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should any of this be user-facing? I'm unsure... At least this last one seems like it should be, though? Otherwise we risk silent no-ops here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure either. I opted for this to be silent because if I ran |
||
continue; | ||
} | ||
} | ||
} | ||
} | ||
|
||
// Replace the existing link | ||
fs_err::remove_file(&to)?; | ||
installation.create_bin_link(&target)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not do an atomic replace here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm yeah we can probably do an atomic replace as in |
||
debug!( | ||
"Updated executable at `{}` to `{}`", | ||
target.simplified_display(), | ||
installation.key(), | ||
); | ||
changelog.installed.insert(installation.key().clone()); | ||
changelog | ||
.installed_executables | ||
.entry(installation.key().clone()) | ||
.or_default() | ||
.push(target.clone()); | ||
} | ||
Err(err) => return Err(err.into()), | ||
} | ||
} | ||
|
||
if changelog.installed.is_empty() { | ||
if changelog.installed.is_empty() && errors.is_empty() { | ||
if is_default_install { | ||
writeln!( | ||
printer.stderr(), | ||
|
@@ -483,3 +544,32 @@ fn warn_if_not_on_path(bin: &Path) { | |
} | ||
} | ||
} | ||
|
||
/// Find the [`ManagedPythonInstallation`] corresponding to an executable link installed at the | ||
/// given path, if any. | ||
/// | ||
/// 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], | ||
path: &Path, | ||
) -> Option<&'a ManagedPythonInstallation> { | ||
let target = if cfg!(unix) { | ||
if !path.is_symlink() { | ||
return None; | ||
} | ||
path.read_link().ok()? | ||
} else if cfg!(windows) { | ||
let launcher = Launcher::try_from_path(path).ok()??; | ||
if !matches!(launcher.kind, LauncherKind::Python) { | ||
return None; | ||
} | ||
launcher.python_path | ||
} else { | ||
unreachable!("Only Windows and Unix are supported") | ||
}; | ||
|
||
installations | ||
.iter() | ||
.find(|installation| installation.executable() == target) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this resolve symlinks? (Does it need to? Should we use same-file?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but I can't think of a reasonable case where it needs to. I don't think we can use
same-file
because it's a directory.