From b14fb9ca0a754f3866447fc646ffd5adcb5d0c6b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 22 Dec 2024 09:58:16 -0500 Subject: [PATCH] Fix downgrades --- .../uv-configuration/src/package_options.rs | 9 ++ crates/uv-installer/src/plan.rs | 6 +- crates/uv-requirements/src/upgrade.rs | 5 + crates/uv-resolver/src/candidate_selector.rs | 77 ++++++++++++---- crates/uv-resolver/src/exclusions.rs | 43 +++------ crates/uv/tests/it/pip_install.rs | 91 ++++++++++++++++++- 6 files changed, 175 insertions(+), 56 deletions(-) diff --git a/crates/uv-configuration/src/package_options.rs b/crates/uv-configuration/src/package_options.rs index 5215425811ef..ac39383862b6 100644 --- a/crates/uv-configuration/src/package_options.rs +++ b/crates/uv-configuration/src/package_options.rs @@ -47,6 +47,15 @@ impl Reinstall { matches!(self, Self::All) } + /// Returns `true` if the specified package should be reinstalled. + pub fn contains(&self, package_name: &PackageName) -> bool { + match &self { + Self::None => false, + Self::All => true, + Self::Packages(packages) => packages.contains(package_name), + } + } + /// Combine a set of [`Reinstall`] values. #[must_use] pub fn combine(self, other: Self) -> Self { diff --git a/crates/uv-installer/src/plan.rs b/crates/uv-installer/src/plan.rs index 0ef3c08a86a2..d26af88b6b66 100644 --- a/crates/uv-installer/src/plan.rs +++ b/crates/uv-installer/src/plan.rs @@ -66,11 +66,7 @@ impl<'a> Planner<'a> { for dist in self.resolution.distributions() { // Check if the package should be reinstalled. - let reinstall = match reinstall { - Reinstall::None => false, - Reinstall::All => true, - Reinstall::Packages(packages) => packages.contains(dist.name()), - }; + let reinstall = reinstall.contains(dist.name()); // Check if installation of a binary version of the package should be allowed. let no_binary = build_options.no_binary_package(dist.name()); diff --git a/crates/uv-requirements/src/upgrade.rs b/crates/uv-requirements/src/upgrade.rs index 8a472b373b94..d416caf5a866 100644 --- a/crates/uv-requirements/src/upgrade.rs +++ b/crates/uv-requirements/src/upgrade.rs @@ -68,6 +68,11 @@ pub fn read_lock_requirements( install_path: &Path, upgrade: &Upgrade, ) -> Result { + // As an optimization, skip iterating over the lockfile is we're upgrading all packages anyway. + if upgrade.is_all() { + return Ok(LockedRequirements::default()); + } + let mut preferences = Vec::new(); let mut git = Vec::new(); diff --git a/crates/uv-resolver/src/candidate_selector.rs b/crates/uv-resolver/src/candidate_selector.rs index 7f2db96a6c6f..3104b38a73f9 100644 --- a/crates/uv-resolver/src/candidate_selector.rs +++ b/crates/uv-resolver/src/candidate_selector.rs @@ -1,6 +1,7 @@ +use std::fmt::{Display, Formatter}; + use itertools::Itertools; use pubgrub::Range; -use std::fmt::{Display, Formatter}; use tracing::{debug, trace}; use uv_configuration::IndexStrategy; @@ -83,17 +84,25 @@ impl CandidateSelector { index: Option<&'a IndexUrl>, env: &ResolverEnvironment, ) -> Option> { - let is_excluded = exclusions.contains(package_name); - - // Check for a preference from a lockfile or a previous fork that satisfies the range and - // is allowed. + let reinstall = exclusions.reinstall(package_name); + let upgrade = exclusions.upgrade(package_name); + + // If we have a preference (e.g., from a lockfile), search for a version matching that + // preference. + // + // If `--reinstall` is provided, we should omit any already-installed packages from here, + // since we can't reinstall already-installed packages. + // + // If `--upgrade` is provided, we should still search for a matching preference. In + // practice, preferences should be empty if `--upgrade` is provided, but it's the caller's + // responsibility to ensure that. if let Some(preferred) = self.get_preferred( package_name, range, version_maps, preferences, installed_packages, - is_excluded, + reinstall, index, env, ) { @@ -101,19 +110,52 @@ impl CandidateSelector { return Some(preferred); } - // Check for a locally installed distribution that satisfies the range and is allowed. - if !is_excluded { - if let Some(installed) = Self::get_installed(package_name, range, installed_packages) { + // If we don't have a preference, find an already-installed distribution that satisfies the + // range. + let installed = if reinstall { + None + } else { + Self::get_installed(package_name, range, installed_packages) + }; + + // If we're not upgrading, we should prefer the already-installed distribution. + if !upgrade { + if let Some(installed) = installed { + trace!( + "Using installed {} {} that satisfies {range}", + installed.name, + installed.version + ); + return Some(installed); + } + } + + // Otherwise, find the best candidate from the version maps. + let compatible = self.select_no_preference(package_name, range, version_maps, env); + + // Cross-reference against the already-installed distribution. + // + // If the already-installed version is _more_ compatible than the best candidate + // from the version maps, use the installed version. + if let Some(installed) = installed { + if compatible.as_ref().is_none_or(|compatible| { + let highest = self.use_highest_version(package_name, env); + if highest { + installed.version() >= compatible.version() + } else { + installed.version() <= compatible.version() + } + }) { trace!( - "Using preference {} {} from installed package", + "Using installed {} {} that satisfies {range}", installed.name, - installed.version, + installed.version ); return Some(installed); } } - self.select_no_preference(package_name, range, version_maps, env) + compatible } /// If the package has a preference, an existing version from an existing lockfile or a version @@ -132,7 +174,7 @@ impl CandidateSelector { version_maps: &'a [VersionMap], preferences: &'a Preferences, installed_packages: &'a InstalledPackages, - is_excluded: bool, + reinstall: bool, index: Option<&'a IndexUrl>, env: &ResolverEnvironment, ) -> Option> { @@ -159,7 +201,7 @@ impl CandidateSelector { range, version_maps, installed_packages, - is_excluded, + reinstall, env, ) } @@ -172,7 +214,7 @@ impl CandidateSelector { range: &Range, version_maps: &'a [VersionMap], installed_packages: &'a InstalledPackages, - is_excluded: bool, + reinstall: bool, env: &ResolverEnvironment, ) -> Option> { for (marker, version) in preferences { @@ -181,8 +223,9 @@ impl CandidateSelector { continue; } - // Check for a locally installed distribution that matches the preferred version. - if !is_excluded { + // Check for a locally installed distribution that matches the preferred version, unless + // we have to reinstall, in which case we can't reuse an already-installed distribution. + if !reinstall { let installed_dists = installed_packages.get_packages(package_name); match installed_dists.as_slice() { [] => {} diff --git a/crates/uv-resolver/src/exclusions.rs b/crates/uv-resolver/src/exclusions.rs index 2222584c9216..24cb7da4298a 100644 --- a/crates/uv-resolver/src/exclusions.rs +++ b/crates/uv-resolver/src/exclusions.rs @@ -1,48 +1,27 @@ -use rustc_hash::FxHashSet; use uv_configuration::{Reinstall, Upgrade}; use uv_pep508::PackageName; /// Tracks locally installed packages that should not be selected during resolution. #[derive(Debug, Default, Clone)] -pub enum Exclusions { - #[default] - None, - /// Exclude some local packages from consideration, e.g. from `--reinstall-package foo --upgrade-package bar` - Some(FxHashSet), - /// Exclude all local packages from consideration, e.g. from `--reinstall` or `--upgrade` - All, +pub struct Exclusions { + reinstall: Reinstall, + upgrade: Upgrade, } impl Exclusions { pub fn new(reinstall: Reinstall, upgrade: Upgrade) -> Self { - if upgrade.is_all() || reinstall.is_all() { - Self::All - } else { - let mut exclusions: FxHashSet = - if let Reinstall::Packages(packages) = reinstall { - FxHashSet::from_iter(packages) - } else { - FxHashSet::default() - }; + Self { reinstall, upgrade } + } - if let Upgrade::Packages(packages) = upgrade { - exclusions.extend(packages.into_keys()); - }; + pub fn reinstall(&self, package: &PackageName) -> bool { + self.reinstall.contains(package) + } - if exclusions.is_empty() { - Self::None - } else { - Self::Some(exclusions) - } - } + pub fn upgrade(&self, package: &PackageName) -> bool { + self.upgrade.contains(package) } - /// Returns true if the package is excluded and a local distribution should not be used. pub fn contains(&self, package: &PackageName) -> bool { - match self { - Self::None => false, - Self::Some(packages) => packages.contains(package), - Self::All => true, - } + self.reinstall(package) || self.upgrade(package) } } diff --git a/crates/uv/tests/it/pip_install.rs b/crates/uv/tests/it/pip_install.rs index ada920f05f9f..5d11d5db1c73 100644 --- a/crates/uv/tests/it/pip_install.rs +++ b/crates/uv/tests/it/pip_install.rs @@ -2617,6 +2617,93 @@ fn no_deps_editable() { context.assert_command("import aiohttp").failure(); } +/// Avoid downgrading already-installed packages when `--upgrade` is provided. +#[test] +fn install_no_downgrade() -> Result<()> { + let context = TestContext::new("3.12"); + + // Create a local package named `idna`. + let idna = context.temp_dir.child("idna"); + idna.child("pyproject.toml").write_str(indoc! {r#" + [project] + name = "idna" + version = "1000" + requires-python = ">=3.12" + dependencies = [] + + [build-system] + requires = ["setuptools>=42"] + build-backend = "setuptools.build_meta" + "#})?; + + // Install the local `idna`. + uv_snapshot!(context.filters(), context.pip_install() + .arg("./idna"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 1 package in [TIME] + Prepared 1 package in [TIME] + Installed 1 package in [TIME] + + idna==1000 (from file://[TEMP_DIR]/idna) + "### + ); + + // Install `anyio`, which depends on `idna`. + uv_snapshot!(context.filters(), context.pip_install() + .arg("anyio"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 3 packages in [TIME] + Prepared 2 packages in [TIME] + Installed 2 packages in [TIME] + + anyio==4.3.0 + + sniffio==1.3.1 + "### + ); + + // Install `anyio` with `--upgrade`, which should retain the local `idna`. + uv_snapshot!(context.filters(), context.pip_install() + .arg("-U") + .arg("anyio"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 3 packages in [TIME] + Audited 3 packages in [TIME] + "### + ); + + // Install `anyio` with `--reinstall`, which should downgrade `idna`. + uv_snapshot!(context.filters(), context.pip_install() + .arg("--reinstall") + .arg("anyio"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 3 packages in [TIME] + Prepared 3 packages in [TIME] + Uninstalled 3 packages in [TIME] + Installed 3 packages in [TIME] + ~ anyio==4.3.0 + - idna==1000 (from file://[TEMP_DIR]/idna) + + idna==3.6 + ~ sniffio==1.3.1 + "### + ); + + Ok(()) +} + /// Upgrade a package. #[test] fn install_upgrade() { @@ -5408,8 +5495,8 @@ fn already_installed_local_version_of_remote_package() { ----- stdout ----- ----- stderr ----- - Resolved 3 packages in [TIME] - Audited 3 packages in [TIME] + Resolved 1 package in [TIME] + Audited 1 package in [TIME] "### );