Auto merge of #2423 - alexcrichton:fix-pkgid-hash, r=brson

All crates being compiled by Cargo are identified by a unique `PackageId` instance. This ID incorporates information such as the name, version, and source from where the crate came from. Package ids are allowed to have path sources to depend on local crates on the filesystem. The package id itself encodes the path of where the crate came from.

Historically, however, the "path source" from where these packages are learned had some interesting logic. Specifically one specific source would be able to return many packages within. In other words, a path source would recursively walk crate definitions and the filesystem attempting to find crates. Each crate returned from a source has the same source id, so consequently all packages from one source path would have the same source path id.

This in turn leads to confusing an surprising behavior, for example:

* When crates are compiled the status message indicates the path of the crate root, not the crate being compiled
* When viewed from two different locations (e.g. two different crate roots) the same package would have two different source ids because the id is based on the root location.

This hash mismatch has been [papered over](#1697) in the past to try to fix some spurious recompiles, but it unfortunately [leaked back in](#2279). This is clearly indicative of the "hack" being inappropriate so instead these commits fix the root of the problem.

---

In short, these commits ensure that the package id for a package defined locally has a path that points precisely at that package. This was a relatively invasive change and had ramifications on a few specific portions which now require a bit of extra code to support.

The fundamental change here was to change `PathSource` to be non-recursive by default in terms of what packages it thinks it contains. There are still two recursive use cases, git repositories and path overrides, which are used for backwards compatibility. This meant, however, that the packaging step for crate no longer has knowledge of other crates in a repository to filter out files from. Some specific logic was added to assist in discovering a git repository as well as filtering out sibling packages.

Another ramification of this patch, however, is that special care needs to be taken when decoding a lockfile. We now need all path dependencies in the lockfile to point precisely at where the path dependency came from, and this information is not encoded in the lock file. The decoding support was altered to do a simple probe of the filesystem to recursively walk path dependencies to ensure that we can match up packages in a lock file to where they're found on the filesystem.

Overall, however, this commit closes #1697 and also addresses servo/servo#9794 where this issue was originally reported.

Author: bors

src/bin/read_manifest.rs

@@ -1,9 +1,8 @@
 use std::env;
 
-use cargo::core::{Package, Source};
+use cargo::core::Package;
 use cargo::util::{CliResult, Config};
 use cargo::util::important_paths::{find_root_manifest_for_wd};
-use cargo::sources::{PathSource};
 
 #[derive(RustcDecodable)]
 pub struct Options {
@@ -32,9 +31,6 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<Package>>
 
     let root = try!(find_root_manifest_for_wd(options.flag_manifest_path, config.cwd()));
 
-    let mut source = try!(PathSource::for_path(root.parent().unwrap(), config));
-    try!(source.update());
-
-    let pkg = try!(source.root_package());
+    let pkg = try!(Package::for_path(&root, config));
     Ok(Some(pkg))
 }

src/cargo/core/package.rs

@@ -87,7 +87,7 @@ impl Package {
     }
 
     pub fn generate_metadata(&self) -> Metadata {
-        self.package_id().generate_metadata(self.root())
+        self.package_id().generate_metadata()
     }
 }
 
@@ -107,15 +107,7 @@ impl Eq for Package {}
 
 impl hash::Hash for Package {
     fn hash<H: hash::Hasher>(&self, into: &mut H) {
-        // 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)
-        }
+        self.package_id().hash(into)
     }
 }
 

src/cargo/core/package_id.rs

@@ -3,7 +3,6 @@ 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;
@@ -136,13 +135,8 @@ 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, 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))
-        };
+    pub fn generate_metadata(&self) -> Metadata {
+        let metadata = short_hash(self);
         let extra_filename = format!("-{}", metadata);
 
         Metadata { metadata: metadata, extra_filename: extra_filename }

src/cargo/core/registry.rs

@@ -143,11 +143,9 @@ impl<'cfg> PackageRegistry<'cfg> {
         self.source_ids.insert(id.clone(), (id.clone(), kind));
     }
 
-    pub fn add_overrides(&mut self, ids: Vec<SourceId>) -> CargoResult<()> {
-        for id in ids.iter() {
-            try!(self.load(id, Kind::Override));
-        }
-        Ok(())
+    pub fn add_override(&mut self, id: &SourceId, source: Box<Source + 'cfg>) {
+        self.add_source(id, source, Kind::Override);
+        self.overrides.push(id.clone());
     }
 
     pub fn register_lock(&mut self, id: PackageId, deps: Vec<PackageId>) {

src/cargo/core/resolver/encode.rs

@@ -3,8 +3,8 @@ use std::collections::{HashMap, BTreeMap};
 use regex::Regex;
 use rustc_serialize::{Encodable, Encoder, Decodable, Decoder};
 
-use core::{PackageId, SourceId};
-use util::{CargoResult, Graph};
+use core::{Package, PackageId, SourceId};
+use util::{CargoResult, Graph, Config};
 
 use super::Resolve;
 
@@ -18,66 +18,113 @@ pub struct EncodableResolve {
 pub type Metadata = BTreeMap<String, String>;
 
 impl EncodableResolve {
-    pub fn to_resolve(&self, default: &SourceId) -> CargoResult<Resolve> {
+    pub fn to_resolve(&self, root: &Package, config: &Config)
+                      -> CargoResult<Resolve> {
+        let mut path_deps = HashMap::new();
+        try!(build_path_deps(root, &mut path_deps, config));
+        let default = root.package_id().source_id();
+
         let mut g = Graph::new();
         let mut tmp = HashMap::new();
 
         let packages = Vec::new();
         let packages = self.package.as_ref().unwrap_or(&packages);
 
+        let root = try!(to_package_id(&self.root.name,
+                                      &self.root.version,
+                                      self.root.source.as_ref(),
+                                      default, &path_deps));
+        let ids = try!(packages.iter().map(|p| {
+            to_package_id(&p.name, &p.version, p.source.as_ref(),
+                          default, &path_deps)
+        }).collect::<CargoResult<Vec<_>>>());
+
         {
-            let mut register_pkg = |pkg: &EncodableDependency|
-                                    -> CargoResult<()> {
-                let pkgid = try!(pkg.to_package_id(default));
+            let mut register_pkg = |pkgid: &PackageId| {
                 let precise = pkgid.source_id().precise()
                                    .map(|s| s.to_string());
                 assert!(tmp.insert(pkgid.clone(), precise).is_none(),
                         "a package was referenced twice in the lockfile");
-                g.add(try!(pkg.to_package_id(default)), &[]);
-                Ok(())
+                g.add(pkgid.clone(), &[]);
             };
 
-            try!(register_pkg(&self.root));
-            for pkg in packages.iter() {
-                try!(register_pkg(pkg));
+            register_pkg(&root);
+            for id in ids.iter() {
+                register_pkg(id);
             }
         }
 
         {
-            let mut add_dependencies = |pkg: &EncodableDependency|
+            let mut add_dependencies = |id: &PackageId, pkg: &EncodableDependency|
                                         -> CargoResult<()> {
-                let package_id = try!(pkg.to_package_id(default));
-
                 let deps = match pkg.dependencies {
                     Some(ref deps) => deps,
                     None => return Ok(()),
                 };
                 for edge in deps.iter() {
-                    let to_depend_on = try!(edge.to_package_id(default));
+                    let to_depend_on = try!(to_package_id(&edge.name,
+                                                          &edge.version,
+                                                          edge.source.as_ref(),
+                                                          default,
+                                                          &path_deps));
                     let precise_pkgid =
                         tmp.get(&to_depend_on)
                            .map(|p| to_depend_on.with_precise(p.clone()))
                            .unwrap_or(to_depend_on.clone());
-                    g.link(package_id.clone(), precise_pkgid);
+                    g.link(id.clone(), precise_pkgid);
                 }
                 Ok(())
             };
 
-            try!(add_dependencies(&self.root));
-            for pkg in packages.iter() {
-                try!(add_dependencies(pkg));
+            try!(add_dependencies(&root, &self.root));
+            for (id, pkg) in ids.iter().zip(packages) {
+                try!(add_dependencies(id, pkg));
             }
         }
 
         Ok(Resolve {
             graph: g,
-            root: try!(self.root.to_package_id(default)),
+            root: root,
             features: HashMap::new(),
             metadata: self.metadata.clone(),
         })
     }
 }
 
+fn build_path_deps(root: &Package,
+                   map: &mut HashMap<String, SourceId>,
+                   config: &Config)
+                   -> CargoResult<()> {
+    assert!(root.package_id().source_id().is_path());
+
+    let deps = root.dependencies()
+                   .iter()
+                   .map(|d| d.source_id())
+                   .filter(|id| id.is_path())
+                   .filter_map(|id| id.url().to_file_path().ok())
+                   .map(|path| path.join("Cargo.toml"))
+                   .filter_map(|path| Package::for_path(&path, config).ok());
+    for pkg in deps {
+        let source_id = pkg.package_id().source_id();
+        if map.insert(pkg.name().to_string(), source_id.clone()).is_none() {
+            try!(build_path_deps(&pkg, map, config));
+        }
+    }
+
+    Ok(())
+}
+
+fn to_package_id(name: &str,
+                 version: &str,
+                 source: Option<&SourceId>,
+                 default_source: &SourceId,
+                 path_sources: &HashMap<String, SourceId>)
+                 -> CargoResult<PackageId> {
+    let source = source.or(path_sources.get(name)).unwrap_or(default_source);
+    PackageId::new(name, version, source)
+}
+
+
 #[derive(RustcEncodable, RustcDecodable, Debug, PartialOrd, Ord, PartialEq, Eq)]
 pub struct EncodableDependency {
     name: String,
@@ -86,15 +133,6 @@ pub struct EncodableDependency {
     dependencies: Option<Vec<EncodablePackageId>>
 }
 
-impl EncodableDependency {
-    fn to_package_id(&self, default_source: &SourceId) -> CargoResult<PackageId> {
-        PackageId::new(
-            &self.name,
-            &self.version,
-            self.source.as_ref().unwrap_or(default_source))
-    }
-}
-
 #[derive(Debug, PartialOrd, Ord, PartialEq, Eq)]
 pub struct EncodablePackageId {
     name: String,
@@ -134,15 +172,6 @@ impl Decodable for EncodablePackageId {
     }
 }
 
-impl EncodablePackageId {
-    fn to_package_id(&self, default_source: &SourceId) -> CargoResult<PackageId> {
-        PackageId::new(
-            &self.name,
-            &self.version,
-            self.source.as_ref().unwrap_or(default_source))
-    }
-}
-
 impl Encodable for Resolve {
     fn encode<S: Encoder>(&self, s: &mut S) -> Result<(), S::Error> {
         let mut ids: Vec<&PackageId> = self.graph.iter().collect();
@@ -151,28 +180,26 @@ impl Encodable for Resolve {
         let encodable = ids.iter().filter_map(|&id| {
             if self.root == *id { return None; }
 
-            Some(encodable_resolve_node(id, &self.root, &self.graph))
+            Some(encodable_resolve_node(id, &self.graph))
         }).collect::<Vec<EncodableDependency>>();
 
         EncodableResolve {
             package: Some(encodable),
-            root: encodable_resolve_node(&self.root, &self.root, &self.graph),
+            root: encodable_resolve_node(&self.root, &self.graph),
             metadata: self.metadata.clone(),
         }.encode(s)
     }
 }
 
-fn encodable_resolve_node(id: &PackageId, root: &PackageId,
-                          graph: &Graph<PackageId>) -> EncodableDependency {
+fn encodable_resolve_node(id: &PackageId, graph: &Graph<PackageId>)
+                          -> EncodableDependency {
     let deps = graph.edges(id).map(|edge| {
-        let mut deps = edge.map(|e| {
-            encodable_package_id(e, root)
-        }).collect::<Vec<EncodablePackageId>>();
+        let mut deps = edge.map(encodable_package_id).collect::<Vec<_>>();
         deps.sort();
         deps
     });
 
-    let source = if id.source_id() == root.source_id() {
+    let source = if id.source_id().is_path() {
         None
     } else {
         Some(id.source_id().clone())
@@ -186,8 +213,8 @@ fn encodable_resolve_node(id: &PackageId, root: &PackageId,
     }
 }
 
-fn encodable_package_id(id: &PackageId, root: &PackageId) -> EncodablePackageId {
-    let source = if id.source_id() == root.source_id() {
+fn encodable_package_id(id: &PackageId) -> EncodablePackageId {
+    let source = if id.source_id().is_path() {
         None
     } else {
         Some(id.source_id().with_precise(None))

src/cargo/ops/cargo_compile.rs

@@ -32,8 +32,9 @@ use core::{Source, SourceId, PackageSet, Package, Target};
 use core::{Profile, TargetKind, Profiles};
 use core::resolver::{Method, Resolve};
 use ops::{self, BuildOutput, ExecEngine};
+use sources::PathSource;
 use util::config::Config;
-use util::{CargoResult, internal, ChainError, profile};
+use util::{CargoResult, profile};
 
 /// Contains information about how a package should be compiled.
 pub struct CompileOptions<'a> {
@@ -104,8 +105,6 @@ pub fn resolve_dependencies<'a>(root_package: &Package,
                                 no_default_features: bool)
                                 -> CargoResult<(PackageSet<'a>, Resolve)> {
 
-    let override_ids = try!(source_ids_from_config(config, root_package.root()));
-
     let mut registry = PackageRegistry::new(config);
 
     if let Some(source) = source {
@@ -114,14 +113,14 @@ pub fn resolve_dependencies<'a>(root_package: &Package,
 
     // First, resolve the root_package's *listed* dependencies, as well as
     // downloading and updating all remotes and such.
-    let resolve = try!(ops::resolve_pkg(&mut registry, root_package));
+    let resolve = try!(ops::resolve_pkg(&mut registry, root_package, config));
 
     // Second, resolve with precisely what we're doing. Filter out
     // transitive dependencies if necessary, specify features, handle
     // overrides, etc.
     let _p = profile::start("resolving w/ overrides...");
 
-    try!(registry.add_overrides(override_ids));
+    try!(add_overrides(&mut registry, root_package.root(), config));
 
     let method = Method::Required{
         dev_deps: true, // TODO: remove this option?
@@ -383,20 +382,14 @@ fn generate_targets<'a>(pkg: &'a Package,
 
 /// Read the `paths` configuration variable to discover all path overrides that
 /// have been configured.
-fn source_ids_from_config(config: &Config, cur_path: &Path)
-                          -> CargoResult<Vec<SourceId>> {
-
-    let configs = try!(config.values());
-    debug!("loaded config; configs={:?}", configs);
-    let config_paths = match configs.get("paths") {
-        Some(cfg) => cfg,
-        None => return Ok(Vec::new())
+fn add_overrides<'a>(registry: &mut PackageRegistry<'a>,
+                     cur_path: &Path,
+                     config: &'a Config) -> CargoResult<()> {
+    let paths = match try!(config.get_list("paths")) {
+        Some(list) => list,
+        None => return Ok(())
     };
-    let paths = try!(config_paths.list().chain_error(|| {
-        internal("invalid configuration for the key `paths`")
-    }));
-
-    paths.iter().map(|&(ref s, ref p)| {
+    let paths = paths.val.iter().map(|&(ref s, ref p)| {
         // The path listed next to the string is the config file in which the
         // key was located, so we want to pop off the `.cargo/config` component
         // to get the directory containing the `.cargo` folder.
@@ -405,7 +398,15 @@ fn source_ids_from_config(config: &Config, cur_path: &Path)
         // Make sure we don't override the local package, even if it's in the
         // list of override paths.
         cur_path != &**p
-    }).map(|p| SourceId::for_path(&p)).collect()
+    });
+
+    for path in paths {
+        let id = try!(SourceId::for_path(&path));
+        let mut source = PathSource::new_recursive(&path, &id, config);
+        try!(source.update());
+        registry.add_override(&id, Box::new(source));
+    }
+    Ok(())
 }
 
 /// Parse all config files to learn about build configuration. Currently

src/cargo/ops/cargo_fetch.rs

@@ -11,7 +11,7 @@ pub fn fetch<'a>(manifest_path: &Path,
                  -> CargoResult<(Resolve, PackageSet<'a>)> {
     let package = try!(Package::for_path(manifest_path, config));
     let mut registry = PackageRegistry::new(config);
-    let resolve = try!(ops::resolve_pkg(&mut registry, &package));
+    let resolve = try!(ops::resolve_pkg(&mut registry, &package, config));
     let packages = get_resolved_packages(&resolve, registry);
     for id in resolve.iter() {
         try!(packages.get(id));