Revert change to automatically select the greatest patch match.

rust-lang · May 20, 2020 · 4f2bae9 · 4f2bae9
1 parent 5dde9cc
commit 4f2bae9
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 26 deletions.
diff --git a/src/cargo/core/registry.rs b/src/cargo/core/registry.rs
@@ -296,7 +296,7 @@ impl<'cfg> PackageRegistry<'cfg> {
                 let (summary, should_unlock) =
                     summary_for_patch(orig_patch, &locked, summaries, source).chain_err(|| {
                         format!(
-                            "patch for `{}` in `{}` did not resolve to any crates",
+                            "patch for `{}` in `{}` failed to resolve",
                             orig_patch.package_name(),
                             url,
                         )
@@ -754,21 +754,28 @@ fn summary_for_patch(
     if summaries.len() == 1 {
         return Ok((summaries.pop().unwrap(), None));
     }
-    let best_summary = |summaries: &mut Vec<Summary>| -> Summary {
-        // TODO: This could maybe honor -Zminimal-versions?
-        summaries.sort_by(|a, b| a.version().cmp(b.version()));
-        summaries.pop().unwrap()
-    };
     if summaries.len() > 1 {
-        let summary = best_summary(&mut summaries);
-        if let Some((_dep, lock_id)) = locked {
-            // I can't think of a scenario where this might happen (locked by
-            // definition should only match at most one summary). Maybe if the
-            // source is broken?
-            return Ok((summary, Some(*lock_id)));
-        } else {
-            return Ok((summary, None));
-        }
+        // TODO: In the future, it might be nice to add all of these
+        // candidates so that version selection would just pick the
+        // appropriate one. However, as this is currently structured, if we
+        // added these all as patches, the unselected versions would end up in
+        // the "unused patch" listing, and trigger a warning. It might take a
+        // fair bit of restructuring to make that work cleanly, and there
+        // isn't any demand at this time to support that.
+        let mut vers: Vec<_> = summaries.iter().map(|summary| summary.version()).collect();
+        vers.sort();
+        let versions: Vec<_> = vers.into_iter().map(|v| v.to_string()).collect();
+        anyhow::bail!(
+            "patch for `{}` in `{}` resolved to more than one candidate\n\
+            Found versions: {}\n\
+            Update the patch definition to select only one package.\n\
+            For example, add an `=` version requirement to the patch definition, \
+            such as `version = \"={}\"`.",
+            orig_patch.package_name(),
+            orig_patch.source_id(),
+            versions.join(", "),
+            versions.last().unwrap()
+        );
     }
     assert!(summaries.is_empty());
     // No summaries found, try to help the user figure out what is wrong.

diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs
@@ -1567,15 +1567,20 @@ fn too_many_matches() {
 
     // Picks 0.1.1, the most recent version.
     p.cargo("check")
+        .with_status(101)
         .with_stderr(
             "\
 [UPDATING] `[..]/alternative-registry` index
-[UPDATING] `[..]/registry` index
-[DOWNLOADING] crates ...
-[DOWNLOADED] bar v0.1.1 (registry `[..]/alternative-registry`)
-[CHECKING] bar v0.1.1 (registry `[..]/alternative-registry`)
-[CHECKING] foo v0.1.0 ([..]/foo)
-[FINISHED] [..]
+[ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
+
+Caused by:
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
+
+Caused by:
+  patch for `bar` in `registry `[..]/alternative-registry`` resolved to more than one candidate
+Found versions: 0.1.0, 0.1.1
+Update the patch definition to select only one package.
+For example, add an `=` version requirement to the patch definition, such as `version = \"=0.1.1\"`.
 ",
         )
         .run();
@@ -1611,7 +1616,7 @@ fn no_matches() {
 error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
 
 Caused by:
   The patch location `[..]/foo/bar` does not appear to contain any packages matching the name `bar`.
@@ -1650,7 +1655,7 @@ fn mismatched_version() {
 [ERROR] failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
 
 Caused by:
   The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, \
@@ -1760,7 +1765,7 @@ fn patch_walks_backwards_restricted() {
 error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index`
 
 Caused by:
-  patch for `bar` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates
+  patch for `bar` in `https://github.com/rust-lang/crates.io-index` failed to resolve
 
 Caused by:
   The patch location `[..]/foo/bar` contains a `bar` package with version `0.1.0`, but the patch definition requires `^0.1.1`.
@@ -1942,7 +1947,7 @@ fn can_update_with_alt_reg() {
                 bar = "0.1"
 
                 [patch.crates-io]
-                bar = { version = "0.1.1", registry = "alternative" }
+                bar = { version = "=0.1.1", registry = "alternative" }
             "#,
         )
         .file("src/lib.rs", "")
@@ -1967,12 +1972,42 @@ fn can_update_with_alt_reg() {
     // Should remain locked.
     p.cargo("check").with_stderr("[FINISHED] [..]").run();
 
+    // This does nothing, due to `=` requirement.
     p.cargo("update -p bar")
         .with_stderr(
             "\
 [UPDATING] `[..]/alternative-registry` index
 [UPDATING] `[..]/registry` index
-[UPDATING] bar v0.1.1 (registry `[..]/alternative-registry`) -> v0.1.2
+",
+        )
+        .run();
+
+    // Bump to 0.1.2.
+    p.change_file(
+        "Cargo.toml",
+        r#"
+            [package]
+            name = "foo"
+            version = "0.1.0"
+
+            [dependencies]
+            bar = "0.1"
+
+            [patch.crates-io]
+            bar = { version = "=0.1.2", registry = "alternative" }
+        "#,
+    );
+
+    p.cargo("check")
+        .with_stderr(
+            "\
+[UPDATING] `[..]/alternative-registry` index
+[UPDATING] `[..]/registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] bar v0.1.2 (registry `[..]/alternative-registry`)
+[CHECKING] bar v0.1.2 (registry `[..]/alternative-registry`)
+[CHECKING] foo v0.1.0 ([..]/foo)
+[FINISHED] [..]
 ",
         )
         .run();