Auto merge of #12602 - epage:resolver, r=Eh2406

fix(resolver): Make resolver behavior independent of package order

This address one of the problems mentioned in #12599

The intent behind the `path_pkg` check is to make sure we update
workspace members in the lockfile if their version number changed.
In this case, we don't need to recursively walk, because the change
doesn't affect dependencies.  However, we also shouldn't *prevent*
recursive walks which is what we are doing today, both for packages
marked to keep and for packages that have been "poisoned".  So we fix
this by moving that call after all recursive adds are complete so as to
not interfere with them.

This should not affect `Cargo.lock` at rest, so no upgrade compatibility concerns.

This just allows more packages to be considered available to change which can prevent unclear failures.

The main case I can think of that this does something "undesirable" is when wanting to prevent another "bug" from manifesting: the updating of git dependencies when updating workspace members (#12599).  I think I'm ok with that as that needs to be looked into separately.

src/cargo/ops/resolve.rs

@@ -632,23 +632,11 @@ fn register_previous_locks(
     // however, nothing else in the dependency graph depends on `log` and the
     // newer version of `serde` requires a new version of `log` it'll get pulled
     // in (as we didn't accidentally lock it to an old version).
-    //
-    // Additionally, here we process all path dependencies listed in the previous
-    // resolve. They can not only have their dependencies change but also
-    // the versions of the package change as well. If this ends up happening
-    // then we want to make sure we don't lock a package ID node that doesn't
-    // actually exist. Note that we don't do transitive visits of all the
-    // package's dependencies here as that'll be covered below to poison those
-    // if they changed.
     let mut avoid_locking = HashSet::new();
     registry.add_to_yanked_whitelist(resolve.iter().filter(keep));
     for node in resolve.iter() {
         if !keep(&node) {
             add_deps(resolve, node, &mut avoid_locking);
-        } else if let Some(pkg) = path_pkg(node.source_id()) {
-            if pkg.package_id() != node {
-                avoid_locking.insert(node);
-            }
         }
     }
 
@@ -741,6 +729,24 @@ fn register_previous_locks(
         }
     }
 
+    // Additionally, here we process all path dependencies listed in the previous
+    // resolve. They can not only have their dependencies change but also
+    // the versions of the package change as well. If this ends up happening
+    // then we want to make sure we don't lock a package ID node that doesn't
+    // actually exist. Note that we don't do transitive visits of all the
+    // package's dependencies here as that'll be covered below to poison those
+    // if they changed.
+    //
+    // This must come after all other `add_deps` calls to ensure it recursively walks the tree when
+    // called.
+    for node in resolve.iter() {
+        if let Some(pkg) = path_pkg(node.source_id()) {
+            if pkg.package_id() != node {
+                avoid_locking.insert(node);
+            }
+        }
+    }
+
     // Alright now that we've got our new, fresh, shiny, and refined `keep`
     // function let's put it to action. Take a look at the previous lock file,
     // filter everything by this callback, and then shove everything else into

tests/testsuite/update.rs

@@ -1,7 +1,7 @@
 //! Tests for the `cargo update` command.
 
 use cargo_test_support::registry::Package;
-use cargo_test_support::{basic_manifest, project};
+use cargo_test_support::{basic_lib_manifest, basic_manifest, git, project};
 
 #[cargo_test]
 fn minor_update_two_places() {
@@ -910,3 +910,185 @@ required by package `foo v0.1.0 ([ROOT]/foo)`
         )
         .run();
 }
+
+#[cargo_test]
+fn update_only_members_order_one() {
+    let git_project = git::new("rustdns", |project| {
+        project
+            .file("Cargo.toml", &basic_lib_manifest("rustdns"))
+            .file("src/lib.rs", "pub fn bar() {}")
+    });
+
+    let workspace_toml = format!(
+        r#"
+[workspace.package]
+version = "2.29.8"
+edition = "2021"
+publish = false
+
+[workspace]
+members = [
+    "rootcrate",
+    "subcrate",
+]
+resolver = "2"
+
+[workspace.dependencies]
+# Internal crates
+subcrate = {{ version = "*", path = "./subcrate" }}
+
+# External dependencies
+rustdns = {{ version = "0.5.0", default-features = false, git = "{}" }}
+                "#,
+        git_project.url()
+    );
+    let p = project()
+        .file("Cargo.toml", &workspace_toml)
+        .file(
+            "rootcrate/Cargo.toml",
+            r#"
+[package]
+name = "rootcrate"
+version.workspace = true
+edition.workspace = true
+publish.workspace = true
+
+[dependencies]
+subcrate.workspace = true
+"#,
+        )
+        .file("rootcrate/src/main.rs", "fn main() {}")
+        .file(
+            "subcrate/Cargo.toml",
+            r#"
+[package]
+name = "subcrate"
+version.workspace = true
+edition.workspace = true
+publish.workspace = true
+
+[dependencies]
+rustdns.workspace = true
+"#,
+        )
+        .file("subcrate/src/lib.rs", "pub foo() {}")
+        .build();
+
+    // First time around we should compile both foo and bar
+    p.cargo("generate-lockfile")
+        .with_stderr(&format!(
+            "[UPDATING] git repository `{}`\n",
+            git_project.url(),
+        ))
+        .run();
+    // Modify a file manually, shouldn't trigger a recompile
+    git_project.change_file("src/lib.rs", r#"pub fn bar() { println!("hello!"); }"#);
+    // Commit the changes and make sure we don't trigger a recompile because the
+    // lock file says not to change
+    let repo = git2::Repository::open(&git_project.root()).unwrap();
+    git::add(&repo);
+    git::commit(&repo);
+    p.change_file("Cargo.toml", &workspace_toml.replace("2.29.8", "2.29.81"));
+
+    p.cargo("update -p rootcrate")
+        .with_stderr(&format!(
+            "\
+[UPDATING] git repository `{}`
+[UPDATING] rootcrate v2.29.8 ([CWD]/rootcrate) -> v2.29.81
+[UPDATING] rustdns v0.5.0 ([..]) -> [..]
+[UPDATING] subcrate v2.29.8 ([CWD]/subcrate) -> v2.29.81",
+            git_project.url(),
+        ))
+        .run();
+}
+
+#[cargo_test]
+fn update_only_members_order_two() {
+    let git_project = git::new("rustdns", |project| {
+        project
+            .file("Cargo.toml", &basic_lib_manifest("rustdns"))
+            .file("src/lib.rs", "pub fn bar() {}")
+    });
+
+    let workspace_toml = format!(
+        r#"
+[workspace.package]
+version = "2.29.8"
+edition = "2021"
+publish = false
+
+[workspace]
+members = [
+    "crate2",
+    "crate1",
+]
+resolver = "2"
+
+[workspace.dependencies]
+# Internal crates
+crate1 = {{ version = "*", path = "./crate1" }}
+
+# External dependencies
+rustdns = {{ version = "0.5.0", default-features = false, git = "{}" }}
+                "#,
+        git_project.url()
+    );
+    let p = project()
+        .file("Cargo.toml", &workspace_toml)
+        .file(
+            "crate2/Cargo.toml",
+            r#"
+[package]
+name = "crate2"
+version.workspace = true
+edition.workspace = true
+publish.workspace = true
+
+[dependencies]
+crate1.workspace = true
+"#,
+        )
+        .file("crate2/src/main.rs", "fn main() {}")
+        .file(
+            "crate1/Cargo.toml",
+            r#"
+[package]
+name = "crate1"
+version.workspace = true
+edition.workspace = true
+publish.workspace = true
+
+[dependencies]
+rustdns.workspace = true
+"#,
+        )
+        .file("crate1/src/lib.rs", "pub foo() {}")
+        .build();
+
+    // First time around we should compile both foo and bar
+    p.cargo("generate-lockfile")
+        .with_stderr(&format!(
+            "[UPDATING] git repository `{}`\n",
+            git_project.url(),
+        ))
+        .run();
+    // Modify a file manually, shouldn't trigger a recompile
+    git_project.change_file("src/lib.rs", r#"pub fn bar() { println!("hello!"); }"#);
+    // Commit the changes and make sure we don't trigger a recompile because the
+    // lock file says not to change
+    let repo = git2::Repository::open(&git_project.root()).unwrap();
+    git::add(&repo);
+    git::commit(&repo);
+    p.change_file("Cargo.toml", &workspace_toml.replace("2.29.8", "2.29.81"));
+
+    p.cargo("update -p crate2")
+        .with_stderr(&format!(
+            "\
+[UPDATING] git repository `{}`
+[UPDATING] crate1 v2.29.8 ([CWD]/crate1) -> v2.29.81
+[UPDATING] crate2 v2.29.8 ([CWD]/crate2) -> v2.29.81
+[UPDATING] rustdns v0.5.0 ([..]) -> [..]",
+            git_project.url(),
+        ))
+        .run();
+}