From a883de4fb096ae3bd954f50940d8a8d2b439fe59 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 18 Jan 2024 15:21:16 -0500 Subject: [PATCH] Enforce modification freshness checks against virtual environment (#959) ## Summary This PR is like #957, but for validating the virtual environment, rather than the cache. So, if you have a local wheel, and you rebuild it, we'll now correctly uninstall and reinstall it in the virtual environment. --- crates/puffin-cli/tests/pip_sync.rs | 33 ++++++++++++++++++++++++ crates/puffin-installer/src/plan.rs | 40 +++++++++++++++++++++++------ 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/crates/puffin-cli/tests/pip_sync.rs b/crates/puffin-cli/tests/pip_sync.rs index ae5c0433fe6c..1b59d8a9e49f 100644 --- a/crates/puffin-cli/tests/pip_sync.rs +++ b/crates/puffin-cli/tests/pip_sync.rs @@ -1174,6 +1174,39 @@ fn install_local_wheel() -> Result<()> { check_command(&venv, "import tomli", &temp_dir); + // "Modify" the wheel. + let archive_file = std::fs::File::open(&archive)?; + archive_file.set_modified(std::time::SystemTime::now())?; + + // Reinstall into the same virtual environment. The wheel should be reinstalled. + insta::with_settings!({ + filters => filters.clone() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip") + .arg("sync") + .arg("requirements.txt") + .arg("--strict") + .arg("--cache-dir") + .arg(cache_dir.path()) + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 1 package in [TIME] + Downloaded 1 package in [TIME] + Uninstalled 1 package in [TIME] + Installed 1 package in [TIME] + - tomli==2.0.1 (from file://[TEMP_DIR]/tomli-2.0.1-py3-none-any.whl) + + tomli==2.0.1 (from file://[TEMP_DIR]/tomli-2.0.1-py3-none-any.whl) + "###); + }); + + check_command(&venv, "import tomli", &temp_dir); + Ok(()) } diff --git a/crates/puffin-installer/src/plan.rs b/crates/puffin-installer/src/plan.rs index 8979e4528e9b..80ac20e46fe4 100644 --- a/crates/puffin-installer/src/plan.rs +++ b/crates/puffin-installer/src/plan.rs @@ -7,8 +7,8 @@ use rustc_hash::FxHashSet; use tracing::{debug, warn}; use distribution_types::{ - git_reference, BuiltDist, CachedDirectUrlDist, CachedDist, Dist, IndexLocations, InstalledDist, - Name, SourceDist, + git_reference, BuiltDist, CachedDirectUrlDist, CachedDist, Dist, IndexLocations, + InstalledDirectUrlDist, InstalledDist, Name, SourceDist, }; use pep508_rs::{Requirement, VersionOrUrl}; use platform_tags::Tags; @@ -151,10 +151,18 @@ 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 { - // TODO(charlie): Check freshness for path dependencies. if &distribution.url == url.raw() { - debug!("Requirement already satisfied: {distribution}"); - continue; + // If the requirement came from a local path, check freshness. + if let Ok(archive) = url.to_file_path() { + if is_fresh_install(distribution, &archive)? { + debug!("Requirement already satisfied (and up-to-date): {distribution}"); + continue; + } + } else { + // Otherwise, assume the requirement is up-to-date. + debug!("Requirement already satisfied (assumed up-to-date): {distribution}"); + continue; + } } } } @@ -244,7 +252,7 @@ impl<'a> Planner<'a> { wheel.filename.stem(), ); - if is_fresh(&cache_entry, &wheel.path)? { + if is_fresh_cache(&cache_entry, &wheel.path)? { let cached_dist = CachedDirectUrlDist::from_url( wheel.filename, wheel.url, @@ -281,7 +289,7 @@ impl<'a> Planner<'a> { ); if let Some(wheel) = BuiltWheelIndex::find(&cache_shard, tags) { - if is_fresh(&wheel.entry, &sdist.path)? { + if is_fresh_cache(&wheel.entry, &sdist.path)? { let cached_dist = wheel.into_url_dist(url.clone()); debug!("Path source requirement already cached: {cached_dist}"); local.push(CachedDist::Url(cached_dist)); @@ -346,7 +354,7 @@ impl<'a> Planner<'a> { /// /// A cache entry is considered fresh if it exists and is newer than the file at the given path. /// If the cache entry is stale, it will be removed from the cache. -fn is_fresh(cache_entry: &CacheEntry, artifact: &Path) -> Result { +fn is_fresh_cache(cache_entry: &CacheEntry, artifact: &Path) -> Result { match fs_err::metadata(cache_entry.path()).and_then(|metadata| metadata.modified()) { Ok(cache_mtime) => { // Determine the modification time of the wheel. @@ -375,6 +383,22 @@ fn is_fresh(cache_entry: &CacheEntry, artifact: &Path) -> Result Result { + // Determine the modification time of the installed distribution. + let dist_metadata = fs_err::metadata(&dist.path)?; + let dist_mtime = dist_metadata.modified()?; + + // Determine the modification time of the wheel. + let Some(artifact_mtime) = puffin_cache::archive_mtime(artifact)? else { + // The artifact doesn't exist, so it's not fresh. + return Ok(false); + }; + + Ok(dist_mtime >= artifact_mtime) +} + #[derive(Debug, Default)] pub struct Plan { /// The distributions that are not already installed in the current environment, but are