Auto merge of #1697 - alexcrichton:fix-path-pkgid, r=brson

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

src/cargo/core/package.rs

@@ -4,18 +4,11 @@ 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};
-use core::source::{SourceId, Source};
+use core::source::Source;
 
 /// Informations about a package that is available somewhere in the file system.
 ///
@@ -27,8 +20,6 @@ pub struct Package {
     manifest: Manifest,
     // The root of the package
     manifest_path: PathBuf,
-    // Where this package came from
-    source_id: SourceId,
 }
 
 #[derive(RustcEncodable)]
@@ -60,12 +51,10 @@ impl Encodable for Package {
 
 impl Package {
     pub fn new(manifest: Manifest,
-               manifest_path: &Path,
-               source_id: &SourceId) -> Package {
+               manifest_path: &Path) -> Package {
         Package {
             manifest: manifest,
             manifest_path: manifest_path.to_path_buf(),
-            source_id: source_id.clone(),
         }
     }
 
@@ -82,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 {
@@ -100,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)
+        }
     }
 }
 

src/cargo/core/package_id.rs

@@ -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;
@@ -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 }

src/cargo/core/registry.rs

@@ -19,6 +19,13 @@ impl Registry for Vec<Summary> {
     }
 }
 
+impl Registry for Vec<Package> {
+    fn query(&mut self, dep: &Dependency) -> CargoResult<Vec<Summary>> {
+        Ok(self.iter().filter(|pkg| dep.matches(pkg.summary()))
+               .map(|pkg| pkg.summary().clone()).collect())
+    }
+}
+
 /// This structure represents a registry of known packages. It internally
 /// contains a number of `Box<Source>` instances which are used to load a
 /// `Package` from.

src/cargo/core/source.rs

@@ -12,7 +12,7 @@ use url::Url;
 use core::{Summary, Package, PackageId, Registry, Dependency};
 use sources::{PathSource, GitSource, RegistrySource};
 use sources::git;
-use util::{human, Config, CargoResult, CargoError, ToUrl};
+use util::{human, Config, CargoResult, ToUrl};
 
 /// A Source finds and downloads remote packages based on names and
 /// versions.
@@ -61,8 +61,6 @@ pub enum GitReference {
     Rev(String),
 }
 
-type Error = Box<CargoError + Send>;
-
 /// Unique identifier for a source of packages.
 #[derive(Clone, Eq, Debug)]
 pub struct SourceId {

src/cargo/ops/cargo_package.rs

@@ -175,7 +175,7 @@ fn run_verify(config: &Config, pkg: &Package, tar: &Path)
     });
     let mut new_manifest = pkg.manifest().clone();
     new_manifest.set_summary(new_summary.override_id(new_pkgid));
-    let new_pkg = Package::new(new_manifest, &manifest_path, &new_src);
+    let new_pkg = Package::new(new_manifest, &manifest_path);
 
     // Now that we've rewritten all our path dependencies, compile it!
     try!(ops::compile_pkg(&new_pkg, None, &ops::CompileOptions {

src/cargo/ops/cargo_read_manifest.rs

@@ -30,7 +30,7 @@ pub fn read_package(path: &Path, source_id: &SourceId, config: &Config)
     let (manifest, nested) =
         try!(read_manifest(&data, layout, source_id, config));
 
-    Ok((Package::new(manifest, path, source_id), nested))
+    Ok((Package::new(manifest, path), nested))
 }
 
 pub fn read_packages(path: &Path, source_id: &SourceId, config: &Config)

src/cargo/ops/cargo_rustc/context.rs

@@ -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 {

src/cargo/ops/cargo_rustc/layout.rs

@@ -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))
     }
 }
 

src/cargo/sources/path.rs

@@ -59,7 +59,7 @@ impl<'cfg> PathSource<'cfg> {
         }
     }
 
-    pub fn read_packages(&self) -> CargoResult<Vec<Package>> {
+    fn read_packages(&self) -> CargoResult<Vec<Package>> {
         if self.updated {
             Ok(self.packages.clone())
         } else if self.id.is_path() && self.id.precise().is_some() {
@@ -272,10 +272,7 @@ impl<'cfg> Debug for PathSource<'cfg> {
 
 impl<'cfg> Registry for PathSource<'cfg> {
     fn query(&mut self, dep: &Dependency) -> CargoResult<Vec<Summary>> {
-        let mut summaries: Vec<Summary> = self.packages.iter()
-                                              .map(|p| p.summary().clone())
-                                              .collect();
-        summaries.query(dep)
+        self.packages.query(dep)
     }
 }
 

src/cargo/util/toml.rs

@@ -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
@@ -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)));
             }
         }
 

tests/test_cargo_compile_path_deps.rs

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