Auto merge of #4562 - SimonSapin:btree-manifest, r=matklad

Make manifest serialization deterministic

Fixes #4326

`cargo package` (and so `cargo publish`) parses a crate’s `Cargo.toml`, makes some modifications, and re-serializes it. Because the `TomlManifest` struct uses `HashMap` with its default `RandomState` hasher, the maps’ iteration order changed on every run.

As a result, when using `cargo vendor`, updating a dependency would generate a diff larger than necessary, with non-significant order-changes obscuring significant changes.

This replaces some uses of `HashMap` with `BTreeMap`, whose iteration order is deterministic (based on `Ord`).

src/cargo/core/manifest.rs

@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::collections::{HashMap, BTreeMap};
 use std::fmt;
 use std::path::{PathBuf, Path};
 use std::rc::Rc;
@@ -75,7 +75,7 @@ pub struct ManifestMetadata {
     pub homepage: Option<String>,       // url
     pub repository: Option<String>,     // url
     pub documentation: Option<String>,  // url
-    pub badges: HashMap<String, HashMap<String, String>>,
+    pub badges: BTreeMap<String, BTreeMap<String, String>>,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]

src/cargo/core/package.rs

@@ -1,5 +1,5 @@
 use std::cell::{Ref, RefCell};
-use std::collections::HashMap;
+use std::collections::{HashMap, BTreeMap};
 use std::fmt;
 use std::hash;
 use std::path::{Path, PathBuf};
@@ -37,7 +37,7 @@ struct SerializedPackage<'a> {
     source: &'a SourceId,
     dependencies: &'a [Dependency],
     targets: &'a [Target],
-    features: &'a HashMap<String, Vec<String>>,
+    features: &'a BTreeMap<String, Vec<String>>,
     manifest_path: &'a str,
 }
 

src/cargo/core/summary.rs

@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::mem;
 use std::rc::Rc;
 
@@ -20,14 +20,14 @@ pub struct Summary {
 struct Inner {
     package_id: PackageId,
     dependencies: Vec<Dependency>,
-    features: HashMap<String, Vec<String>>,
+    features: BTreeMap<String, Vec<String>>,
     checksum: Option<String>,
 }
 
 impl Summary {
     pub fn new(pkg_id: PackageId,
                dependencies: Vec<Dependency>,
-               features: HashMap<String, Vec<String>>) -> CargoResult<Summary> {
+               features: BTreeMap<String, Vec<String>>) -> CargoResult<Summary> {
         for dep in dependencies.iter() {
             if features.get(dep.name()).is_some() {
                 bail!("Features and dependencies cannot have the \
@@ -78,7 +78,7 @@ impl Summary {
     pub fn version(&self) -> &Version { self.package_id().version() }
     pub fn source_id(&self) -> &SourceId { self.package_id().source_id() }
     pub fn dependencies(&self) -> &[Dependency] { &self.inner.dependencies }
-    pub fn features(&self) -> &HashMap<String, Vec<String>> { &self.inner.features }
+    pub fn features(&self) -> &BTreeMap<String, Vec<String>> { &self.inner.features }
     pub fn checksum(&self) -> Option<&str> {
         self.inner.checksum.as_ref().map(|s| &s[..])
     }

src/cargo/sources/registry/mod.rs

@@ -159,7 +159,7 @@
 //! ```
 
 use std::borrow::Cow;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::fmt;
 use std::fs::File;
 use std::path::{PathBuf, Path};
@@ -206,7 +206,7 @@ struct RegistryPackage<'a> {
     name: Cow<'a, str>,
     vers: Version,
     deps: DependencyList,
-    features: HashMap<String, Vec<String>>,
+    features: BTreeMap<String, Vec<String>>,
     cksum: String,
     yanked: Option<bool>,
 }

src/cargo/util/toml/mod.rs

@@ -1,4 +1,4 @@
-use std::collections::{HashMap, HashSet, BTreeSet};
+use std::collections::{HashMap, BTreeMap, HashSet, BTreeSet};
 use std::fmt;
 use std::fs;
 use std::path::{Path, PathBuf};
@@ -207,21 +207,21 @@ pub struct TomlManifest {
     example: Option<Vec<TomlExampleTarget>>,
     test: Option<Vec<TomlTestTarget>>,
     bench: Option<Vec<TomlTestTarget>>,
-    dependencies: Option<HashMap<String, TomlDependency>>,
+    dependencies: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "dev-dependencies")]
-    dev_dependencies: Option<HashMap<String, TomlDependency>>,
+    dev_dependencies: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "dev_dependencies")]
-    dev_dependencies2: Option<HashMap<String, TomlDependency>>,
+    dev_dependencies2: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "build-dependencies")]
-    build_dependencies: Option<HashMap<String, TomlDependency>>,
+    build_dependencies: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "build_dependencies")]
-    build_dependencies2: Option<HashMap<String, TomlDependency>>,
-    features: Option<HashMap<String, Vec<String>>>,
-    target: Option<HashMap<String, TomlPlatform>>,
-    replace: Option<HashMap<String, TomlDependency>>,
-    patch: Option<HashMap<String, HashMap<String, TomlDependency>>>,
+    build_dependencies2: Option<BTreeMap<String, TomlDependency>>,
+    features: Option<BTreeMap<String, Vec<String>>>,
+    target: Option<BTreeMap<String, TomlPlatform>>,
+    replace: Option<BTreeMap<String, TomlDependency>>,
+    patch: Option<BTreeMap<String, BTreeMap<String, TomlDependency>>>,
     workspace: Option<TomlWorkspace>,
-    badges: Option<HashMap<String, HashMap<String, String>>>,
+    badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
     #[serde(rename = "cargo-features")]
     cargo_features: Option<Vec<String>>,
 }
@@ -475,8 +475,8 @@ impl TomlManifest {
             cargo_features: self.cargo_features.clone(),
         };
 
-        fn map_deps(deps: Option<&HashMap<String, TomlDependency>>)
-                        -> Option<HashMap<String, TomlDependency>>
+        fn map_deps(deps: Option<&BTreeMap<String, TomlDependency>>)
+                        -> Option<BTreeMap<String, TomlDependency>>
         {
             let deps = match deps {
                 Some(deps) => deps,
@@ -557,7 +557,7 @@ impl TomlManifest {
 
             fn process_dependencies(
                 cx: &mut Context,
-                new_deps: Option<&HashMap<String, TomlDependency>>,
+                new_deps: Option<&BTreeMap<String, TomlDependency>>,
                 kind: Option<Kind>)
                 -> CargoResult<()>
             {
@@ -600,7 +600,7 @@ impl TomlManifest {
         }
 
         {
-            let mut names_sources = HashMap::new();
+            let mut names_sources = BTreeMap::new();
             for dep in &deps {
                 let name = dep.name();
                 let prev = names_sources.insert(name, dep.source_id());
@@ -616,7 +616,7 @@ impl TomlManifest {
         let include = project.include.clone().unwrap_or_default();
 
         let summary = Summary::new(pkgid, deps, me.features.clone()
-            .unwrap_or_else(HashMap::new))?;
+            .unwrap_or_else(BTreeMap::new))?;
         let metadata = ManifestMetadata {
             description: project.description.clone(),
             homepage: project.homepage.clone(),
@@ -985,15 +985,15 @@ impl ser::Serialize for PathValue {
 /// Corresponds to a `target` entry, but `TomlTarget` is already used.
 #[derive(Serialize, Deserialize, Debug)]
 struct TomlPlatform {
-    dependencies: Option<HashMap<String, TomlDependency>>,
+    dependencies: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "build-dependencies")]
-    build_dependencies: Option<HashMap<String, TomlDependency>>,
+    build_dependencies: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "build_dependencies")]
-    build_dependencies2: Option<HashMap<String, TomlDependency>>,
+    build_dependencies2: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "dev-dependencies")]
-    dev_dependencies: Option<HashMap<String, TomlDependency>>,
+    dev_dependencies: Option<BTreeMap<String, TomlDependency>>,
     #[serde(rename = "dev_dependencies")]
-    dev_dependencies2: Option<HashMap<String, TomlDependency>>,
+    dev_dependencies2: Option<BTreeMap<String, TomlDependency>>,
 }
 
 impl TomlTarget {

src/crates-io/lib.rs

@@ -8,7 +8,7 @@ extern crate serde_json;
 #[macro_use]
 extern crate serde_derive;
 
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::fs::File;
 use std::io::prelude::*;
 use std::io::{self, Cursor};
@@ -76,7 +76,7 @@ pub struct NewCrate {
     pub name: String,
     pub vers: String,
     pub deps: Vec<NewCrateDependency>,
-    pub features: HashMap<String, Vec<String>>,
+    pub features: BTreeMap<String, Vec<String>>,
     pub authors: Vec<String>,
     pub description: Option<String>,
     pub documentation: Option<String>,
@@ -87,7 +87,7 @@ pub struct NewCrate {
     pub license: Option<String>,
     pub license_file: Option<String>,
     pub repository: Option<String>,
-    pub badges: HashMap<String, HashMap<String, String>>,
+    pub badges: BTreeMap<String, BTreeMap<String, String>>,
 }
 
 #[derive(Serialize)]

tests/package.rs

@@ -700,6 +700,9 @@ to proceed despite this, pass the `--allow-dirty` flag
 
 #[test]
 fn generated_manifest() {
+    Package::new("abc", "1.0.0").publish();
+    Package::new("def", "1.0.0").publish();
+    Package::new("ghi", "1.0.0").publish();
     let p = project("foo")
         .file("Cargo.toml", r#"
             [project]
@@ -717,6 +720,9 @@ fn generated_manifest() {
 
             [dependencies]
             bar = { path = "bar", version = "0.1" }
+            def = "1.0"
+            ghi = "1.0"
+            abc = "1.0"
         "#)
         .file("src/main.rs", "")
         .file("bar/Cargo.toml", r#"
@@ -741,6 +747,8 @@ fn generated_manifest() {
                         .unwrap();
     let mut contents = String::new();
     entry.read_to_string(&mut contents).unwrap();
+    // BTreeMap makes the order of dependencies in the generated file deterministic
+    // by sorting alphabetically
     assert_that(&contents[..], equal_to(
 r#"# THIS FILE IS AUTOMATICALLY GENERATED BY CARGO
 #
@@ -764,8 +772,17 @@ license = "MIT"
 
 [package.metadata]
 foo = "bar"
+[dependencies.abc]
+version = "1.0"
+
 [dependencies.bar]
 version = "0.1"
+
+[dependencies.def]
+version = "1.0"
+
+[dependencies.ghi]
+version = "1.0"
 "#));
 }
 

tests/resolve.rs

@@ -3,7 +3,7 @@
 extern crate hamcrest;
 extern crate cargo;
 
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 
 use hamcrest::{assert_that, equal_to, contains, not};
 
@@ -32,7 +32,7 @@ fn resolve(pkg: &PackageId, deps: Vec<Dependency>, registry: &[Summary])
         fn requires_precise(&self) -> bool { false }
     }
     let mut registry = MyRegistry(registry);
-    let summary = Summary::new(pkg.clone(), deps, HashMap::new()).unwrap();
+    let summary = Summary::new(pkg.clone(), deps, BTreeMap::new()).unwrap();
     let method = Method::Everything;
     let resolve = resolver::resolve(&[(summary, method)], &[], &mut registry, None)?;
     let res = resolve.iter().cloned().collect();
@@ -78,11 +78,11 @@ macro_rules! pkg {
     ($pkgid:expr => [$($deps:expr),+]) => ({
         let d: Vec<Dependency> = vec![$($deps.to_dep()),+];
 
-        Summary::new($pkgid.to_pkgid(), d, HashMap::new()).unwrap()
+        Summary::new($pkgid.to_pkgid(), d, BTreeMap::new()).unwrap()
     });
 
     ($pkgid:expr) => (
-        Summary::new($pkgid.to_pkgid(), Vec::new(), HashMap::new()).unwrap()
+        Summary::new($pkgid.to_pkgid(), Vec::new(), BTreeMap::new()).unwrap()
     )
 }
 
@@ -92,7 +92,7 @@ fn registry_loc() -> SourceId {
 }
 
 fn pkg(name: &str) -> Summary {
-    Summary::new(pkg_id(name), Vec::new(), HashMap::new()).unwrap()
+    Summary::new(pkg_id(name), Vec::new(), BTreeMap::new()).unwrap()
 }
 
 fn pkg_id(name: &str) -> PackageId {
@@ -108,7 +108,7 @@ fn pkg_id_loc(name: &str, loc: &str) -> PackageId {
 }
 
 fn pkg_loc(name: &str, loc: &str) -> Summary {
-    Summary::new(pkg_id_loc(name, loc), Vec::new(), HashMap::new()).unwrap()
+    Summary::new(pkg_id_loc(name, loc), Vec::new(), BTreeMap::new()).unwrap()
 }
 
 fn dep(name: &str) -> Dependency { dep_req(name, "1.0.0") }