Fix cargo install with a semver metadata version.

Author: ehuss

src/cargo/core/package_id.rs

@@ -31,11 +31,22 @@ struct PackageIdInner {
     source_id: SourceId,
 }
 
-// Custom equality that uses full equality of SourceId, rather than its custom equality.
+// Custom equality that uses full equality of SourceId, rather than its custom equality,
+// and Version, which usually ignores `build` metadata.
+//
+// The `build` part of the version is usually ignored (like a "comment").
+// However, there are some cases where it is important. The download path from
+// a registry includes the build metadata, and Cargo uses PackageIds for
+// creating download paths. Including it here prevents the PackageId interner
+// from getting poisoned with PackageIds where that build metadata is missing.
 impl PartialEq for PackageIdInner {
     fn eq(&self, other: &Self) -> bool {
         self.name == other.name
-            && self.version == other.version
+            && self.version.major == other.version.major
+            && self.version.minor == other.version.minor
+            && self.version.patch == other.version.patch
+            && self.version.pre == other.version.pre
+            && self.version.build == other.version.build
             && self.source_id.full_eq(other.source_id)
     }
 }
@@ -44,7 +55,11 @@ impl PartialEq for PackageIdInner {
 impl Hash for PackageIdInner {
     fn hash<S: hash::Hasher>(&self, into: &mut S) {
         self.name.hash(into);
-        self.version.hash(into);
+        self.version.major.hash(into);
+        self.version.minor.hash(into);
+        self.version.patch.hash(into);
+        self.version.pre.hash(into);
+        self.version.build.hash(into);
         self.source_id.full_hash(into);
     }
 }
@@ -97,6 +112,8 @@ impl PartialEq for PackageId {
         if ptr::eq(self.inner, other.inner) {
             return true;
         }
+        // This is here so that PackageId uses SourceId's and Version's idea
+        // of equality. PackageIdInner uses a more exact notion of equality.
         self.inner.name == other.inner.name
             && self.inner.version == other.inner.version
             && self.inner.source_id == other.inner.source_id
@@ -105,6 +122,9 @@ impl PartialEq for PackageId {
 
 impl Hash for PackageId {
     fn hash<S: hash::Hasher>(&self, state: &mut S) {
+        // This is here (instead of derived) so that PackageId uses SourceId's
+        // and Version's idea of equality. PackageIdInner uses a more exact
+        // notion of hashing.
         self.inner.name.hash(state);
         self.inner.version.hash(state);
         self.inner.source_id.hash(state);
@@ -166,6 +186,12 @@ impl PackageId {
         }
     }
 
+    /// Returns a value that implements a "stable" hashable value.
+    ///
+    /// Stable hashing removes the path prefix of the workspace from path
+    /// packages. This helps with reproducible builds, since this hash is part
+    /// of the symbol metadata, and we don't want the absolute path where the
+    /// build is performed to affect the binary output.
     pub fn stable_hash(self, workspace: &Path) -> PackageIdStableHash<'_> {
         PackageIdStableHash(self, workspace)
     }

tests/testsuite/install.rs

@@ -5,15 +5,15 @@ use std::io::prelude::*;
 
 use cargo_test_support::cross_compile;
 use cargo_test_support::git;
-use cargo_test_support::registry::{registry_path, registry_url, Package};
+use cargo_test_support::registry::{self, registry_path, registry_url, Package};
 use cargo_test_support::{
     basic_manifest, cargo_process, no_such_file_err_msg, project, symlink_supported, t,
 };
 
 use cargo_test_support::install::{
     assert_has_installed_exe, assert_has_not_installed_exe, cargo_home,
 };
-use cargo_test_support::paths;
+use cargo_test_support::paths::{self, CargoPathExt};
 use std::env;
 use std::path::PathBuf;
 
@@ -1739,3 +1739,60 @@ fn locked_install_without_published_lockfile() {
         .with_stderr_contains("[WARNING] no Cargo.lock file published in foo v0.1.0")
         .run();
 }
+
+#[cargo_test]
+fn install_semver_metadata() {
+    // Check trying to install a package that uses semver metadata.
+    // This uses alt registry because the bug this is exercising doesn't
+    // trigger with a replaced source.
+    registry::alt_init();
+    Package::new("foo", "1.0.0+abc")
+        .alternative(true)
+        .file("src/main.rs", "fn main() {}")
+        .publish();
+
+    cargo_process("install foo --registry alternative --version 1.0.0+abc").run();
+    cargo_process("install foo --registry alternative")
+        .with_stderr("\
+[UPDATING] `[ROOT]/alternative-registry` index
+[IGNORED] package `foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)` is already installed, use --force to override
+[WARNING] be sure to add [..]
+")
+        .run();
+    // "Updating" is not displayed here due to the --version fast-path.
+    cargo_process("install foo --registry alternative --version 1.0.0+abc")
+        .with_stderr("\
+[IGNORED] package `foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)` is already installed, use --force to override
+[WARNING] be sure to add [..]
+")
+        .run();
+    cargo_process("install foo --registry alternative --version 1.0.0 --force")
+        .with_stderr(
+            "\
+[UPDATING] `[ROOT]/alternative-registry` index
+[INSTALLING] foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)
+[COMPILING] foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)
+[FINISHED] [..]
+[REPLACING] [ROOT]/home/.cargo/bin/foo[EXE]
+[REPLACED] package [..]
+[WARNING] be sure to add [..]
+",
+        )
+        .run();
+    // Check that from a fresh cache will work without metadata, too.
+    paths::home().join(".cargo/registry").rm_rf();
+    paths::home().join(".cargo/bin").rm_rf();
+    cargo_process("install foo --registry alternative --version 1.0.0")
+        .with_stderr("\
+[UPDATING] `[ROOT]/alternative-registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)
+[INSTALLING] foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)
+[COMPILING] foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)
+[FINISHED] [..]
+[INSTALLING] [ROOT]/home/.cargo/bin/foo[EXE]
+[INSTALLED] package `foo v1.0.0+abc (registry `[ROOT]/alternative-registry`)` (executable `foo[EXE]`)
+[WARNING] be sure to add [..]
+")
+        .run();
+}