Auto merge of #9467 - ehuss:install-metadata, r=alexcrichton

Fix `cargo install` with a semver metadata version.

This fixes an issue where `cargo install cargo-c --version 0.8.0+cargo-0.51` fails (returns a 404 error when downloading) when the index has not yet been populated through other means. The crux of the issue is that the `PackageId` interner was treating `0.8.0+cargo-0.51` and `0.8.0` the same. Due to a chain of events, the interner was getting populated with `0.8.0` first, and then from there on always returned `0.8.0`. The full version information is needed to construct the download URL, so it was failing.

The reason the interner was getting populated with a version without the metadata is the following sequence of events:

1. There is [this "fast path" code path](https://github.com/rust-lang/cargo/blob/d1baf0d81d0e4f91881de8e544c71fb49f623047/src/cargo/ops/cargo_install.rs#L570) which checks if a version of a package is already installed *before updating the index*.
2. Since the index doesn't exist yet, the resolver query returns zero entries (because the Registry Source is empty) [here](https://github.com/rust-lang/cargo/blob/d1baf0d81d0e4f91881de8e544c71fb49f623047/src/cargo/ops/common_for_install_and_uninstall.rs#L546-L550).
3. That code checks if the package has been yanked (because it can't tell the difference between "yanked" and "index not downloaded, yet").
4. It constructs a `PackageId` using a `VersionReq` where the build metadata has been removed (because version reqs don't have build metadata).
5. When the real install continues (the error here is ignored for the purpose of this fast-path check if it is already installed), it downloads the index. However, the `PackageId` values created when parsing the index JSON files are now missing the build metadata because the interner is returning the wrong entries.
6. When the download starts, the URL is built from the `PackageId` missing the build metadata.

I only changed `PackageIdInner` to pay attention to the build metadata. This seems a bit fragile, as perhaps `PackageId` should also pay attention to it. However, I don't really want to do an audit of every use of `PackageId`, and offhand I can't think of other situations where it would matter.

Closes #9410

Author: bors

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();
+}