From 58da6b05d47da76bd8309745ae1c271b948ae900 Mon Sep 17 00:00:00 2001 From: Tyler Hall Date: Sun, 29 Jan 2017 17:55:38 -0500 Subject: [PATCH 1/3] Adjust custom target test for workspace-relative hashing Switching to workspace-relative metadata hashing means that output can change based on what is considered to be the top-level crate. This test is affected by this change, but in general hashes should be more stable than before. This test verifies that a crate will not rebuild when built as a dependency of two different crates but with the same CARGO_TARGET_DIR. This currently relies on the absolute path to the crate being the same across runs. Set an explicit workspace so the hashes will be the same when using workspace-relative hashing. Setting a workspace would cause the target dir to be the same in both invocations, defeating the intent of this test, so move the target dir between runs to verify that the contents of the target dir are not dependent on its path. --- tests/path.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/path.rs b/tests/path.rs index 16335e2168a..97ccac7b19a 100644 --- a/tests/path.rs +++ b/tests/path.rs @@ -1,4 +1,5 @@ extern crate cargo; +#[macro_use] extern crate cargotest; extern crate hamcrest; @@ -808,6 +809,8 @@ fn custom_target_no_rebuild() { authors = [] [dependencies] a = { path = "a" } + [workspace] + members = ["a", "b"] "#) .file("src/lib.rs", "") .file("a/Cargo.toml", r#" @@ -835,9 +838,10 @@ fn custom_target_no_rebuild() { [FINISHED] dev [unoptimized + debuginfo] target(s) in [..] ")); + t!(fs::rename(p.root().join("target"), p.root().join("target_moved"))); assert_that(p.cargo("build") .arg("--manifest-path=b/Cargo.toml") - .env("CARGO_TARGET_DIR", "target"), + .env("CARGO_TARGET_DIR", "target_moved"), execs().with_status(0) .with_stderr("\ [COMPILING] b v0.5.0 ([..]) From dd02aa34eebe8203e6ebe19b5d9326f3b3ae5e8b Mon Sep 17 00:00:00 2001 From: Tyler Hall Date: Sun, 29 Jan 2017 17:55:44 -0500 Subject: [PATCH 2/3] Stabilize SourceId hashes relative to the workspace Currently a crate from a path source will have its metadata hash incorporate its absolute path on the system where it is built. This always impacts top-level crates, which means that compiling the same source with the same dependencies and compiler version will generate libraries with symbol names that vary depending on where the workspace resides on the machine. For paths inside the Cargo workspace, hash their SourceId relative to the root of the workspace. Paths outside of a workspace are still hashed as absolute. This stability is important for reproducible builds as part of a larger build system that employs caching of artifacts, such as OpenEmbedded. OpenEmbedded tightly controls all inputs to a build and its caching assumes that an equivalent artifact will always result from the same set of inputs. The workspace path is not considered to be an influential input, however. For example, if Cargo is used to compile libstd shared objects which downstream crates link to dynamically, it must be possible to rebuild libstd given the same inputs and produce a library that is at least link-compatible with the original. If the build system happens to cache the downstream crates but needs to rebuild libstd and the user happens to be building in a different workspace path, currently Cargo will generate a library incompatible with the original and the downstream executables will fail at runtime on the target. --- src/cargo/core/package_id.rs | 15 +++++++++++++++ src/cargo/core/source.rs | 15 +++++++++++++-- src/cargo/ops/cargo_rustc/context.rs | 11 +++++++++-- src/cargo/ops/cargo_rustc/mod.rs | 4 ++-- 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index 4058eeaacc4..7035ef5db0f 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -2,6 +2,7 @@ use std::cmp::Ordering; use std::fmt::{self, Formatter}; use std::hash::Hash; use std::hash; +use std::path::Path; use std::sync::Arc; use semver; @@ -131,6 +132,20 @@ impl PackageId { }), } } + + pub fn stable_hash<'a>(&'a self, workspace: &'a Path) -> PackageIdStableHash<'a> { + PackageIdStableHash(&self, workspace) + } +} + +pub struct PackageIdStableHash<'a>(&'a PackageId, &'a Path); + +impl<'a> Hash for PackageIdStableHash<'a> { + fn hash(&self, state: &mut S) { + self.0.inner.name.hash(state); + self.0.inner.version.hash(state); + self.0.inner.source_id.stable_hash(self.1, state); + } } impl fmt::Display for PackageId { diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs index e7a3ce4a2e8..98730e3f081 100644 --- a/src/cargo/core/source.rs +++ b/src/cargo/core/source.rs @@ -1,7 +1,7 @@ use std::cmp::{self, Ordering}; use std::collections::hash_map::{HashMap, Values, IterMut}; use std::fmt::{self, Formatter}; -use std::hash; +use std::hash::{self, Hash}; use std::path::Path; use std::sync::Arc; use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT}; @@ -297,6 +297,17 @@ impl SourceId { } self.inner.url.to_string() == CRATES_IO } + + pub fn stable_hash(&self, workspace: &Path, into: &mut S) { + if self.is_path() { + if let Ok(p) = self.inner.url.to_file_path().unwrap().strip_prefix(workspace) { + self.inner.kind.hash(into); + p.to_str().unwrap().hash(into); + return + } + } + self.hash(into) + } } impl PartialEq for SourceId { @@ -417,7 +428,7 @@ impl Ord for SourceIdInner { // The hash of SourceId is used in the name of some Cargo folders, so shouldn't // vary. `as_str` gives the serialisation of a url (which has a spec) and so // insulates against possible changes in how the url crate does hashing. -impl hash::Hash for SourceId { +impl Hash for SourceId { fn hash(&self, into: &mut S) { self.inner.kind.hash(into); match *self.inner { diff --git a/src/cargo/ops/cargo_rustc/context.rs b/src/cargo/ops/cargo_rustc/context.rs index 64e2e5b9f25..b42247293a4 100644 --- a/src/cargo/ops/cargo_rustc/context.rs +++ b/src/cargo/ops/cargo_rustc/context.rs @@ -407,7 +407,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { let name = unit.pkg.package_id().name(); match self.target_metadata(unit) { Some(meta) => format!("{}-{}", name, meta), - None => format!("{}-{}", name, util::short_hash(unit.pkg)), + None => format!("{}-{}", name, self.target_short_hash(unit)), } } @@ -426,6 +426,13 @@ impl<'a, 'cfg> Context<'a, 'cfg> { self.build_config.requested_target.as_ref().map(|s| &s[..]) } + /// Get the short hash based only on the PackageId + /// Used for the metadata when target_metadata returns None + pub fn target_short_hash(&self, unit: &Unit) -> String { + let hashable = unit.pkg.package_id().stable_hash(self.ws.root()); + util::short_hash(&hashable) + } + /// Get the metadata for a target in a specific profile /// We build to the path: "{filename}-{target_metadata}" /// We use a linking step to link/copy to a predictable filename @@ -459,7 +466,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> { // Unique metadata per (name, source, version) triple. This'll allow us // to pull crates from anywhere w/o worrying about conflicts - unit.pkg.package_id().hash(&mut hasher); + unit.pkg.package_id().stable_hash(self.ws.root()).hash(&mut hasher); // Add package properties which map to environment variables // exposed by Cargo diff --git a/src/cargo/ops/cargo_rustc/mod.rs b/src/cargo/ops/cargo_rustc/mod.rs index 67bb834a271..6f2925740d9 100644 --- a/src/cargo/ops/cargo_rustc/mod.rs +++ b/src/cargo/ops/cargo_rustc/mod.rs @@ -12,7 +12,7 @@ use core::{Package, PackageId, PackageSet, Target, Resolve}; use core::{Profile, Profiles, Workspace}; use core::shell::ColorChoice; use util::{self, ProcessBuilder, machine_message}; -use util::{Config, internal, profile, join_paths, short_hash}; +use util::{Config, internal, profile, join_paths}; use util::errors::{CargoResult, CargoResultExt}; use util::Freshness; @@ -791,7 +791,7 @@ fn build_base_args(cx: &mut Context, cmd.arg("-C").arg(&format!("extra-filename=-{}", m)); } None => { - cmd.arg("-C").arg(&format!("metadata={}", short_hash(unit.pkg))); + cmd.arg("-C").arg(&format!("metadata={}", cx.target_short_hash(unit))); } } From eb0e4c0b1180b63be74fda649d5f4994b0955199 Mon Sep 17 00:00:00 2001 From: Tyler Hall Date: Sun, 29 Jan 2017 17:55:47 -0500 Subject: [PATCH 3/3] Add a test for stable metadata across workspaces --- tests/build.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/build.rs b/tests/build.rs index f2081c61fa3..f096b8fbd1e 100644 --- a/tests/build.rs +++ b/tests/build.rs @@ -1,4 +1,5 @@ extern crate cargo; +#[macro_use] extern crate cargotest; extern crate hamcrest; extern crate tempdir; @@ -3271,3 +3272,32 @@ fn no_bin_in_src_with_lib() { execs().with_status(101) .with_stderr_contains(r#"[ERROR] couldn't read "[..]main.rs"[..]"#)); } + +#[test] +fn same_metadata_different_directory() { + // A top-level crate built in two different workspaces should have the + // same metadata hash. + let p = project("foo1") + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file("src/foo.rs", &main_file(r#""i am foo""#, &[])); + let output = t!(String::from_utf8( + t!(p.cargo_process("build").arg("-v").exec_with_output()) + .stderr, + )); + let metadata = output + .split_whitespace() + .filter(|arg| arg.starts_with("metadata=")) + .next() + .unwrap(); + + let p = project("foo2") + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file("src/foo.rs", &main_file(r#""i am foo""#, &[])); + + assert_that( + p.cargo_process("build").arg("-v"), + execs().with_status(0).with_stderr_contains( + format!("[..]{}[..]", metadata), + ), + ); +}