Auto merge of #10902 - epage:lock, r=ehuss

fix(add): Update the lock file

This is done in the command, rather than in the op,
- To consistently construct the `Workspace`
- It is more composable as an API

A downside is we update the git dependencies a second time and any sources we didn't initially update.

Unlike the proposal in the attached issue, this does not roll back on error.
- For some errors, the user might want to debug what went wrong.  Losing the intermediate state makes that difficult
- Rollback adds its own complications and risks, including since its
  non-atomic

We've already tried to address most potential errors during `cargo add`s processing.  To meet this desire, we now error if `--locked` is passed in and we would change the manifest.  See that individual commit's message for more details.

Fixes #10901

src/bin/cargo/commands/add.rs

@@ -7,6 +7,7 @@ use cargo::ops::cargo_add::add;
 use cargo::ops::cargo_add::AddOptions;
 use cargo::ops::cargo_add::DepOp;
 use cargo::ops::cargo_add::DepTable;
+use cargo::ops::resolve_ws;
 use cargo::util::command_prelude::*;
 use cargo::util::interning::InternedString;
 use cargo::CargoResult;
@@ -193,6 +194,12 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     };
     add(&ws, &options)?;
 
+    if !dry_run {
+        // Reload the workspace since we've changed dependencies
+        let ws = args.workspace(config)?;
+        resolve_ws(&ws)?;
+    }
+
     Ok(())
 }
 

src/cargo/ops/cargo_add/manifest.rs

@@ -238,8 +238,7 @@ impl str::FromStr for Manifest {
 
 impl std::fmt::Display for Manifest {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let s = self.data.to_string();
-        s.fmt(f)
+        self.data.fmt(f)
     }
 }
 
@@ -433,6 +432,12 @@ impl LocalManifest {
     }
 }
 
+impl std::fmt::Display for LocalManifest {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.manifest.fmt(f)
+    }
+}
+
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
 enum DependencyStatus {
     None,

src/cargo/ops/cargo_add/mod.rs

@@ -62,6 +62,7 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
 
     let manifest_path = options.spec.manifest_path().to_path_buf();
     let mut manifest = LocalManifest::try_new(&manifest_path)?;
+    let original_raw_manifest = manifest.to_string();
     let legacy = manifest.get_legacy_sections();
     if !legacy.is_empty() {
         anyhow::bail!(
@@ -142,6 +143,16 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
         }
     }
 
+    if options.config.locked() {
+        let new_raw_manifest = manifest.to_string();
+        if original_raw_manifest != new_raw_manifest {
+            anyhow::bail!(
+                "the manifest file {} needs to be updated but --locked was passed to prevent this",
+                manifest.path.display()
+            );
+        }
+    }
+
     if options.dry_run {
         options.config.shell().warn("aborting add due to dry run")?;
     } else {

tests/testsuite/cargo_add/build_prefer_existing_version/in/dependency/Cargo.toml

@@ -3,3 +3,7 @@
 [package]
 name = "cargo-list-test-fixture-dependency"
 version = "0.0.0"
+
+[features]
+one = []
+two = []

tests/testsuite/cargo_add/build_prefer_existing_version/out/dependency/Cargo.toml

@@ -3,3 +3,7 @@
 [package]
 name = "cargo-list-test-fixture-dependency"
 version = "0.0.0"
+
+[features]
+one = []
+two = []

tests/testsuite/cargo_add/build_prefer_existing_version/stderr.log

@@ -1 +1,4 @@
       Adding cargo-list-test-fixture-dependency (local) to build-dependencies.
+             Features:
+             - one
+             - two

tests/testsuite/cargo_add/dev_prefer_existing_version/in/dependency/Cargo.toml

@@ -3,3 +3,7 @@
 [package]
 name = "cargo-list-test-fixture-dependency"
 version = "0.0.0"
+
+[features]
+one = []
+two = []

tests/testsuite/cargo_add/dev_prefer_existing_version/out/dependency/Cargo.toml

@@ -3,3 +3,7 @@
 [package]
 name = "cargo-list-test-fixture-dependency"
 version = "0.0.0"
+
+[features]
+one = []
+two = []

tests/testsuite/cargo_add/dev_prefer_existing_version/stderr.log

@@ -1 +1,4 @@
       Adding cargo-list-test-fixture-dependency (local) to dev-dependencies.
+             Features:
+             - one
+             - two

tests/testsuite/cargo_add/git/stderr.log

@@ -1,2 +1,3 @@
     Updating git repository `[ROOTURL]/git-package`
       Adding git-package (git) to dependencies.
+    Updating git repository `[ROOTURL]/git-package`

tests/testsuite/cargo_add/git_branch/stderr.log

@@ -1,2 +1,3 @@
     Updating git repository `[ROOTURL]/git-package`
       Adding git-package (git) to dependencies.
+    Updating git repository `[ROOTURL]/git-package`

tests/testsuite/cargo_add/git_dev/stderr.log

@@ -1,2 +1,3 @@
     Updating git repository `[ROOTURL]/git-package`
       Adding git-package (git) to dev-dependencies.
+    Updating git repository `[ROOTURL]/git-package`

tests/testsuite/cargo_add/git_inferred_name/stderr.log

@@ -1,3 +1,4 @@
     Updating git repository `[ROOTURL]/git-package`
     Updating git repository `[ROOTURL]/git-package`
       Adding git-package (git) to dependencies.
+    Updating git repository `[ROOTURL]/git-package`

tests/testsuite/cargo_add/git_multiple_names/stderr.log

@@ -1,3 +1,4 @@
     Updating git repository `[ROOTURL]/git-package`
       Adding my-package1 (git) to dependencies.
       Adding my-package2 (git) to dependencies.
+    Updating git repository `[ROOTURL]/git-package`

tests/testsuite/cargo_add/git_registry/mod.rs

@@ -32,7 +32,7 @@ fn git_registry() {
         ])
         .current_dir(cwd)
         .assert()
-        .success()
+        .failure()
         .stdout_matches_path(curr_dir!().join("stdout.log"))
         .stderr_matches_path(curr_dir!().join("stderr.log"));
 

tests/testsuite/cargo_add/git_registry/stderr.log

@@ -1,2 +1,6 @@
     Updating git repository `[ROOTURL]/versioned-package`
       Adding versioned-package (git) to dependencies.
+error: failed to parse manifest at `[ROOT]/case/Cargo.toml`
+
+Caused by:
+  dependency (versioned-package) specification is ambiguous. Only one of `git` or `registry` is allowed.

tests/testsuite/cargo_add/git_rev/stderr.log

@@ -1,2 +1,3 @@
     Updating git repository `[ROOTURL]/git-package`
       Adding git-package (git) to dependencies.
+    Updating git repository `[ROOTURL]/git-package`

tests/testsuite/cargo_add/git_tag/stderr.log

@@ -1,2 +1,3 @@
     Updating git repository `[ROOTURL]/git-package`
       Adding git-package (git) to dependencies.
+    Updating git repository `[ROOTURL]/git-package`

tests/testsuite/cargo_add/list_features_path/in/dependency/Cargo.toml

@@ -3,8 +3,7 @@ name = "your-face"
 version = "0.1.3"
 
 [dependencies]
-toml_edit = "0.1.5"
-atty = "0.2.13"
+my-package = "0.1.1"
 optional-dependency = { path = "../optional", optional = true }
 
 [features]

tests/testsuite/cargo_add/list_features_path/in/optional/Cargo.toml

@@ -1,7 +1,6 @@
 [package]
-name = "optional-dep"
+name = "optional-dependency"
 version = "0.1.3"
 
 [dependencies]
-toml_edit = "0.1.5"
-atty = "0.2.13"
+my-package = "0.1.1"

tests/testsuite/cargo_add/list_features_path/out/dependency/Cargo.toml

@@ -3,8 +3,7 @@ name = "your-face"
 version = "0.1.3"
 
 [dependencies]
-toml_edit = "0.1.5"
-atty = "0.2.13"
+my-package = "0.1.1"
 optional-dependency = { path = "../optional", optional = true }
 
 [features]

tests/testsuite/cargo_add/list_features_path/stderr.log

@@ -4,3 +4,4 @@
              + nose
              - eyes
              - optional-dependency
+    Updating `dummy-registry` index

tests/testsuite/cargo_add/list_features_path_no_default/in/dependency/Cargo.toml

@@ -3,8 +3,7 @@ name = "your-face"
 version = "0.1.3"
 
 [dependencies]
-toml_edit = "0.1.5"
-atty = "0.2.13"
+my-package = "0.1.1"
 optional-dependency = { path = "../optional", optional = true }
 
 [features]

tests/testsuite/cargo_add/list_features_path_no_default/in/optional/Cargo.toml

@@ -1,7 +1,6 @@
 [package]
-name = "optional-dep"
+name = "optional-dependency"
 version = "0.1.3"
 
 [dependencies]
-toml_edit = "0.1.5"
-atty = "0.2.13"
+my-package = "0.1.1"

tests/testsuite/cargo_add/list_features_path_no_default/out/dependency/Cargo.toml

@@ -3,8 +3,7 @@ name = "your-face"
 version = "0.1.3"
 
 [dependencies]
-toml_edit = "0.1.5"
-atty = "0.2.13"
+my-package = "0.1.1"
 optional-dependency = { path = "../optional", optional = true }
 
 [features]

tests/testsuite/cargo_add/list_features_path_no_default/stderr.log

@@ -4,3 +4,4 @@
              - mouth
              - nose
              - optional-dependency
+    Updating `dummy-registry` index

tests/testsuite/cargo_add/locked_changed/in

@@ -0,0 +1 @@
+../add-basic.in

tests/testsuite/cargo_add/locked_changed/mod.rs

@@ -0,0 +1,25 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::prelude::*;
+use cargo_test_support::Project;
+
+use crate::cargo_add::init_registry;
+use cargo_test_support::curr_dir;
+
+#[cargo_test]
+fn locked_changed() {
+    init_registry();
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = project.root();
+    let cwd = &project_root;
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg("add")
+        .arg_line("my-package --locked")
+        .current_dir(cwd)
+        .assert()
+        .failure()
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+
+    assert_ui().subset_matches(curr_dir!().join("out"), &project_root);
+}

tests/testsuite/cargo_add/locked_changed/out/Cargo.toml

@@ -0,0 +1,5 @@
+[workspace]
+
+[package]
+name = "cargo-list-test-fixture"
+version = "0.0.0"

tests/testsuite/cargo_add/locked_changed/stderr.log

@@ -0,0 +1,3 @@
+    Updating `dummy-registry` index
+      Adding my-package v99999.0.0 to dependencies.
+error: the manifest file [ROOT]/case/Cargo.toml needs to be updated but --locked was passed to prevent this

tests/testsuite/cargo_add/locked_unchanged/in/Cargo.toml

@@ -0,0 +1,8 @@
+[workspace]
+
+[package]
+name = "cargo-list-test-fixture"
+version = "0.0.0"
+
+[dependencies]
+my-package = "99999.0.0"

tests/testsuite/cargo_add/locked_unchanged/mod.rs

@@ -0,0 +1,25 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::prelude::*;
+use cargo_test_support::Project;
+
+use crate::cargo_add::init_registry;
+use cargo_test_support::curr_dir;
+
+#[cargo_test]
+fn locked_unchanged() {
+    init_registry();
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = project.root();
+    let cwd = &project_root;
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg("add")
+        .arg_line("my-package --locked")
+        .current_dir(cwd)
+        .assert()
+        .success()
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+
+    assert_ui().subset_matches(curr_dir!().join("out"), &project_root);
+}

tests/testsuite/cargo_add/locked_unchanged/out/Cargo.toml

@@ -0,0 +1,8 @@
+[workspace]
+
+[package]
+name = "cargo-list-test-fixture"
+version = "0.0.0"
+
+[dependencies]
+my-package = "99999.0.0"

tests/testsuite/cargo_add/locked_unchanged/stderr.log

@@ -0,0 +1,2 @@
+    Updating `dummy-registry` index
+      Adding my-package v99999.0.0 to dependencies.

tests/testsuite/cargo_add/lockfile_updated/in/Cargo.toml

@@ -0,0 +1,8 @@
+[workspace]
+
+[package]
+name = "cargo-list-test-fixture"
+version = "0.0.0"
+
+[dependencies]
+unrelateed-crate = "0.2.0"

tests/testsuite/cargo_add/lockfile_updated/mod.rs

@@ -0,0 +1,25 @@
+use cargo_test_support::compare::assert_ui;
+use cargo_test_support::prelude::*;
+use cargo_test_support::Project;
+
+use crate::cargo_add::init_registry;
+use cargo_test_support::curr_dir;
+
+#[cargo_test]
+fn lockfile_updated() {
+    init_registry();
+    let project = Project::from_template(curr_dir!().join("in"));
+    let project_root = project.root();
+    let cwd = &project_root;
+
+    snapbox::cmd::Command::cargo_ui()
+        .arg("add")
+        .arg_line("my-package")
+        .current_dir(cwd)
+        .assert()
+        .success()
+        .stdout_matches_path(curr_dir!().join("stdout.log"))
+        .stderr_matches_path(curr_dir!().join("stderr.log"));
+
+    assert_ui().subset_matches(curr_dir!().join("out"), &project_root);
+}