From 6f94dc3c7794fbaf1bd05dfb593ec9ee542df597 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 4 Mar 2024 11:10:51 -0800 Subject: [PATCH] Centralize up-to-date checking for path installations (#2168) ## Summary Internal-only refactor to consolidate multiple codepaths we have for checking whether a cached or installed entry is up-to-date with a local requirement. --- crates/uv-cache/src/lib.rs | 31 ++++++++ .../src/distribution_database.rs | 52 ++++++------- .../src/index/built_wheel_index.rs | 3 +- crates/uv-installer/src/editable.rs | 14 +--- crates/uv-installer/src/lib.rs | 2 +- crates/uv-installer/src/plan.rs | 75 ++++++------------- crates/uv-installer/src/site_packages.rs | 8 +- crates/uv/src/commands/pip_sync.rs | 17 +++-- 8 files changed, 98 insertions(+), 104 deletions(-) diff --git a/crates/uv-cache/src/lib.rs b/crates/uv-cache/src/lib.rs index 9e8fa5255..592f88c22 100644 --- a/crates/uv-cache/src/lib.rs +++ b/crates/uv-cache/src/lib.rs @@ -6,6 +6,7 @@ use std::ops::Deref; use std::path::{Path, PathBuf}; use std::sync::Arc; +use distribution_types::InstalledDist; use fs_err as fs; use tempfile::{tempdir, TempDir}; @@ -692,6 +693,36 @@ impl ArchiveTimestamp { Self::Approximate(timestamp) => *timestamp, } } + + /// Returns `true` if the `target` (an installed or cached distribution) is up-to-date with the + /// source archive (`source`). + /// + /// The `target` should be an installed package in a virtual environment, or an unzipped + /// package in the cache. + /// + /// The `source` is a source archive, i.e., a path to a built wheel or a Python package directory. + pub fn up_to_date_with(source: &Path, target: ArchiveTarget) -> Result { + let Some(modified_at) = Self::from_path(source)? else { + // If there's no entrypoint, we can't determine the modification time, so we assume that the + // target is not up-to-date. + return Ok(false); + }; + let created_at = match target { + ArchiveTarget::Install(installed) => { + Timestamp::from_path(installed.path().join("METADATA"))? + } + ArchiveTarget::Cache(cache) => Timestamp::from_path(cache)?, + }; + Ok(modified_at.timestamp() <= created_at) + } +} + +#[derive(Debug, Clone, Copy)] +pub enum ArchiveTarget<'a> { + /// The target is an installed package in a virtual environment. + Install(&'a InstalledDist), + /// The target is an unzipped package in the cache. + Cache(&'a Path), } impl PartialOrd for ArchiveTimestamp { diff --git a/crates/uv-distribution/src/distribution_database.rs b/crates/uv-distribution/src/distribution_database.rs index b79bb3526..c4379ebd3 100644 --- a/crates/uv-distribution/src/distribution_database.rs +++ b/crates/uv-distribution/src/distribution_database.rs @@ -13,9 +13,9 @@ use distribution_types::{ }; use platform_tags::Tags; use pypi_types::Metadata21; -use uv_cache::{Cache, CacheBucket, Timestamp, WheelCache}; +use uv_cache::{ArchiveTarget, ArchiveTimestamp, Cache, CacheBucket, WheelCache}; use uv_client::{CacheControl, CachedClientError, Connectivity, RegistryClient}; -use uv_fs::metadata_if_exists; + use uv_git::GitSource; use uv_traits::{BuildContext, NoBinary, NoBuild}; @@ -123,19 +123,17 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context> // return it. match cache_entry.path().canonicalize() { Ok(archive) => { - if let (Some(cache_metadata), Some(path_metadata)) = ( - metadata_if_exists(&archive).map_err(Error::CacheRead)?, - metadata_if_exists(path).map_err(Error::CacheRead)?, - ) { - let cache_modified = Timestamp::from_metadata(&cache_metadata); - let path_modified = Timestamp::from_metadata(&path_metadata); - if cache_modified >= path_modified { - return Ok(LocalWheel::Unzipped(UnzippedWheel { - dist: dist.clone(), - archive, - filename: wheel.filename.clone(), - })); - } + if ArchiveTimestamp::up_to_date_with( + path, + ArchiveTarget::Cache(&archive), + ) + .map_err(Error::CacheRead)? + { + return Ok(LocalWheel::Unzipped(UnzippedWheel { + dist: dist.clone(), + archive, + filename: wheel.filename.clone(), + })); } } Err(err) if err.kind() == io::ErrorKind::NotFound => {} @@ -290,19 +288,17 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context> // return it. match cache_entry.path().canonicalize() { Ok(archive) => { - if let (Some(cache_metadata), Some(path_metadata)) = ( - metadata_if_exists(&archive).map_err(Error::CacheRead)?, - metadata_if_exists(&wheel.path).map_err(Error::CacheRead)?, - ) { - let cache_modified = Timestamp::from_metadata(&cache_metadata); - let path_modified = Timestamp::from_metadata(&path_metadata); - if cache_modified >= path_modified { - return Ok(LocalWheel::Unzipped(UnzippedWheel { - dist: dist.clone(), - archive, - filename: wheel.filename.clone(), - })); - } + if ArchiveTimestamp::up_to_date_with( + &wheel.path, + ArchiveTarget::Cache(&archive), + ) + .map_err(Error::CacheRead)? + { + return Ok(LocalWheel::Unzipped(UnzippedWheel { + dist: dist.clone(), + archive, + filename: wheel.filename.clone(), + })); } } Err(err) if err.kind() == io::ErrorKind::NotFound => {} diff --git a/crates/uv-distribution/src/index/built_wheel_index.rs b/crates/uv-distribution/src/index/built_wheel_index.rs index f50895215..a082fa316 100644 --- a/crates/uv-distribution/src/index/built_wheel_index.rs +++ b/crates/uv-distribution/src/index/built_wheel_index.rs @@ -48,7 +48,8 @@ impl BuiltWheelIndex { ); // Determine the last-modified time of the source distribution. - let Some(modified) = ArchiveTimestamp::from_path(&source_dist.path).expect("archived") + let Some(modified) = + ArchiveTimestamp::from_path(&source_dist.path).map_err(Error::CacheRead)? else { return Err(Error::DirWithoutEntrypoint); }; diff --git a/crates/uv-installer/src/editable.rs b/crates/uv-installer/src/editable.rs index 88b9803d5..54ffb2a32 100644 --- a/crates/uv-installer/src/editable.rs +++ b/crates/uv-installer/src/editable.rs @@ -6,7 +6,7 @@ use distribution_types::{ }; use pypi_types::Metadata21; use requirements_txt::EditableRequirement; -use uv_cache::ArchiveTimestamp; + use uv_normalize::PackageName; /// An editable distribution that has been built. @@ -69,18 +69,6 @@ impl std::fmt::Display for ResolvedEditable { } } -/// Returns `true` if the installed distribution is up-to-date with the [`EditableRequirement`]. -pub fn not_modified(editable: &EditableRequirement, installed: &InstalledDist) -> bool { - let Ok(Some(installed_at)) = ArchiveTimestamp::from_path(installed.path().join("METADATA")) - else { - return false; - }; - let Ok(Some(modified_at)) = ArchiveTimestamp::from_path(&editable.path) else { - return false; - }; - installed_at > modified_at -} - /// Returns `true` if the [`EditableRequirement`] contains dynamic metadata. pub fn is_dynamic(editable: &EditableRequirement) -> bool { // If there's no `pyproject.toml`, we assume it's dynamic. diff --git a/crates/uv-installer/src/lib.rs b/crates/uv-installer/src/lib.rs index 9e6b4ab3c..51bc86724 100644 --- a/crates/uv-installer/src/lib.rs +++ b/crates/uv-installer/src/lib.rs @@ -1,5 +1,5 @@ pub use downloader::{Downloader, Reporter as DownloadReporter}; -pub use editable::{is_dynamic, not_modified, BuiltEditable, ResolvedEditable}; +pub use editable::{is_dynamic, BuiltEditable, ResolvedEditable}; pub use installer::{Installer, Reporter as InstallReporter}; pub use plan::{Plan, Planner, Reinstall}; pub use site_packages::SitePackages; diff --git a/crates/uv-installer/src/plan.rs b/crates/uv-installer/src/plan.rs index dd8358801..a9f856f47 100644 --- a/crates/uv-installer/src/plan.rs +++ b/crates/uv-installer/src/plan.rs @@ -1,19 +1,18 @@ use std::collections::hash_map::Entry; use std::hash::BuildHasherDefault; use std::io; -use std::path::Path; use anyhow::{bail, Result}; use rustc_hash::FxHashMap; use tracing::{debug, warn}; use distribution_types::{ - BuiltDist, CachedDirectUrlDist, CachedDist, Dist, IndexLocations, InstalledDirectUrlDist, - InstalledDist, InstalledMetadata, InstalledVersion, Name, SourceDist, + BuiltDist, CachedDirectUrlDist, CachedDist, Dist, IndexLocations, InstalledDist, + InstalledMetadata, InstalledVersion, Name, SourceDist, }; use pep508_rs::{Requirement, VersionOrUrl}; use platform_tags::Tags; -use uv_cache::{ArchiveTimestamp, Cache, CacheBucket, CacheEntry, Timestamp, WheelCache}; +use uv_cache::{ArchiveTarget, ArchiveTimestamp, Cache, CacheBucket, WheelCache}; use uv_distribution::{BuiltWheelIndex, RegistryWheelIndex}; use uv_fs::Simplified; use uv_interpreter::PythonEnvironment; @@ -186,17 +185,20 @@ impl<'a> Planner<'a> { // If the requirement comes from a direct URL, check by URL. Some(VersionOrUrl::Url(url)) => { - if let InstalledDist::Url(distribution) = &distribution { - if &distribution.url == url.raw() { + if let InstalledDist::Url(installed) = &distribution { + if &installed.url == url.raw() { // If the requirement came from a local path, check freshness. if let Ok(archive) = url.to_file_path() { - if not_modified_install(distribution, &archive)? { - debug!("Requirement already satisfied (and up-to-date): {distribution}"); + if ArchiveTimestamp::up_to_date_with( + &archive, + ArchiveTarget::Install(distribution), + )? { + debug!("Requirement already satisfied (and up-to-date): {installed}"); continue; } } else { // Otherwise, assume the requirement is up-to-date. - debug!("Requirement already satisfied (assumed up-to-date): {distribution}"); + debug!("Requirement already satisfied (assumed up-to-date): {installed}"); continue; } } @@ -320,9 +322,12 @@ impl<'a> Planner<'a> { ) .entry(wheel.filename.stem()); - if not_modified_cache(&cache_entry, &wheel.path)? { - match cache_entry.path().canonicalize() { - Ok(archive) => { + match cache_entry.path().canonicalize() { + Ok(archive) => { + if ArchiveTimestamp::up_to_date_with( + &wheel.path, + ArchiveTarget::Cache(&archive), + )? { let cached_dist = CachedDirectUrlDist::from_url( wheel.filename, wheel.url, @@ -335,11 +340,11 @@ impl<'a> Planner<'a> { local.push(CachedDist::Url(cached_dist)); continue; } - Err(err) if err.kind() == io::ErrorKind::NotFound => { - // The cache entry doesn't exist, so it's not fresh. - } - Err(err) => return Err(err.into()), } + Err(err) if err.kind() == io::ErrorKind::NotFound => { + // The cache entry doesn't exist, so it's not fresh. + } + Err(err) => return Err(err.into()), } } Dist::Source(SourceDist::DirectUrl(sdist)) => { @@ -418,44 +423,6 @@ enum Specifier<'a> { 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. -fn not_modified_cache(cache_entry: &CacheEntry, artifact: &Path) -> Result { - match fs_err::metadata(cache_entry.path()).map(|metadata| Timestamp::from_metadata(&metadata)) { - Ok(cache_timestamp) => { - // Determine the modification time of the wheel. - if let Some(artifact_timestamp) = ArchiveTimestamp::from_path(artifact)? { - Ok(cache_timestamp >= artifact_timestamp.timestamp()) - } else { - // The artifact doesn't exist, so it's not fresh. - Ok(false) - } - } - Err(err) if err.kind() == io::ErrorKind::NotFound => { - // The cache entry doesn't exist, so it's not fresh. - Ok(false) - } - Err(err) => Err(err), - } -} - -/// Returns `true` if the installed distribution linked to the file at the given [`Path`] is -/// not-modified based on the modification time of the installed distribution. -fn not_modified_install(dist: &InstalledDirectUrlDist, artifact: &Path) -> Result { - // Determine the modification time of the installed distribution. - let dist_metadata = fs_err::metadata(dist.path.join("METADATA"))?; - let dist_timestamp = Timestamp::from_metadata(&dist_metadata); - - // Determine the modification time of the wheel. - let Some(artifact_timestamp) = ArchiveTimestamp::from_path(artifact)? else { - // The artifact doesn't exist, so it's not fresh. - return Ok(false); - }; - - Ok(dist_timestamp >= artifact_timestamp.timestamp()) -} - #[derive(Debug, Default)] pub struct Plan { /// The distributions that are not already installed in the current environment, but are diff --git a/crates/uv-installer/src/site_packages.rs b/crates/uv-installer/src/site_packages.rs index f83ed8a21..66f1240ef 100644 --- a/crates/uv-installer/src/site_packages.rs +++ b/crates/uv-installer/src/site_packages.rs @@ -11,10 +11,11 @@ use distribution_types::{InstalledDist, InstalledMetadata, InstalledVersion, Nam use pep440_rs::{Version, VersionSpecifiers}; use pep508_rs::{Requirement, VerbatimUrl}; use requirements_txt::EditableRequirement; +use uv_cache::{ArchiveTarget, ArchiveTimestamp}; use uv_interpreter::PythonEnvironment; use uv_normalize::PackageName; -use crate::{is_dynamic, not_modified}; +use crate::is_dynamic; /// An index over the packages installed in an environment. /// @@ -276,7 +277,10 @@ impl<'a> SitePackages<'a> { } [distribution] => { // Is the editable out-of-date? - if !not_modified(requirement, distribution) { + if !ArchiveTimestamp::up_to_date_with( + &requirement.path, + ArchiveTarget::Install(distribution), + )? { return Ok(false); } diff --git a/crates/uv/src/commands/pip_sync.rs b/crates/uv/src/commands/pip_sync.rs index 3ba139452..a79310c8b 100644 --- a/crates/uv/src/commands/pip_sync.rs +++ b/crates/uv/src/commands/pip_sync.rs @@ -11,13 +11,12 @@ use platform_host::Platform; use platform_tags::Tags; use pypi_types::Yanked; use requirements_txt::EditableRequirement; -use uv_cache::Cache; +use uv_cache::{ArchiveTarget, ArchiveTimestamp, Cache}; use uv_client::{Connectivity, FlatIndex, FlatIndexClient, RegistryClient, RegistryClientBuilder}; use uv_dispatch::BuildDispatch; use uv_fs::Simplified; use uv_installer::{ - is_dynamic, not_modified, Downloader, NoBinary, Plan, Planner, Reinstall, ResolvedEditable, - SitePackages, + is_dynamic, Downloader, NoBinary, Plan, Planner, Reinstall, ResolvedEditable, SitePackages, }; use uv_interpreter::PythonEnvironment; use uv_resolver::InMemoryIndex; @@ -426,7 +425,11 @@ async fn resolve_editables( match existing.as_slice() { [] => uninstalled.push(editable), [dist] => { - if not_modified(&editable, dist) && !is_dynamic(&editable) { + if ArchiveTimestamp::up_to_date_with( + &editable.path, + ArchiveTarget::Install(dist), + )? && !is_dynamic(&editable) + { installed.push((*dist).clone()); } else { uninstalled.push(editable); @@ -447,7 +450,11 @@ async fn resolve_editables( [dist] => { if packages.contains(dist.name()) { uninstalled.push(editable); - } else if not_modified(&editable, dist) && !is_dynamic(&editable) { + } else if ArchiveTimestamp::up_to_date_with( + &editable.path, + ArchiveTarget::Install(dist), + )? && !is_dynamic(&editable) + { installed.push((*dist).clone()); } else { uninstalled.push(editable);