Skip to content

Commit

Permalink
Enforce modification freshness checks against virtual environment (#959)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
charliermarsh authored Jan 18, 2024
1 parent 96a61fb commit a883de4
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 8 deletions.
33 changes: 33 additions & 0 deletions crates/puffin-cli/tests/pip_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}

Expand Down
40 changes: 32 additions & 8 deletions crates/puffin-installer/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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<bool, io::Error> {
fn is_fresh_cache(cache_entry: &CacheEntry, artifact: &Path) -> Result<bool, io::Error> {
match fs_err::metadata(cache_entry.path()).and_then(|metadata| metadata.modified()) {
Ok(cache_mtime) => {
// Determine the modification time of the wheel.
Expand Down Expand Up @@ -375,6 +383,22 @@ fn is_fresh(cache_entry: &CacheEntry, artifact: &Path) -> Result<bool, io::Error
}
}

/// Returns `true` if the installed distribution linked to the file at the given [`Path`] is fresh,
/// based on the modification time of the installed distribution.
fn is_fresh_install(dist: &InstalledDirectUrlDist, artifact: &Path) -> Result<bool, io::Error> {
// 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
Expand Down

0 comments on commit a883de4

Please sign in to comment.