From 9387a308f4251e2e30690c31c169ece8a6a1ba6c Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 8 May 2021 16:31:09 -0700 Subject: [PATCH] Fix `cargo install` with a semver metadata version. --- src/cargo/core/package_id.rs | 32 +++++++++++++++++-- tests/testsuite/install.rs | 61 ++++++++++++++++++++++++++++++++++-- 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index 24f2ce6c641..534b3017166 100644 --- a/src/cargo/core/package_id.rs +++ b/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(&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(&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) } diff --git a/tests/testsuite/install.rs b/tests/testsuite/install.rs index 6ea137c4ae8..e8891e21b2a 100644 --- a/tests/testsuite/install.rs +++ b/tests/testsuite/install.rs @@ -5,7 +5,7 @@ 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, }; @@ -13,7 +13,7 @@ use cargo_test_support::{ 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(); +}