From 00bf69be282d5105f4e8311d9655816c43a8deb7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 11 Nov 2024 22:38:05 -0500 Subject: [PATCH] Revert `uv.lock` changes when `uv add` fails (#9030) ## Summary If a `uv add` fails at the sync stage, we need to clean up the changes to the `uv.lock`, since it might've been edited during in the lock stage (which, by necessity, succeeded). As-is, we revert the `pyproject.toml` but not the `uv.lock`, so the two are out-of-sync. Closes https://github.com/astral-sh/uv/issues/9028. Closes https://github.com/astral-sh/uv/issues/7992. --- crates/uv/src/commands/project/add.rs | 58 ++++++--- crates/uv/src/commands/project/lock.rs | 11 ++ crates/uv/tests/it/edit.rs | 172 ++++++++++++++++++++++--- 3 files changed, 204 insertions(+), 37 deletions(-) diff --git a/crates/uv/src/commands/project/add.rs b/crates/uv/src/commands/project/add.rs index 05b57ff1509c..db76cc2e7a3c 100644 --- a/crates/uv/src/commands/project/add.rs +++ b/crates/uv/src/commands/project/add.rs @@ -1,5 +1,6 @@ use std::collections::hash_map::Entry; use std::fmt::Write; +use std::io; use std::path::{Path, PathBuf}; use anyhow::{bail, Context, Result}; @@ -44,7 +45,7 @@ use crate::commands::pip::loggers::{ use crate::commands::pip::operations::Modifications; use crate::commands::project::lock::LockMode; use crate::commands::project::{ - init_script_python_requirement, validate_script_requires_python, ProjectError, + init_script_python_requirement, lock, validate_script_requires_python, ProjectError, ProjectInterpreter, ScriptPython, }; use crate::commands::reporters::{PythonDownloadReporter, ResolverReporter}; @@ -618,8 +619,10 @@ pub(crate) async fn add( } // Store the content prior to any modifications. - let existing = project.pyproject_toml().as_ref().to_vec(); - let root = project.root().to_path_buf(); + let project_root = project.root().to_path_buf(); + let workspace_root = project.workspace().install_path().clone(); + let existing_pyproject_toml = project.pyproject_toml().as_ref().to_vec(); + let existing_uv_lock = lock::read_bytes(project.workspace()).await?; // Update the `pypackage.toml` in-memory. let project = project @@ -628,12 +631,18 @@ pub(crate) async fn add( // Set the Ctrl-C handler to revert changes on exit. let _ = ctrlc::set_handler({ - let root = root.clone(); - let existing = existing.clone(); + let project_root = project_root.clone(); + let workspace_root = workspace_root.clone(); + let existing_pyproject_toml = existing_pyproject_toml.clone(); + let existing_uv_lock = existing_uv_lock.clone(); move || { - // Revert the changes to the `pyproject.toml`, if necessary. if modified { - let _ = fs_err::write(root.join("pyproject.toml"), &existing); + let _ = revert( + &project_root, + &workspace_root, + &existing_pyproject_toml, + existing_uv_lock.as_deref(), + ); } #[allow(clippy::exit, clippy::cast_possible_wrap)] @@ -667,9 +676,13 @@ pub(crate) async fn add( { Ok(()) => Ok(ExitStatus::Success), Err(err) => { - // Revert the changes to the `pyproject.toml`, if necessary. if modified { - fs_err::write(root.join("pyproject.toml"), &existing)?; + let _ = revert( + &project_root, + &workspace_root, + &existing_pyproject_toml, + existing_uv_lock.as_deref(), + ); } match err { @@ -703,13 +716,7 @@ pub(crate) async fn add( diagnostics::build(dist, err); Ok(ExitStatus::Failure) } - err => { - // Revert the changes to the `pyproject.toml`, if necessary. - if modified { - fs_err::write(root.join("pyproject.toml"), &existing)?; - } - Err(err.into()) - } + err => Err(err.into()), } } } @@ -943,6 +950,25 @@ async fn lock_and_sync( Ok(()) } +/// Revert the changes to the `pyproject.toml` and `uv.lock`, if necessary. +fn revert( + project_root: &Path, + workspace_root: &Path, + pyproject_toml: &[u8], + uv_lock: Option<&[u8]>, +) -> Result<(), io::Error> { + debug!("Reverting changes to `pyproject.toml`"); + let () = fs_err::write(project_root.join("pyproject.toml"), pyproject_toml)?; + if let Some(uv_lock) = uv_lock.as_ref() { + debug!("Reverting changes to `uv.lock`"); + let () = fs_err::write(workspace_root.join("uv.lock"), uv_lock)?; + } else { + debug!("Removing `uv.lock`"); + let () = fs_err::remove_file(workspace_root.join("uv.lock"))?; + } + Ok(()) +} + /// Augment a user-provided requirement by attaching any specification data that was provided /// separately from the requirement itself (e.g., `--branch main`). fn augment_requirement( diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index 3f3866b87b58..01a5833566f7 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -1009,6 +1009,17 @@ pub(crate) async fn read(workspace: &Workspace) -> Result, ProjectE } } +/// Read the lockfile from the workspace as bytes. +/// +/// Returns `Ok(None)` if the lockfile does not exist. +pub(crate) async fn read_bytes(workspace: &Workspace) -> Result>, ProjectError> { + match fs_err::tokio::read(&workspace.install_path().join("uv.lock")).await { + Ok(encoded) => Ok(Some(encoded)), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(err) => Err(err.into()), + } +} + /// Reports on the versions that were upgraded in the new lockfile. /// /// Returns `true` if any upgrades were reported. diff --git a/crates/uv/tests/it/edit.rs b/crates/uv/tests/it/edit.rs index 04f25d4ad4f1..622771e44260 100644 --- a/crates/uv/tests/it/edit.rs +++ b/crates/uv/tests/it/edit.rs @@ -5506,57 +5506,77 @@ fn add_git_to_script() -> Result<()> { Ok(()) } -/// Revert changes to a `pyproject.toml` the `add` fails. +/// Revert changes to the `pyproject.toml` and `uv.lock` when the `add` operation fails. #[test] fn fail_to_add_revert_project() -> Result<()> { let context = TestContext::new("3.12"); - let pyproject_toml = context.temp_dir.child("pyproject.toml"); - pyproject_toml.write_str(indoc! {r#" + context + .temp_dir + .child("pyproject.toml") + .write_str(indoc! {r#" [project] - name = "project" + name = "parent" version = "0.1.0" requires-python = ">=3.12" dependencies = [] + "#})?; + + // Add a dependency on a package that declares static metadata (so can always resolve), but + // can't be installed. + let pyproject_toml = context.temp_dir.child("child/pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "child" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["iniconfig"] [build-system] requires = ["setuptools>=42"] build-backend = "setuptools.build_meta" "#})?; + context + .temp_dir + .child("src") + .child("child") + .child("__init__.py") + .touch()?; + context + .temp_dir + .child("child") + .child("setup.py") + .write_str("1/0")?; - // Adding `pytorch==1.0.2` should produce an error let filters = std::iter::once((r"exit code: 1", "exit status: 1")) .chain(context.filters()) .collect::>(); - uv_snapshot!(filters, context.add().arg("pytorch==1.0.2"), @r###" + uv_snapshot!(filters, context.add().arg("./child"), @r###" success: false exit_code: 2 ----- stdout ----- ----- stderr ----- - Resolved 2 packages in [TIME] + Resolved 3 packages in [TIME] error: Failed to prepare distributions - Caused by: Failed to download and build `pytorch==1.0.2` - Caused by: Build backend failed to build wheel through `build_wheel` (exit status: 1) + Caused by: Failed to build `child @ file://[TEMP_DIR]/child` + Caused by: Build backend failed to determine requirements with `build_wheel()` (exit status: 1) [stderr] Traceback (most recent call last): - File "", line 11, in - File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 410, in build_wheel - return self._build_with_temp_dir( - ^^^^^^^^^^^^^^^^^^^^^^^^^^ - File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 395, in _build_with_temp_dir + File "", line 14, in + File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 325, in get_requires_for_build_wheel + return self._get_build_requires(config_settings, requirements=['wheel']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 295, in _get_build_requires self.run_setup() - File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 487, in run_setup - super().run_setup(setup_script=setup_script) File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 311, in run_setup exec(code, locals()) - File "", line 15, in - Exception: You tried to install "pytorch". The package named for PyTorch is "torch" - + File "", line 1, in + ZeroDivisionError: division by zero "###); - let pyproject_toml = context.read("pyproject.toml"); + let pyproject_toml = fs_err::read_to_string(context.temp_dir.join("pyproject.toml"))?; insta::with_settings!({ filters => context.filters(), @@ -5564,18 +5584,128 @@ fn fail_to_add_revert_project() -> Result<()> { assert_snapshot!( pyproject_toml, @r###" [project] - name = "project" + name = "parent" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = [] + "### + ); + }); + + // The lockfile should not exist, even though resolution succeeded. + assert!(!context.temp_dir.join("uv.lock").exists()); + + Ok(()) +} + +/// Revert changes to the `pyproject.toml` and `uv.lock` when the `add` operation fails. +/// +/// In this case, the project has an existing lockfile. +#[test] +fn fail_to_edit_revert_project() -> Result<()> { + let context = TestContext::new("3.12"); + + context + .temp_dir + .child("pyproject.toml") + .write_str(indoc! {r#" + [project] + name = "parent" version = "0.1.0" requires-python = ">=3.12" dependencies = [] + "#})?; + + uv_snapshot!(context.filters(), context.add().arg("iniconfig"), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved 2 packages in [TIME] + Prepared 1 package in [TIME] + Installed 1 package in [TIME] + + iniconfig==2.0.0 + "###); + + let before = fs_err::read_to_string(context.temp_dir.join("uv.lock"))?; + + // Add a dependency on a package that declares static metadata (so can always resolve), but + // can't be installed. + let pyproject_toml = context.temp_dir.child("child/pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "child" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["iniconfig"] [build-system] requires = ["setuptools>=42"] build-backend = "setuptools.build_meta" + "#})?; + context + .temp_dir + .child("src") + .child("child") + .child("__init__.py") + .touch()?; + context + .temp_dir + .child("child") + .child("setup.py") + .write_str("1/0")?; + + let filters = std::iter::once((r"exit code: 1", "exit status: 1")) + .chain(context.filters()) + .collect::>(); + uv_snapshot!(filters, context.add().arg("./child"), @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + Resolved 3 packages in [TIME] + error: Failed to prepare distributions + Caused by: Failed to build `child @ file://[TEMP_DIR]/child` + Caused by: Build backend failed to determine requirements with `build_wheel()` (exit status: 1) + + [stderr] + Traceback (most recent call last): + File "", line 14, in + File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 325, in get_requires_for_build_wheel + return self._get_build_requires(config_settings, requirements=['wheel']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 295, in _get_build_requires + self.run_setup() + File "[CACHE_DIR]/builds-v0/[TMP]/build_meta.py", line 311, in run_setup + exec(code, locals()) + File "", line 1, in + ZeroDivisionError: division by zero + "###); + + let pyproject_toml = fs_err::read_to_string(context.temp_dir.join("pyproject.toml"))?; + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + pyproject_toml, @r###" + [project] + name = "parent" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = [ + "iniconfig>=2.0.0", + ] "### ); }); + // The lockfile should exist, but be unchanged. + let after = fs_err::read_to_string(context.temp_dir.join("uv.lock"))?; + assert_eq!(before, after); + Ok(()) }