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

Avoid downgrading packages when --upgrade is provided #10097

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
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)
}
}
100 changes: 93 additions & 7 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 @@ -5242,14 +5329,13 @@ fn already_installed_local_path_dependent() {
.arg("--no-index")
.arg("--find-links")
.arg(build_vendor_links_url()), @r###"
success: false
exit_code: 1
success: true
exit_code: 0
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because first-local was not found in the provided package locations and second-local==0.1.0 depends on first-local, we can conclude that second-local==0.1.0 cannot be used.
And because only second-local==0.1.0 is available and you require second-local, we can conclude that your requirements are unsatisfiable.
Resolved 2 packages in [TIME]
Audited 2 packages in [TIME]
"###
);

Expand Down Expand Up @@ -5408,8 +5494,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
Loading