From 9d514cbbe07a5ca35ab2570fa06d059a41357a31 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 15 Aug 2024 13:19:40 -0400 Subject: [PATCH] Return a structured result from `Lock::satisfies` (#6119) ## Summary Gives the caller control over how messages are reported back to the user. Also merges the index-location validation into the lock, since we're already iterating over the packages. --- crates/uv-resolver/src/lib.rs | 2 +- crates/uv-resolver/src/lock.rs | 96 +++++++++++++++++++--- crates/uv/src/commands/project/lock.rs | 108 +++++++++++++++---------- crates/uv/tests/lock.rs | 1 - 4 files changed, 151 insertions(+), 56 deletions(-) diff --git a/crates/uv-resolver/src/lib.rs b/crates/uv-resolver/src/lib.rs index 00154740e..a21a3ebaa 100644 --- a/crates/uv-resolver/src/lib.rs +++ b/crates/uv-resolver/src/lib.rs @@ -3,7 +3,7 @@ pub use error::{NoSolutionError, NoSolutionHeader, ResolveError}; pub use exclude_newer::ExcludeNewer; pub use exclusions::Exclusions; pub use flat_index::FlatIndex; -pub use lock::{Lock, LockError, ResolverManifest, TreeDisplay}; +pub use lock::{Lock, LockError, ResolverManifest, SatisfiesResult, TreeDisplay}; pub use manifest::Manifest; pub use options::{Options, OptionsBuilder}; pub use preferences::{Preference, PreferenceError, Preferences}; diff --git a/crates/uv-resolver/src/lock.rs b/crates/uv-resolver/src/lock.rs index 4cc2cbdbd..1f4b7ed58 100644 --- a/crates/uv-resolver/src/lock.rs +++ b/crates/uv-resolver/src/lock.rs @@ -17,9 +17,10 @@ use cache_key::RepositoryUrl; use distribution_filename::{DistExtension, ExtensionError, SourceDistExtension, WheelFilename}; use distribution_types::{ BuiltDist, DirectUrlBuiltDist, DirectUrlSourceDist, DirectorySourceDist, Dist, - DistributionMetadata, FileLocation, GitSourceDist, HashPolicy, IndexUrl, Name, PathBuiltDist, - PathSourceDist, RegistryBuiltDist, RegistryBuiltWheel, RegistrySourceDist, RemoteSource, - Resolution, ResolvedDist, ToUrlError, UrlString, + DistributionMetadata, FileLocation, FlatIndexLocation, GitSourceDist, HashPolicy, + IndexLocations, IndexUrl, Name, PathBuiltDist, PathSourceDist, RegistryBuiltDist, + RegistryBuiltWheel, RegistrySourceDist, RemoteSource, Resolution, ResolvedDist, ToUrlError, + UrlString, }; use pep440_rs::Version; use pep508_rs::{MarkerEnvironment, MarkerTree, VerbatimUrl, VerbatimUrlError}; @@ -630,9 +631,10 @@ impl Lock { members: &[PackageName], constraints: &[Requirement], overrides: &[Requirement], + indexes: Option<&IndexLocations>, tags: &Tags, database: &DistributionDatabase<'_, Context>, - ) -> Result { + ) -> Result, LockError> { let mut queue: VecDeque<&Package> = VecDeque::new(); let mut seen = FxHashSet::default(); @@ -645,7 +647,7 @@ impl Lock { "Mismatched members:\n expected: {:?}\n found: {:?}", expected, actual ); - return Ok(false); + return Ok(SatisfiesResult::MismatchedMembers(expected, actual)); } } @@ -668,7 +670,7 @@ impl Lock { "Mismatched constraints:\n expected: {:?}\n found: {:?}", expected, actual ); - return Ok(false); + return Ok(SatisfiesResult::MismatchedConstraints(expected, actual)); } } @@ -691,10 +693,20 @@ impl Lock { "Mismatched overrides:\n expected: {:?}\n found: {:?}", expected, actual ); - return Ok(false); + return Ok(SatisfiesResult::MismatchedOverrides(expected, actual)); } } + // Collect the set of available indexes (both `--index-url` and `--find-links` entries). + let indexes = indexes.map(|locations| { + locations + .indexes() + .map(IndexUrl::redacted) + .chain(locations.flat_index().map(FlatIndexLocation::redacted)) + .map(UrlString::from) + .collect::>() + }); + // Add the workspace packages to the queue. for root_name in workspace.packages().keys() { let root = self @@ -704,7 +716,7 @@ impl Lock { let Some(root) = root else { // The package is not in the lockfile, so it can't be satisfied. debug!("Workspace package `{root_name}` not found in lockfile"); - return Ok(false); + return Ok(SatisfiesResult::MissingRoot(root_name.clone())); }; // Add the base package. @@ -712,6 +724,20 @@ impl Lock { } while let Some(package) = queue.pop_front() { + // If the lockfile references an index that was not provided, we can't validate it. + if let Source::Registry(index) = &package.id.source { + if indexes + .as_ref() + .is_some_and(|indexes| !indexes.contains(index)) + { + return Ok(SatisfiesResult::MissingIndex( + &package.id.name, + &package.id.version, + index, + )); + } + } + // If the package is immutable, we don't need to validate it (or its dependencies). if package.id.source.is_immutable() { continue; @@ -725,7 +751,10 @@ impl Lock { .await else { debug!("Failed to get metadata for: {}", package.id); - return Ok(false); + return Ok(SatisfiesResult::MissingMetadata( + &package.id.name, + &package.id.version, + )); }; // Validate the `requires-dist` metadata. @@ -749,7 +778,12 @@ impl Lock { "Mismatched `requires-dist` for {}:\n expected: {:?}\n found: {:?}", package.id, expected, actual ); - return Ok(false); + return Ok(SatisfiesResult::MismatchedRequiresDist( + &package.id.name, + &package.id.version, + expected, + actual, + )); } } @@ -790,7 +824,12 @@ impl Lock { "Mismatched `requires-dev` for {}:\n expected: {:?}\n found: {:?}", package.id, expected, actual ); - return Ok(false); + return Ok(SatisfiesResult::MismatchedDevDependencies( + &package.id.name, + &package.id.version, + expected, + actual, + )); } } @@ -823,10 +862,43 @@ impl Lock { } } - Ok(true) + Ok(SatisfiesResult::Satisfied) } } +/// The result of checking if a lockfile satisfies a set of requirements. +#[derive(Debug)] +pub enum SatisfiesResult<'lock> { + /// The lockfile satisfies the requirements. + Satisfied, + /// The lockfile uses a different set of workspace members. + MismatchedMembers(BTreeSet, &'lock BTreeSet), + /// The lockfile uses a different set of constraints. + MismatchedConstraints(BTreeSet, BTreeSet), + /// The lockfile uses a different set of overrides. + MismatchedOverrides(BTreeSet, BTreeSet), + /// The lockfile is missing a workspace member. + MissingRoot(PackageName), + /// The lockfile referenced an index that was not provided + MissingIndex(&'lock PackageName, &'lock Version, &'lock UrlString), + /// The resolver failed to generate metadata for a given package. + MissingMetadata(&'lock PackageName, &'lock Version), + /// A package in the lockfile contains different `requires-dist` metadata than expected. + MismatchedRequiresDist( + &'lock PackageName, + &'lock Version, + BTreeSet, + BTreeSet, + ), + /// A package in the lockfile contains different `dev-dependencies` metadata than expected. + MismatchedDevDependencies( + &'lock PackageName, + &'lock Version, + BTreeMap>, + BTreeMap>, + ), +} + /// We discard the lockfile if these options match. #[derive(Clone, Debug, Default, serde::Deserialize, PartialEq, Eq)] #[serde(rename_all = "kebab-case")] diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index 80f71e719..6e7896153 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -8,9 +8,7 @@ use owo_colors::OwoColorize; use rustc_hash::{FxBuildHasher, FxHashMap}; use tracing::debug; -use distribution_types::{ - FlatIndexLocation, IndexLocations, IndexUrl, UnresolvedRequirementSpecification, UrlString, -}; +use distribution_types::{IndexLocations, UnresolvedRequirementSpecification}; use pep440_rs::Version; use pypi_types::Requirement; use uv_auth::store_credentials_from_url; @@ -29,7 +27,7 @@ use uv_requirements::upgrade::{read_lock_requirements, LockedRequirements}; use uv_requirements::NamedRequirementsResolver; use uv_resolver::{ FlatIndex, Lock, Options, OptionsBuilder, PythonRequirement, RequiresPython, ResolverManifest, - ResolverMarkers, + ResolverMarkers, SatisfiesResult, }; use uv_types::{BuildContext, BuildIsolation, EmptyInstalledPackages, HashStrategy}; use uv_warnings::{warn_user, warn_user_once}; @@ -627,54 +625,80 @@ impl ValidatedLock { // However, iIf _no_ indexes were provided, we assume that the user wants to reuse the existing // distributions, even though a failure to reuse the lockfile will result in re-resolving // against PyPI by default. - if !index_locations.is_none() { - // Collect the set of available indexes (both `--index-url` and `--find-links` entries). - let indexes = index_locations - .indexes() - .map(IndexUrl::redacted) - .chain( - index_locations - .flat_index() - .map(FlatIndexLocation::redacted), - ) - .map(UrlString::from) - .collect::>(); - - // Find any packages in the lockfile that reference a registry that is no longer included in - // the current configuration. - for package in lock.packages() { - let Some(index) = package.index() else { - continue; - }; - if !indexes.contains(index) { - let _ = writeln!( - printer.stderr(), - "Ignoring existing lockfile due to removal of referenced registry: {index}" - ); - - // It's fine to prefer the existing versions, though. - return Ok(Self::Preferable(lock)); - } - } - } + let indexes = if index_locations.is_none() { + None + } else { + Some(index_locations) + }; // Determine whether the lockfile satisfies the workspace requirements. - if lock + match lock .satisfies( workspace, members, constraints, overrides, + indexes, interpreter.tags()?, database, ) .await? { - debug!("Existing `uv.lock` satisfies workspace requirements"); - Ok(Self::Satisfies(lock)) - } else { - debug!("Existing `uv.lock` does not satisfy workspace requirements; ignoring..."); - Ok(Self::Preferable(lock)) + SatisfiesResult::Satisfied => { + debug!("Existing `uv.lock` satisfies workspace requirements"); + Ok(Self::Satisfies(lock)) + } + SatisfiesResult::MismatchedMembers(expected, actual) => { + debug!( + "Ignoring existing lockfile due to mismatched members:\n Expected: {:?}\n Actual: {:?}", + expected, actual + ); + Ok(Self::Preferable(lock)) + } + SatisfiesResult::MismatchedConstraints(expected, actual) => { + debug!( + "Ignoring existing lockfile due to mismatched constraints:\n Expected: {:?}\n Actual: {:?}", + expected, actual + ); + Ok(Self::Preferable(lock)) + } + SatisfiesResult::MismatchedOverrides(expected, actual) => { + debug!( + "Ignoring existing lockfile due to mismatched overrides:\n Expected: {:?}\n Actual: {:?}", + expected, actual + ); + Ok(Self::Preferable(lock)) + } + SatisfiesResult::MissingRoot(name) => { + debug!("Ignoring existing lockfile due to missing root package: `{name}`"); + Ok(Self::Preferable(lock)) + } + SatisfiesResult::MissingIndex(name, version, index) => { + debug!( + "Ignoring existing lockfile due to missing index: `{name}` `{version}` from `{index}`" + ); + Ok(Self::Preferable(lock)) + } + SatisfiesResult::MissingMetadata(name, version) => { + debug!( + "Ignoring existing lockfile due to missing metadata for: `{name}=={version}`" + ); + Ok(Self::Preferable(lock)) + } + SatisfiesResult::MismatchedRequiresDist(name, version, expected, actual) => { + debug!( + "Ignoring existing lockfile due to mismatched `requires-dist` for: `{name}=={version}`\n Expected: {:?}\n Actual: {:?}", + expected, actual + ); + Ok(Self::Preferable(lock)) + } + SatisfiesResult::MismatchedDevDependencies(name, version, expected, actual) => { + debug!( + "Ignoring existing lockfile due to mismatched dev dependencies for: `{name}=={version}`\n Expected: {:?}\n Actual: {:?}", + expected, actual + ); + Ok(Self::Preferable(lock)) + } } } @@ -699,7 +723,7 @@ impl ValidatedLock { } /// Write the lockfile to disk. -pub(crate) async fn commit(lock: &Lock, workspace: &Workspace) -> Result<(), ProjectError> { +async fn commit(lock: &Lock, workspace: &Workspace) -> Result<(), ProjectError> { let encoded = lock.to_toml()?; fs_err::tokio::write(workspace.install_path().join("uv.lock"), encoded).await?; Ok(()) @@ -708,7 +732,7 @@ pub(crate) async fn commit(lock: &Lock, workspace: &Workspace) -> Result<(), Pro /// Read the lockfile from the workspace. /// /// Returns `Ok(None)` if the lockfile does not exist. -pub(crate) async fn read(workspace: &Workspace) -> Result, ProjectError> { +async fn read(workspace: &Workspace) -> Result, ProjectError> { match fs_err::tokio::read_to_string(&workspace.install_path().join("uv.lock")).await { Ok(encoded) => match toml::from_str(&encoded) { Ok(lock) => Ok(Some(lock)), diff --git a/crates/uv/tests/lock.rs b/crates/uv/tests/lock.rs index a18c5f8cd..ac41dd14d 100644 --- a/crates/uv/tests/lock.rs +++ b/crates/uv/tests/lock.rs @@ -8203,7 +8203,6 @@ fn lock_change_index() -> Result<()> { ----- stderr ----- warning: `uv lock` is experimental and may change without warning - Ignoring existing lockfile due to removal of referenced registry: https://pypi-proxy.fly.dev/basic-auth/simple Resolved 2 packages in [TIME] "###);