Make --upgrade imply --refresh (#5943)

I think this seems reasonable... Otherwise, we might not go back to PyPI to revalidate the list of available versions despite the user passing `--upgrade`.
astral-sh · Aug 9, 2024 · 4f56a27 · 4f56a27
1 parent e9ec7be
commit 4f56a27
Show file tree

Hide file tree

Showing 5 changed files with 134 additions and 101 deletions.
diff --git a/crates/uv-cache/src/lib.rs b/crates/uv-cache/src/lib.rs
@@ -229,26 +229,6 @@ impl Cache {
         }
     }
 
-    /// Returns `true` if a cache entry is up-to-date. Unlike [`Cache::freshness`], this method does
-    /// not take the [`Refresh`] policy into account.
-    ///
-    /// A cache entry is considered up-to-date if it was created after the [`Cache`] instance itself
-    /// was initialized.
-    pub fn is_fresh(&self, entry: &CacheEntry) -> io::Result<bool> {
-        // Grab the cutoff timestamp.
-        let timestamp = match &self.refresh {
-            Refresh::None(timestamp) => timestamp,
-            Refresh::All(timestamp) => timestamp,
-            Refresh::Packages(_packages, timestamp) => timestamp,
-        };
-
-        match fs::metadata(entry.path()) {
-            Ok(metadata) => Ok(Timestamp::from_metadata(&metadata) >= *timestamp),
-            Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(false),
-            Err(err) => Err(err),
-        }
-    }
-
     /// Persist a temporary directory to the artifact store, returning its unique ID.
     pub async fn persist(
         &self,
@@ -1004,9 +984,6 @@ impl Freshness {
 }
 
 /// A refresh policy for cache entries.
-///
-/// Each policy stores a timestamp, even if no entries are refreshed, to enable out-of-policy
-/// freshness checks via [`Cache::is_fresh`].
 #[derive(Debug, Clone)]
 pub enum Refresh {
     /// Don't refresh any entries.
@@ -1038,4 +1015,42 @@ impl Refresh {
     pub fn is_none(&self) -> bool {
         matches!(self, Self::None(_))
     }
+
+    /// Combine two [`Refresh`] policies, taking the "max" of the two policies.
+    #[must_use]
+    pub fn combine(self, other: Refresh) -> Self {
+        /// Return the maximum of two timestamps.
+        fn max(a: Timestamp, b: Timestamp) -> Timestamp {
+            if a > b {
+                a
+            } else {
+                b
+            }
+        }
+
+        match (self, other) {
+            // If the policy is `None`, return the existing refresh policy.
+            // Take the `max` of the two timestamps.
+            (Self::None(t1), Refresh::None(t2)) => Refresh::None(max(t1, t2)),
+            (Self::None(t1), Refresh::All(t2)) => Refresh::All(max(t1, t2)),
+            (Self::None(t1), Refresh::Packages(packages, t2)) => {
+                Refresh::Packages(packages, max(t1, t2))
+            }
+
+            // If the policy is `All`, refresh all packages.
+            (Self::All(t1), Refresh::None(t2)) => Refresh::All(max(t1, t2)),
+            (Self::All(t1), Refresh::All(t2)) => Refresh::All(max(t1, t2)),
+            (Self::All(t1), Refresh::Packages(_packages, t2)) => Refresh::All(max(t1, t2)),
+
+            // If the policy is `Packages`, take the "max" of the two policies.
+            (Self::Packages(packages, t1), Refresh::None(t2)) => {
+                Refresh::Packages(packages, max(t1, t2))
+            }
+            (Self::Packages(_packages, t1), Refresh::All(t2)) => Refresh::All(max(t1, t2)),
+            (Self::Packages(packages1, t1), Refresh::Packages(packages2, t2)) => Refresh::Packages(
+                packages1.into_iter().chain(packages2).collect(),
+                max(t1, t2),
+            ),
+        }
+    }
 }
diff --git a/crates/uv-cli/src/lib.rs b/crates/uv-cli/src/lib.rs
@@ -3132,7 +3132,8 @@ pub struct ResolverArgs {
     #[command(flatten)]
     pub index_args: IndexArgs,
 
-    /// Allow package upgrades, ignoring pinned versions in any existing output file.
+    /// Allow package upgrades, ignoring pinned versions in any existing output file. Implies
+    /// `--refresh`.
     #[arg(
         long,
         short = 'U',
@@ -3150,7 +3151,7 @@ pub struct ResolverArgs {
     pub no_upgrade: bool,
 
     /// Allow upgrades for a specific package, ignoring pinned versions in any existing output
-    /// file.
+    /// file. Implies `--refresh-package`.
     #[arg(long, short = 'P', help_heading = "Resolver options")]
     pub upgrade_package: Vec<Requirement<VerbatimParsedUrl>>,
 
@@ -3280,7 +3281,8 @@ pub struct ResolverInstallerArgs {
     #[command(flatten)]
     pub index_args: IndexArgs,
 
-    /// Allow package upgrades, ignoring pinned versions in any existing output file.
+    /// Allow package upgrades, ignoring pinned versions in any existing output file. Implies
+    /// `--refresh`.
     #[arg(
         long,
         short = 'U',
@@ -3298,7 +3300,7 @@ pub struct ResolverInstallerArgs {
     pub no_upgrade: bool,
 
     /// Allow upgrades for a specific package, ignoring pinned versions in any existing output
-    /// file.
+    /// file. Implies `--refresh-package`.
     #[arg(long, short = 'P', help_heading = "Resolver options")]
     pub upgrade_package: Vec<Requirement<VerbatimParsedUrl>>,
 

diff --git a/crates/uv-configuration/src/package_options.rs b/crates/uv-configuration/src/package_options.rs
@@ -3,7 +3,7 @@ use pep508_rs::PackageName;
 
 use pypi_types::Requirement;
 use rustc_hash::FxHashMap;
-use uv_cache::Refresh;
+use uv_cache::{Refresh, Timestamp};
 
 /// Whether to reinstall packages.
 #[derive(Debug, Default, Clone)]
@@ -44,30 +44,15 @@ impl Reinstall {
     pub fn is_all(&self) -> bool {
         matches!(self, Self::All)
     }
+}
 
-    /// Create a [`Refresh`] policy by integrating the [`Reinstall`] policy.
-    pub fn to_refresh(self, refresh: Refresh) -> Refresh {
-        match (self, refresh) {
-            // If the policy is `None`, return the existing refresh policy.
-            (Self::None, Refresh::None(timestamp)) => Refresh::None(timestamp),
-            (Self::None, Refresh::All(timestamp)) => Refresh::All(timestamp),
-            (Self::None, Refresh::Packages(packages, timestamp)) => {
-                Refresh::Packages(packages, timestamp)
-            }
-
-            // If the policy is `All`, refresh all packages.
-            (Self::All, Refresh::None(timestamp)) => Refresh::All(timestamp),
-            (Self::All, Refresh::All(timestamp)) => Refresh::All(timestamp),
-            (Self::All, Refresh::Packages(_packages, timestamp)) => Refresh::All(timestamp),
-
-            // If the policy is `Packages`, take the "max" of the two policies.
-            (Self::Packages(packages), Refresh::None(timestamp)) => {
-                Refresh::Packages(packages, timestamp)
-            }
-            (Self::Packages(_packages), Refresh::All(timestamp)) => Refresh::All(timestamp),
-            (Self::Packages(packages1), Refresh::Packages(packages2, timestamp)) => {
-                Refresh::Packages(packages1.into_iter().chain(packages2).collect(), timestamp)
-            }
+/// Create a [`Refresh`] policy by integrating the [`Reinstall`] policy.
+impl From<Reinstall> for Refresh {
+    fn from(value: Reinstall) -> Self {
+        match value {
+            Reinstall::None => Self::None(Timestamp::now()),
+            Reinstall::All => Self::All(Timestamp::now()),
+            Reinstall::Packages(packages) => Self::Packages(packages, Timestamp::now()),
         }
     }
 }
@@ -144,3 +129,16 @@ impl Upgrade {
         }
     }
 }
+
+/// Create a [`Refresh`] policy by integrating the [`Upgrade`] policy.
+impl From<Upgrade> for Refresh {
+    fn from(value: Upgrade) -> Self {
+        match value {
+            Upgrade::None => Self::None(Timestamp::now()),
+            Upgrade::All => Self::All(Timestamp::now()),
+            Upgrade::Packages(packages) => {
+                Self::Packages(packages.into_keys().collect::<Vec<_>>(), Timestamp::now())
+            }
+        }
+    }
+}
diff --git a/crates/uv/src/lib.rs b/crates/uv/src/lib.rs
@@ -12,7 +12,7 @@ use owo_colors::OwoColorize;
 use tracing::{debug, instrument};
 
 use settings::PipTreeSettings;
-use uv_cache::Cache;
+use uv_cache::{Cache, Refresh};
 use uv_cli::{
     compat::CompatArgs, CacheCommand, CacheNamespace, Cli, Commands, PipCommand, PipNamespace,
     ProjectCommand,
@@ -224,9 +224,11 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
                 .expect("failed to initialize global rayon pool");
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             let requirements = args
                 .src_file
@@ -317,9 +319,11 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
                 .expect("failed to initialize global rayon pool");
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             let requirements = args
                 .src_file
@@ -389,9 +393,11 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
                 .expect("failed to initialize global rayon pool");
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             let requirements = args
                 .package
@@ -705,9 +711,11 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             let requirements = args
                 .with
@@ -748,9 +756,11 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             let requirements = args
                 .with
@@ -996,9 +1006,11 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             let requirements = args
                 .with
@@ -1041,9 +1053,11 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             commands::sync(
                 args.locked,
@@ -1095,9 +1109,11 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             commands::add(
                 args.locked,
@@ -1131,9 +1147,11 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache
-                .init()?
-                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+            let cache = cache.init()?.with_refresh(
+                args.refresh
+                    .combine(Refresh::from(args.settings.reinstall.clone()))
+                    .combine(Refresh::from(args.settings.upgrade.clone())),
+            );
 
             commands::remove(
                 args.locked,