From f897ee3f88796704e1e7c79b0e81c26578698c3c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 16 Feb 2024 20:33:40 -0500 Subject: [PATCH] Allow repeated dependencies when installing (#1558) ## Summary It turns out that it's not uncommon to end up with repeated packages in requirements files when running `pip-sync`, e.g., you might have `anyio==4.0.0` specified multiple times. This PR relaxes our assertions in the install plan to allow such repeated packages, as long as the requirement markers are exactly the same (i.e., they are truly duplicates). Closes https://github.com/astral-sh/uv/issues/1552. --- crates/distribution-types/src/lib.rs | 2 +- crates/uv-installer/src/plan.rs | 61 +++++++++++++++++++-------- crates/uv/src/commands/pip_install.rs | 2 +- crates/uv/src/commands/pip_sync.rs | 2 +- crates/uv/tests/pip_sync.rs | 49 ++++++++++++++++++++- 5 files changed, 94 insertions(+), 22 deletions(-) diff --git a/crates/distribution-types/src/lib.rs b/crates/distribution-types/src/lib.rs index 4e5075963..5c291741e 100644 --- a/crates/distribution-types/src/lib.rs +++ b/crates/distribution-types/src/lib.rs @@ -96,7 +96,7 @@ impl std::fmt::Display for VersionOrUrl<'_> { } } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum InstalledVersion<'a> { /// A PEP 440 version specifier, used to identify a distribution in a registry. Version(&'a Version), diff --git a/crates/uv-installer/src/plan.rs b/crates/uv-installer/src/plan.rs index 464f61d58..cb5169171 100644 --- a/crates/uv-installer/src/plan.rs +++ b/crates/uv-installer/src/plan.rs @@ -1,14 +1,15 @@ +use std::collections::hash_map::Entry; use std::hash::BuildHasherDefault; use std::io; use std::path::Path; use anyhow::{bail, Result}; -use rustc_hash::FxHashSet; +use rustc_hash::FxHashMap; use tracing::{debug, warn}; use distribution_types::{ BuiltDist, CachedDirectUrlDist, CachedDist, Dist, IndexLocations, InstalledDirectUrlDist, - InstalledDist, Name, SourceDist, + InstalledDist, InstalledMetadata, InstalledVersion, Name, SourceDist, }; use pep508_rs::{Requirement, VersionOrUrl}; use platform_tags::Tags; @@ -25,7 +26,7 @@ use crate::{ResolvedEditable, SitePackages}; #[derive(Debug)] pub struct Planner<'a> { requirements: &'a [Requirement], - editable_requirements: Vec, + editable_requirements: &'a [ResolvedEditable], } impl<'a> Planner<'a> { @@ -34,13 +35,13 @@ impl<'a> Planner<'a> { pub fn with_requirements(requirements: &'a [Requirement]) -> Self { Self { requirements, - editable_requirements: Vec::new(), + editable_requirements: &[], } } /// Set the editable requirements use in the [`Plan`]. #[must_use] - pub fn with_editable_requirements(self, editable_requirements: Vec) -> Self { + pub fn with_editable_requirements(self, editable_requirements: &'a [ResolvedEditable]) -> Self { Self { editable_requirements, ..self @@ -72,7 +73,7 @@ impl<'a> Planner<'a> { let mut remote = vec![]; let mut reinstalls = vec![]; let mut extraneous = vec![]; - let mut seen = FxHashSet::with_capacity_and_hasher( + let mut seen = FxHashMap::with_capacity_and_hasher( self.requirements.len(), BuildHasherDefault::default(), ); @@ -80,11 +81,20 @@ impl<'a> Planner<'a> { // Remove any editable requirements. for requirement in self.editable_requirements { // If we see the same requirement twice, then we have a conflict. - if !seen.insert(requirement.name().clone()) { - bail!( - "Detected duplicate package in requirements: {}", - requirement.name() - ); + let specifier = Specifier::Editable(requirement.installed_version()); + match seen.entry(requirement.name().clone()) { + Entry::Occupied(value) => { + if value.get() == &specifier { + continue; + } + bail!( + "Detected duplicate package in requirements: {}", + requirement.name() + ); + } + Entry::Vacant(entry) => { + entry.insert(specifier); + } } match requirement { @@ -111,7 +121,7 @@ impl<'a> Planner<'a> { // Remove any non-editable installs of the same package. reinstalls.push(dist); } - local.push(built.wheel); + local.push(built.wheel.clone()); } } } @@ -123,11 +133,20 @@ impl<'a> Planner<'a> { } // If we see the same requirement twice, then we have a conflict. - if !seen.insert(requirement.name.clone()) { - bail!( - "Detected duplicate package in requirements: {}", - requirement.name - ); + let specifier = Specifier::NonEditable(requirement.version_or_url.as_ref()); + match seen.entry(requirement.name.clone()) { + Entry::Occupied(value) => { + if value.get() == &specifier { + continue; + } + bail!( + "Detected duplicate package in requirements: {}", + requirement.name + ); + } + Entry::Vacant(entry) => { + entry.insert(specifier); + } } // Check if the package should be reinstalled. A reinstall involves (1) purging any @@ -382,6 +401,14 @@ impl<'a> Planner<'a> { } } +#[derive(Debug, PartialEq, Eq)] +enum Specifier<'a> { + /// An editable requirement, marked by the installed version of the package. + Editable(InstalledVersion<'a>), + /// A non-editable requirement, marked by the version or URL specifier. + NonEditable(Option<&'a VersionOrUrl>), +} + /// Returns `true` if the cache entry linked to the file at the given [`Path`] is not-modified. /// /// A cache entry is not modified if it exists and is newer than the file at the given path. diff --git a/crates/uv/src/commands/pip_install.rs b/crates/uv/src/commands/pip_install.rs index 54c68e5d4..6198a2465 100644 --- a/crates/uv/src/commands/pip_install.rs +++ b/crates/uv/src/commands/pip_install.rs @@ -506,7 +506,7 @@ async fn install( reinstalls, extraneous: _, } = Planner::with_requirements(&requirements) - .with_editable_requirements(editables) + .with_editable_requirements(&editables) .build( site_packages, reinstall, diff --git a/crates/uv/src/commands/pip_sync.rs b/crates/uv/src/commands/pip_sync.rs index ed6272fdc..9aea1c4ad 100644 --- a/crates/uv/src/commands/pip_sync.rs +++ b/crates/uv/src/commands/pip_sync.rs @@ -142,7 +142,7 @@ pub(crate) async fn pip_sync( reinstalls, extraneous, } = Planner::with_requirements(&requirements) - .with_editable_requirements(resolved_editables.editables) + .with_editable_requirements(&resolved_editables.editables) .build( site_packages, reinstall, diff --git a/crates/uv/tests/pip_sync.rs b/crates/uv/tests/pip_sync.rs index 9eda54d5a..f363e633d 100644 --- a/crates/uv/tests/pip_sync.rs +++ b/crates/uv/tests/pip_sync.rs @@ -2247,7 +2247,7 @@ fn sync_editable() -> Result<()> { "../../scripts/editable-installs/maturin_editable/python/maturin_editable/__init__.py"; let python_version_1 = indoc::indoc! {r" from .maturin_editable import * - + version = 1 "}; fs_err::write(python_source_file, python_version_1)?; @@ -2263,7 +2263,7 @@ fn sync_editable() -> Result<()> { // Edit the sources. let python_version_2 = indoc::indoc! {r" from .maturin_editable import * - + version = 2 "}; fs_err::write(python_source_file, python_version_2)?; @@ -2664,3 +2664,48 @@ fn offline() -> Result<()> { Ok(()) } + +/// Sync with a repeated `anyio` requirement. The second requirement should be ignored. +#[test] +fn repeat_requirement() -> Result<()> { + let context = TestContext::new("3.12"); + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str("anyio\nanyio")?; + + uv_snapshot!(command(&context) + .arg("requirements.in"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 1 package in [TIME] + Downloaded 1 package in [TIME] + Installed 1 package in [TIME] + + anyio==4.2.0 + "###); + + Ok(()) +} + +/// Sync with a repeated, but conflicting `anyio` requirement. The second requirement should cause +/// an error. +#[test] +fn conflicting_requirement() -> Result<()> { + let context = TestContext::new("3.12"); + let requirements_in = context.temp_dir.child("requirements.in"); + requirements_in.write_str("anyio\nanyio==4.0.0")?; + + uv_snapshot!(command(&context) + .arg("requirements.in"), @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to determine installation plan + Caused by: Detected duplicate package in requirements: anyio + "###); + + Ok(()) +}