Skip to content

Commit

Permalink
Fix downgrades
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Dec 22, 2024
1 parent 38a38fa commit b14fb9c
Show file tree
Hide file tree
Showing 6 changed files with 175 additions and 56 deletions.
9 changes: 9 additions & 0 deletions crates/uv-configuration/src/package_options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 1 addition & 5 deletions crates/uv-installer/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
5 changes: 5 additions & 0 deletions crates/uv-requirements/src/upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ pub fn read_lock_requirements(
install_path: &Path,
upgrade: &Upgrade,
) -> Result<LockedRequirements, LockError> {
// 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();

Expand Down
77 changes: 60 additions & 17 deletions crates/uv-resolver/src/candidate_selector.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -83,37 +84,78 @@ impl CandidateSelector {
index: Option<&'a IndexUrl>,
env: &ResolverEnvironment,
) -> Option<Candidate<'a>> {
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,
) {
trace!("Using preference {} {}", preferred.name, preferred.version);
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
Expand All @@ -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<Candidate<'a>> {
Expand All @@ -159,7 +201,7 @@ impl CandidateSelector {
range,
version_maps,
installed_packages,
is_excluded,
reinstall,
env,
)
}
Expand All @@ -172,7 +214,7 @@ impl CandidateSelector {
range: &Range<Version>,
version_maps: &'a [VersionMap],
installed_packages: &'a InstalledPackages,
is_excluded: bool,
reinstall: bool,
env: &ResolverEnvironment,
) -> Option<Candidate<'a>> {
for (marker, version) in preferences {
Expand All @@ -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() {
[] => {}
Expand Down
43 changes: 11 additions & 32 deletions crates/uv-resolver/src/exclusions.rs
Original file line number Diff line number Diff line change
@@ -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<PackageName>),
/// 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<PackageName> =
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)
}
}
91 changes: 89 additions & 2 deletions crates/uv/tests/it/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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]
"###
);

Expand Down

0 comments on commit b14fb9c

Please sign in to comment.