Skip to content

Commit

Permalink
Fix spurious rebuilds when switching source paths
Browse files Browse the repository at this point in the history
The method of creating package ids in Cargo means that all sub-crates of a main
repo have the same package id, which encodes the path it came from. This means
that if the "root crate" switches, the package id for all dependencies will
change, causing an alteration in package id hashes, causing recompiles.

This commit alters a few points of hashing to ensure that whenever a package is
being hashed for freshness the *source root* of the crate is used instead of the
root of the main crate. This cause the hashes to be consistent across compiles,
regardless of the root package.

Closes #1694
  • Loading branch information
alexcrichton committed Jun 8, 2015
1 parent 348c738 commit ddf3c79
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 18 deletions.
23 changes: 14 additions & 9 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,7 @@ use std::slice;
use std::path::{Path, PathBuf};
use semver::Version;

use core::{
Dependency,
Manifest,
PackageId,
Registry,
Target,
Summary,
};
use core::{Dependency, Manifest, PackageId, Registry, Target, Summary, Metadata};
use core::dependency::SerializedDependency;
use util::{CargoResult, graph};
use rustc_serialize::{Encoder,Encodable};
Expand Down Expand Up @@ -78,6 +71,10 @@ impl Package {
pub fn has_custom_build(&self) -> bool {
self.targets().iter().any(|t| t.is_custom_build())
}

pub fn generate_metadata(&self) -> Metadata {
self.package_id().generate_metadata(self.root())
}
}

impl fmt::Display for Package {
Expand All @@ -96,7 +93,15 @@ impl Eq for Package {}

impl hash::Hash for Package {
fn hash<H: hash::Hasher>(&self, into: &mut H) {
self.package_id().hash(into)
// We want to be sure that a path-based package showing up at the same
// location always has the same hash. To that effect we don't hash the
// vanilla package ID if we're a path, but instead feed in our own root
// path.
if self.package_id().source_id().is_path() {
(0, self.root(), self.name(), self.package_id().version()).hash(into)
} else {
(1, self.package_id()).hash(into)
}
}
}

Expand Down
12 changes: 8 additions & 4 deletions src/cargo/core/package_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::error::Error;
use std::fmt::{self, Formatter};
use std::hash::Hash;
use std::hash;
use std::path::Path;
use std::sync::Arc;

use regex::Regex;
Expand Down Expand Up @@ -135,10 +136,13 @@ impl PackageId {
pub fn version(&self) -> &semver::Version { &self.inner.version }
pub fn source_id(&self) -> &SourceId { &self.inner.source_id }

pub fn generate_metadata(&self) -> Metadata {
let metadata = short_hash(
&(&self.inner.name, self.inner.version.to_string(),
&self.inner.source_id));
pub fn generate_metadata(&self, source_root: &Path) -> Metadata {
// See comments in Package::hash for why we have this test
let metadata = if self.inner.source_id.is_path() {
short_hash(&(0, &self.inner.name, &self.inner.version, source_root))
} else {
short_hash(&(1, self))
};
let extra_filename = format!("-{}", metadata);

Metadata { metadata: metadata, extra_filename: extra_filename }
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_rustc/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
// Make sure that the name of this test executable doesn't
// conflict with a library that has the same name and is
// being tested
let mut metadata = pkg.package_id().generate_metadata();
let mut metadata = pkg.generate_metadata();
metadata.mix(&format!("bin-{}", target.name()));
Some(metadata)
} else if pkg.package_id() == self.resolve.root() && !profile.test {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_rustc/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ impl Layout {
}

fn pkg_dir(&self, pkg: &Package) -> String {
format!("{}-{}", pkg.name(), short_hash(pkg.package_id()))
format!("{}-{}", pkg.name(), short_hash(pkg))
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/cargo/util/toml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ impl TomlManifest {
}

let pkgid = try!(project.to_package_id(source_id));
let metadata = pkgid.generate_metadata();
let metadata = pkgid.generate_metadata(&layout.root);

// If we have no lib at all, use the inferred lib if available
// If we have a lib with a path, we're done
Expand Down Expand Up @@ -427,8 +427,8 @@ impl TomlManifest {

for bin in bins.iter() {
if blacklist.iter().find(|&x| *x == bin.name) != None {
return Err(human(&format!("the binary target name `{}` is forbidden",
bin.name)));
return Err(human(&format!("the binary target name `{}` is \
forbidden", bin.name)));
}
}

Expand Down
44 changes: 44 additions & 0 deletions tests/test_cargo_compile_path_deps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -749,3 +749,47 @@ test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured
", COMPILING, p.url(), COMPILING, p.url())));
});

test!(custom_target_no_rebuild {
let p = project("foo")
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.5.0"
authors = []
[dependencies]
a = { path = "a" }
"#)
.file("src/lib.rs", "")
.file("a/Cargo.toml", r#"
[project]
name = "a"
version = "0.5.0"
authors = []
"#)
.file("a/src/lib.rs", "")
.file("b/Cargo.toml", r#"
[project]
name = "b"
version = "0.5.0"
authors = []
[dependencies]
a = { path = "../a" }
"#)
.file("b/src/lib.rs", "");
p.build();
assert_that(p.cargo("build"),
execs().with_status(0)
.with_stdout(&format!("\
{compiling} a v0.5.0 ([..])
{compiling} foo v0.5.0 ([..])
", compiling = COMPILING)));

assert_that(p.cargo("build")
.arg("--manifest-path=b/Cargo.toml")
.env("CARGO_TARGET_DIR", "target"),
execs().with_status(0)
.with_stdout(&format!("\
{compiling} b v0.5.0 ([..])
", compiling = COMPILING)));
});

0 comments on commit ddf3c79

Please sign in to comment.