Skip to content

Commit

Permalink
Treat uninstallable packages as warnings, rather than errors (astral-…
Browse files Browse the repository at this point in the history
…sh#2557)

## Summary

Closes astral-sh#2467.
  • Loading branch information
charliermarsh authored Mar 20, 2024
1 parent baa3069 commit cfd18aa
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 20 deletions.
2 changes: 1 addition & 1 deletion crates/uv-installer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pub use editable::{is_dynamic, BuiltEditable, ResolvedEditable};
pub use installer::{Installer, Reporter as InstallReporter};
pub use plan::{Plan, Planner, Reinstall};
pub use site_packages::{Diagnostic, SitePackages};
pub use uninstall::uninstall;
pub use uninstall::{uninstall, UninstallError};
pub use uv_traits::NoBinary;

mod compile;
Expand Down
12 changes: 11 additions & 1 deletion crates/uv-installer/src/uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ use anyhow::Result;
use distribution_types::InstalledDist;

/// Uninstall a package from the specified Python environment.
pub async fn uninstall(dist: &InstalledDist) -> Result<install_wheel_rs::Uninstall> {
pub async fn uninstall(
dist: &InstalledDist,
) -> Result<install_wheel_rs::Uninstall, UninstallError> {
let uninstall = tokio::task::spawn_blocking({
let path = dist.path().to_owned();
move || install_wheel_rs::uninstall_wheel(&path)
Expand All @@ -12,3 +14,11 @@ pub async fn uninstall(dist: &InstalledDist) -> Result<install_wheel_rs::Uninsta

Ok(uninstall)
}

#[derive(thiserror::Error, Debug)]
pub enum UninstallError {
#[error(transparent)]
Uninstall(#[from] install_wheel_rs::Error),
#[error(transparent)]
Join(#[from] tokio::task::JoinError),
}
34 changes: 25 additions & 9 deletions crates/uv/src/commands/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ use uv_resolver::{
ResolutionGraph, ResolutionMode, Resolver,
};
use uv_traits::{BuildIsolation, ConfigSettings, InFlight, NoBuild, SetupPyStrategy};
use uv_warnings::warn_user;

use crate::commands::reporters::{DownloadReporter, InstallReporter, ResolverReporter};
use crate::commands::{compile_bytecode, elapsed, ChangeEvent, ChangeEventKind, ExitStatus};
Expand Down Expand Up @@ -671,15 +672,27 @@ async fn install(
// Remove any existing installations.
if !reinstalls.is_empty() {
for dist_info in &reinstalls {
let summary = uv_installer::uninstall(dist_info).await?;
debug!(
"Uninstalled {} ({} file{}, {} director{})",
dist_info.name(),
summary.file_count,
if summary.file_count == 1 { "" } else { "s" },
summary.dir_count,
if summary.dir_count == 1 { "y" } else { "ies" },
);
match uv_installer::uninstall(dist_info).await {
Ok(summary) => {
debug!(
"Uninstalled {} ({} file{}, {} director{})",
dist_info.name(),
summary.file_count,
if summary.file_count == 1 { "" } else { "s" },
summary.dir_count,
if summary.dir_count == 1 { "y" } else { "ies" },
);
}
Err(uv_installer::UninstallError::Uninstall(
install_wheel_rs::Error::MissingRecord(_),
)) => {
warn_user!(
"Failed to uninstall package at {} due to missing RECORD file. Installation may result in an incomplete environment.",
dist_info.path().simplified_display().cyan(),
);
}
Err(err) => return Err(err.into()),
}
}
}

Expand Down Expand Up @@ -939,6 +952,9 @@ enum Error {
#[error(transparent)]
Resolve(#[from] uv_resolver::ResolveError),

#[error(transparent)]
Uninstall(#[from] uv_installer::UninstallError),

#[error(transparent)]
Client(#[from] uv_client::Error),

Expand Down
31 changes: 22 additions & 9 deletions crates/uv/src/commands/pip_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use uv_installer::{
use uv_interpreter::{Interpreter, PythonEnvironment};
use uv_resolver::InMemoryIndex;
use uv_traits::{BuildIsolation, ConfigSettings, InFlight, NoBuild, SetupPyStrategy};
use uv_warnings::warn_user;

use crate::commands::reporters::{DownloadReporter, FinderReporter, InstallReporter};
use crate::commands::{compile_bytecode, elapsed, ChangeEvent, ChangeEventKind, ExitStatus};
Expand Down Expand Up @@ -286,15 +287,27 @@ pub(crate) async fn pip_sync(
let start = std::time::Instant::now();

for dist_info in extraneous.iter().chain(reinstalls.iter()) {
let summary = uv_installer::uninstall(dist_info).await?;
debug!(
"Uninstalled {} ({} file{}, {} director{})",
dist_info.name(),
summary.file_count,
if summary.file_count == 1 { "" } else { "s" },
summary.dir_count,
if summary.dir_count == 1 { "y" } else { "ies" },
);
match uv_installer::uninstall(dist_info).await {
Ok(summary) => {
debug!(
"Uninstalled {} ({} file{}, {} director{})",
dist_info.name(),
summary.file_count,
if summary.file_count == 1 { "" } else { "s" },
summary.dir_count,
if summary.dir_count == 1 { "y" } else { "ies" },
);
}
Err(uv_installer::UninstallError::Uninstall(
install_wheel_rs::Error::MissingRecord(_),
)) => {
warn_user!(
"Failed to uninstall package at {} due to missing RECORD file. Installation may result in an incomplete environment.",
dist_info.path().simplified_display().cyan(),
);
}
Err(err) => return Err(err.into()),
}
}

let s = if extraneous.len() + reinstalls.len() == 1 {
Expand Down
65 changes: 65 additions & 0 deletions crates/uv/tests/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,71 @@ fn reinstall_extras() -> Result<()> {
Ok(())
}

/// Warn, but don't fail, when uninstalling incomplete packages.
#[test]
#[cfg(unix)]
fn reinstall_incomplete() -> Result<()> {
let context = TestContext::new("3.12");

// Install anyio.
let requirements_txt = context.temp_dir.child("requirements.txt");
requirements_txt.touch()?;
requirements_txt.write_str("anyio==3.7.0")?;

uv_snapshot!(command(&context)
.arg("-r")
.arg("requirements.txt"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Resolved 3 packages in [TIME]
Downloaded 3 packages in [TIME]
Installed 3 packages in [TIME]
+ anyio==3.7.0
+ idna==3.4
+ sniffio==1.3.0
"###
);

// Manually remove the `RECORD` file.
fs_err::remove_file(
context
.venv
.join("lib/python3.12/site-packages/anyio-3.7.0.dist-info/RECORD"),
)?;

// Re-install anyio.
let requirements_txt = context.temp_dir.child("requirements.txt");
requirements_txt.touch()?;
requirements_txt.write_str("anyio==4.0.0")?;

let filters = [(r"Failed to uninstall package at .* due to missing RECORD", "Failed to uninstall package at .venv/lib/python3.12/site-packages/anyio-3.7.0.dist-info due to missing RECORD")]
.into_iter()
.chain(INSTA_FILTERS.to_vec())
.collect::<Vec<_>>();

uv_snapshot!(filters, command(&context)
.arg("-r")
.arg("requirements.txt"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Resolved 3 packages in [TIME]
Downloaded 1 package in [TIME]
warning: Failed to uninstall package at .venv/lib/python3.12/site-packages/anyio-3.7.0.dist-info due to missing RECORD file. Installation may result in an incomplete environment.
Installed 1 package in [TIME]
- anyio==3.7.0
+ anyio==4.0.0
"###
);

Ok(())
}

/// Like `pip`, we (unfortunately) allow incompatible environments.
#[test]
fn allow_incompatibilities() -> Result<()> {
Expand Down

0 comments on commit cfd18aa

Please sign in to comment.