Make --reinstall imply --refresh (#5425)

## Summary It's hard for me to imagine a scenario in which a user passed `--reinstall`, but wanted us to keep respecting cached data for a package. For example, to actually "rebuild and reinstall" an editable today, you have to pass both `--reinstall` and `--refresh`. This PR makes `--reinstall` imply `--refresh`, so we always validate that the cached data is fresh. Closes #5424.
astral-sh · Jul 25, 2024 · d091932 · d091932
1 parent 4d9098a
commit d091932
Show file tree

Hide file tree

Showing 10 changed files with 114 additions and 23 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/uv-cli/src/lib.rs b/crates/uv-cli/src/lib.rs
@@ -2548,14 +2548,16 @@ pub struct InstallerArgs {
     #[command(flatten)]
     pub index_args: IndexArgs,
 
-    /// Reinstall all packages, regardless of whether they're already installed.
+    /// Reinstall all packages, regardless of whether they're already installed. Implies
+    /// `--refresh`.
     #[arg(long, alias = "force-reinstall", overrides_with("no_reinstall"))]
     pub reinstall: bool,
 
     #[arg(long, overrides_with("reinstall"), hide = true)]
     pub no_reinstall: bool,
 
-    /// Reinstall a specific package, regardless of whether it's already installed.
+    /// Reinstall a specific package, regardless of whether it's already installed. Implies
+    /// `--refresh-package`.
     #[arg(long)]
     pub reinstall_package: Vec<PackageName>,
 
@@ -2712,14 +2714,16 @@ pub struct ResolverInstallerArgs {
     #[arg(long, short = 'P')]
     pub upgrade_package: Vec<Requirement<VerbatimParsedUrl>>,
 
-    /// Reinstall all packages, regardless of whether they're already installed.
+    /// Reinstall all packages, regardless of whether they're already installed. Implies
+    /// `--refresh`.
     #[arg(long, alias = "force-reinstall", overrides_with("no_reinstall"))]
     pub reinstall: bool,
 
     #[arg(long, overrides_with("reinstall"), hide = true)]
     pub no_reinstall: bool,
 
-    /// Reinstall a specific package, regardless of whether it's already installed.
+    /// Reinstall a specific package, regardless of whether it's already installed. Implies
+    /// `--refresh-package`.
     #[arg(long)]
     pub reinstall_package: Vec<PackageName>,
 

diff --git a/crates/uv-configuration/Cargo.toml b/crates/uv-configuration/Cargo.toml
@@ -17,6 +17,7 @@ pep508_rs = { workspace = true, features = ["schemars"] }
 platform-tags = { workspace = true }
 pypi-types = { workspace = true }
 uv-auth = { workspace = true }
+uv-cache = { workspace = true }
 uv-normalize = { workspace = true }
 
 clap = { workspace = true, features = ["derive"], optional = true }

diff --git a/crates/uv-configuration/src/package_options.rs b/crates/uv-configuration/src/package_options.rs
@@ -3,6 +3,7 @@ use pep508_rs::PackageName;
 
 use pypi_types::Requirement;
 use rustc_hash::FxHashMap;
+use uv_cache::Refresh;
 
 /// Whether to reinstall packages.
 #[derive(Debug, Default, Clone)]
@@ -43,6 +44,32 @@ 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)
+            }
+        }
+    }
 }
 
 /// Whether to allow package upgrades.

diff --git a/crates/uv-settings/src/settings.rs b/crates/uv-settings/src/settings.rs
@@ -380,7 +380,7 @@ pub struct ResolverInstallerOptions {
         "#
     )]
     pub upgrade_package: Option<Vec<Requirement<VerbatimParsedUrl>>>,
-    /// Reinstall all packages, regardless of whether they're already installed.
+    /// Reinstall all packages, regardless of whether they're already installed. Implies `refresh`.
     #[option(
         default = "false",
         value_type = "bool",
@@ -389,7 +389,8 @@ pub struct ResolverInstallerOptions {
         "#
     )]
     pub reinstall: Option<bool>,
-    /// Reinstall a specific package, regardless of whether it's already installed.
+    /// Reinstall a specific package, regardless of whether it's already installed. Implies
+    /// `refresh-package`.
     #[option(
         default = "[]",
         value_type = "list[str]",
@@ -1056,7 +1057,7 @@ pub struct PipOptions {
         "#
     )]
     pub upgrade_package: Option<Vec<Requirement<VerbatimParsedUrl>>>,
-    /// Reinstall all packages, regardless of whether they're already installed.
+    /// Reinstall all packages, regardless of whether they're already installed. Implies `refresh`.
     #[option(
         default = "false",
         value_type = "bool",
@@ -1065,7 +1066,8 @@ pub struct PipOptions {
         "#
     )]
     pub reinstall: Option<bool>,
-    /// Reinstall a specific package, regardless of whether it's already installed.
+    /// Reinstall a specific package, regardless of whether it's already installed. Implies
+    /// `refresh-package`.
     #[option(
         default = "[]",
         value_type = "list[str]",

diff --git a/crates/uv/src/lib.rs b/crates/uv/src/lib.rs
@@ -179,6 +179,7 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
                 .expect("failed to initialize global rayon pool");
 
             // Initialize the cache.
+
             let cache = cache.init()?.with_refresh(args.refresh);
 
             let requirements = args
@@ -262,7 +263,9 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
                 .expect("failed to initialize global rayon pool");
 
             // Initialize the cache.
-            let cache = cache.init()?.with_refresh(args.refresh);
+            let cache = cache
+                .init()?
+                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
 
             let requirements = args
                 .src_file
@@ -324,7 +327,10 @@ async fn run(cli: Cli) -> Result<ExitStatus> {
                 .expect("failed to initialize global rayon pool");
 
             // Initialize the cache.
-            let cache = cache.init()?.with_refresh(args.refresh);
+            let cache = cache
+                .init()?
+                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
+
             let requirements = args
                 .package
                 .into_iter()
@@ -880,7 +886,9 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache.init()?.with_refresh(args.refresh);
+            let cache = cache
+                .init()?
+                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
 
             let requirements = args
                 .with
@@ -921,7 +929,9 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache.init()?.with_refresh(args.refresh);
+            let cache = cache
+                .init()?
+                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
 
             commands::sync(
                 args.locked,
@@ -948,7 +958,7 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache.init()?.with_refresh(args.refresh);
+            let cache = cache.init()?;
 
             commands::lock(
                 args.locked,
@@ -972,7 +982,9 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache.init()?.with_refresh(args.refresh);
+            let cache = cache
+                .init()?
+                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
 
             commands::add(
                 args.locked,
@@ -1005,7 +1017,9 @@ async fn run_project(
             show_settings!(args);
 
             // Initialize the cache.
-            let cache = cache.init()?.with_refresh(args.refresh);
+            let cache = cache
+                .init()?
+                .with_refresh(args.settings.reinstall.clone().to_refresh(args.refresh));
 
             commands::remove(
                 args.locked,

diff --git a/crates/uv/tests/pip_install.rs b/crates/uv/tests/pip_install.rs
@@ -585,6 +585,7 @@ fn respect_installed_and_reinstall() -> Result<()> {
 
     ----- stderr -----
     Resolved [N] packages in [TIME]
+    Prepared [N] packages in [TIME]
     Uninstalled [N] packages in [TIME]
     Installed [N] packages in [TIME]
      - flask==3.0.2
@@ -1816,6 +1817,7 @@ fn reinstall_no_binary() {
 
     ----- stderr -----
     Resolved [N] packages in [TIME]
+    Prepared [N] packages in [TIME]
     Uninstalled [N] packages in [TIME]
     Installed [N] packages in [TIME]
      - anyio==4.3.0
@@ -4972,6 +4974,7 @@ fn already_installed_remote_url() {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - uv-public-pypackage==0.1.0 (from git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389)

diff --git a/crates/uv/tests/pip_sync.rs b/crates/uv/tests/pip_sync.rs
@@ -1962,6 +1962,7 @@ fn reinstall() -> Result<()> {
 
     ----- stderr -----
     Resolved 2 packages in [TIME]
+    Prepared 2 packages in [TIME]
     Uninstalled 2 packages in [TIME]
     Installed 2 packages in [TIME]
      - markupsafe==2.1.3
@@ -2016,6 +2017,7 @@ fn reinstall_package() -> Result<()> {
 
     ----- stderr -----
     Resolved 2 packages in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - tomli==2.0.1
@@ -2069,6 +2071,7 @@ fn reinstall_git() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - uv-public-pypackage==0.1.0 (from git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389)
@@ -2340,6 +2343,31 @@ fn sync_editable() -> Result<()> {
     "###
     );
 
+    // Modify the `pyproject.toml` file.
+    let pyproject_toml = poetry_editable.path().join("pyproject.toml");
+    let pyproject_toml_contents = fs_err::read_to_string(&pyproject_toml)?;
+    fs_err::write(
+        &pyproject_toml,
+        pyproject_toml_contents.replace("0.1.0", "0.1.1"),
+    )?;
+
+    // Reinstall the editable package. This will trigger a rebuild and reinstall.
+    uv_snapshot!(context.filters(), context.pip_sync()
+        .arg(requirements_txt.path()), @r###"
+    success: true
+    exit_code: 0
+    ----- stdout -----
+
+    ----- stderr -----
+    Resolved 3 packages in [TIME]
+    Prepared 1 package in [TIME]
+    Uninstalled 1 package in [TIME]
+    Installed 1 package in [TIME]
+     - poetry-editable==0.1.0 (from file://[TEMP_DIR]/poetry_editable)
+     + poetry-editable==0.1.1 (from file://[TEMP_DIR]/poetry_editable)
+    "###
+    );
+
     Ok(())
 }
 
@@ -2781,6 +2809,7 @@ fn find_links_wheel_cache() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - tqdm==1000.0.0
@@ -2831,6 +2860,7 @@ fn find_links_source_cache() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - tqdm==999.0.0
@@ -3746,6 +3776,7 @@ fn require_hashes_source_url() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - source-distribution==0.0.1 (from https://files.pythonhosted.org/packages/10/1f/57aa4cce1b1abf6b433106676e15f9fa2c92ed2bd4cf77c3b50a9e9ac773/source_distribution-0.0.1.tar.gz)
@@ -3847,6 +3878,7 @@ fn require_hashes_wheel_url() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - anyio==4.0.0 (from https://files.pythonhosted.org/packages/36/55/ad4de788d84a630656ece71059665e01ca793c04294c463fd84132f40fe6/anyio-4.0.0-py3-none-any.whl)
@@ -4059,6 +4091,7 @@ fn require_hashes_re_download() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - anyio==4.0.0
@@ -4459,6 +4492,7 @@ fn require_hashes_at_least_one() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - anyio==4.0.0
@@ -4481,6 +4515,7 @@ fn require_hashes_at_least_one() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Uninstalled 1 package in [TIME]
     Installed 1 package in [TIME]
      - anyio==4.0.0
@@ -4716,6 +4751,7 @@ fn require_hashes_find_links_invalid_hash() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Installed 1 package in [TIME]
      + example-a-961b4c22==1.0.0
     "###
@@ -4922,6 +4958,7 @@ fn require_hashes_registry_invalid_hash() -> Result<()> {
 
     ----- stderr -----
     Resolved 1 package in [TIME]
+    Prepared 1 package in [TIME]
     Installed 1 package in [TIME]
      + example-a-961b4c22==1.0.0
     "###