Skip to content

Commit

Permalink
Revert uv.lock changes when uv add fails (#9030)
Browse files Browse the repository at this point in the history
## 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 #9028.
Closes #7992.
  • Loading branch information
charliermarsh authored Nov 12, 2024
1 parent de9dc39 commit 00bf69b
Show file tree
Hide file tree
Showing 3 changed files with 204 additions and 37 deletions.
58 changes: 42 additions & 16 deletions crates/uv/src/commands/project/add.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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
Expand All @@ -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)]
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()),
}
}
}
Expand Down Expand Up @@ -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(
Expand Down
11 changes: 11 additions & 0 deletions crates/uv/src/commands/project/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,17 @@ pub(crate) async fn read(workspace: &Workspace) -> Result<Option<Lock>, 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<Option<Vec<u8>>, 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.
Expand Down
172 changes: 151 additions & 21 deletions crates/uv/tests/it/edit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5506,76 +5506,206 @@ 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::<Vec<_>>();
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 "<string>", line 11, in <module>
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 "<string>", line 14, in <module>
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 "<string>", line 15, in <module>
Exception: You tried to install "pytorch". The package named for PyTorch is "torch"
File "<string>", line 1, in <module>
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(),
}, {
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::<Vec<_>>();
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 "<string>", line 14, in <module>
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 "<string>", line 1, in <module>
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(())
}

Expand Down

0 comments on commit 00bf69b

Please sign in to comment.