From d0919329fd8d392d397bcbf6a84afa05d8e9d68d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 25 Jul 2024 09:45:58 -0400 Subject: [PATCH] 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 https://github.com/astral-sh/uv/issues/5424. --- Cargo.lock | 1 + crates/uv-cli/src/lib.rs | 12 ++++-- crates/uv-configuration/Cargo.toml | 1 + .../uv-configuration/src/package_options.rs | 27 ++++++++++++++ crates/uv-settings/src/settings.rs | 10 +++-- crates/uv/src/lib.rs | 28 ++++++++++---- crates/uv/tests/pip_install.rs | 3 ++ crates/uv/tests/pip_sync.rs | 37 +++++++++++++++++++ docs/settings.md | 10 +++-- uv.schema.json | 8 ++-- 10 files changed, 114 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b4dc0df8b..fc04a8676 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4668,6 +4668,7 @@ dependencies = [ "serde_json", "tracing", "uv-auth", + "uv-cache", "uv-normalize", ] diff --git a/crates/uv-cli/src/lib.rs b/crates/uv-cli/src/lib.rs index 7e6a141d9..4926a980a 100644 --- 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, @@ -2712,14 +2714,16 @@ pub struct ResolverInstallerArgs { #[arg(long, short = 'P')] pub upgrade_package: Vec>, - /// 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, diff --git a/crates/uv-configuration/Cargo.toml b/crates/uv-configuration/Cargo.toml index 894237547..a02c1d76b 100644 --- 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 index 928f03aef..6f39dc5a4 100644 --- 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 index 6190cf48e..18e07da19 100644 --- 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>>, - /// 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, - /// 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>>, - /// 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, - /// 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 index 0f32eca54..d5a6cbe2a 100644 --- a/crates/uv/src/lib.rs +++ b/crates/uv/src/lib.rs @@ -179,6 +179,7 @@ async fn run(cli: Cli) -> Result { .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 { .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 { .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 index ede666ca5..108dd5c50 100644 --- 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 index ec9c45b2c..2129d2ccd 100644 --- 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 "### diff --git a/docs/settings.md b/docs/settings.md index 461c783e9..197db5492 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -672,7 +672,7 @@ those that are downloaded and installed by uv. #### [`reinstall`](#reinstall) {: #reinstall } -Reinstall all packages, regardless of whether they're already installed. +Reinstall all packages, regardless of whether they're already installed. Implies `refresh`. **Default value**: `false` @@ -697,7 +697,8 @@ Reinstall all packages, regardless of whether they're already installed. #### [`reinstall-package`](#reinstall-package) {: #reinstall-package } -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`. **Default value**: `[]` @@ -2062,7 +2063,7 @@ mapped to `3.8.0`. #### [`reinstall`](#pip_reinstall) {: #pip_reinstall } -Reinstall all packages, regardless of whether they're already installed. +Reinstall all packages, regardless of whether they're already installed. Implies `refresh`. **Default value**: `false` @@ -2088,7 +2089,8 @@ Reinstall all packages, regardless of whether they're already installed. #### [`reinstall-package`](#pip_reinstall-package) {: #pip_reinstall-package } -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`. **Default value**: `[]` diff --git a/uv.schema.json b/uv.schema.json index eb8540c17..15153b007 100644 --- a/uv.schema.json +++ b/uv.schema.json @@ -253,14 +253,14 @@ ] }, "reinstall": { - "description": "Reinstall all packages, regardless of whether they're already installed.", + "description": "Reinstall all packages, regardless of whether they're already installed. Implies `refresh`.", "type": [ "boolean", "null" ] }, "reinstall-package": { - "description": "Reinstall a specific package, regardless of whether it's already installed.", + "description": "Reinstall a specific package, regardless of whether it's already installed. Implies `refresh-package`.", "type": [ "array", "null" @@ -845,14 +845,14 @@ ] }, "reinstall": { - "description": "Reinstall all packages, regardless of whether they're already installed.", + "description": "Reinstall all packages, regardless of whether they're already installed. Implies `refresh`.", "type": [ "boolean", "null" ] }, "reinstall-package": { - "description": "Reinstall a specific package, regardless of whether it's already installed.", + "description": "Reinstall a specific package, regardless of whether it's already installed. Implies `refresh-package`.", "type": [ "array", "null"