From 0af2f65ce8f20835d4cb2f82501e5d0453b3d6ab Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 30 Aug 2023 13:12:52 -0500 Subject: [PATCH] fix(resolver): Make resolver behavior independent of package order This addresses the ordering issue of for one of the problems from #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. --- src/cargo/ops/resolve.rs | 30 ++++++++++++++++++------------ tests/testsuite/update.rs | 9 ++++++--- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/cargo/ops/resolve.rs b/src/cargo/ops/resolve.rs index d528e8a0441..8fe60d593af 100644 --- a/src/cargo/ops/resolve.rs +++ b/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 diff --git a/tests/testsuite/update.rs b/tests/testsuite/update.rs index ab178a41f32..fe1d86bd763 100644 --- a/tests/testsuite/update.rs +++ b/tests/testsuite/update.rs @@ -1082,10 +1082,13 @@ rustdns.workspace = true p.change_file("Cargo.toml", &workspace_toml.replace("2.29.8", "2.29.81")); p.cargo("update -p crate2") - .with_stderr( + .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] crate2 v2.29.8 ([CWD]/crate2) -> v2.29.81 +[UPDATING] rustdns v0.5.0 ([..]) -> [..]", + git_project.url(), + )) .run(); }