Stop depending on PackageId format for node ordering

In rust-lang/cargo#12914, the format used for
PackageId strings in cargo metadata was changed. This led to a number of
test failures due to the ordering of nodes when logging graphs changing
relative to earlier versions of cargo.

To work around this issue, the ordering of nodes in the graph was
changed to be based on package name and version explicitly, rather than
implicitly through the PackageId string. This slightly changes the
ordering of some crates in outputs.

In addition, in order to keep tests passing across all versions, the
package_id member has been hidden from the JSON graph dump output. This
field was already missing for path dependencies.

Fixes mozilla#602

Author: mystor

src/resolver.rs

@@ -180,9 +180,12 @@ pub type PackageIdx = usize;
 
 #[derive(Debug, Clone, Serialize)]
 pub struct PackageNode<'a> {
-    #[serde(skip_serializing_if = "pkgid_unstable")]
+    #[serde(skip)]
     /// The PackageId that cargo uses to uniquely identify this package
     ///
+    /// This ID is not guaranteed to be stable across cargo versions, so is not
+    /// serialized into graph JSON.
+    ///
     /// Prefer using a [`DepGraph`] and its memoized [`PackageIdx`]'s.
     pub package_id: &'a PackageId,
     /// The name of the package
@@ -211,11 +214,6 @@ pub struct PackageNode<'a> {
     pub is_dev_only: bool,
 }
 
-/// Don't serialize path package ids, not stable across systems
-fn pkgid_unstable(pkgid: &PackageId) -> bool {
-    pkgid.repr.contains("(path+file:/")
-}
-
 /// The dependency graph in a form we can use more easily.
 #[derive(Debug, Clone)]
 pub struct DepGraph<'a> {
@@ -449,9 +447,13 @@ impl<'a> DepGraph<'a> {
             });
         }
 
-        // Sort the nodes by package_id to make the graph more stable and to make
-        // anything sorted by package_idx to also be approximately sorted by name and version.
-        nodes.sort_by_key(|k| k.package_id);
+        // Sort the nodes by package name and version to make the graph as
+        // stable as possible.  We avoid sorting by the package_id if possible,
+        // as for some packages it may not be stable (e.g. file:///), and the
+        // package_id format can also vary between cargo versions.
+        nodes.sort_by(|a, b| {
+            (a.name, &a.version, &a.package_id).cmp(&(b.name, &b.version, &b.package_id))
+        });
 
         // Populate the interners based on the new ordering
         for (idx, node) in nodes.iter_mut().enumerate() {

src/tests/snapshots/cargo_vet__tests__vet__builtin-complex-full-audited.json.snap

@@ -7,11 +7,11 @@ expression: json
   "vetted_fully": [
     {
       "name": "third-core",
-      "version": "10.0.0"
+      "version": "5.0.0"
     },
     {
       "name": "third-core",
-      "version": "5.0.0"
+      "version": "10.0.0"
     },
     {
       "name": "thirdA",

src/tests/snapshots/cargo_vet__tests__vet__builtin-complex-inited.json.snap

@@ -9,11 +9,11 @@ expression: json
   "vetted_with_exemptions": [
     {
       "name": "third-core",
-      "version": "10.0.0"
+      "version": "5.0.0"
     },
     {
       "name": "third-core",
-      "version": "5.0.0"
+      "version": "10.0.0"
     },
     {
       "name": "thirdA",

src/tests/snapshots/cargo_vet__tests__vet__builtin-complex-minimal-audited.json.snap

@@ -7,11 +7,11 @@ expression: json
   "vetted_fully": [
     {
       "name": "third-core",
-      "version": "10.0.0"
+      "version": "5.0.0"
     },
     {
       "name": "third-core",
-      "version": "5.0.0"
+      "version": "10.0.0"
     },
     {
       "name": "thirdA",

src/tests/snapshots/cargo_vet__tests__vet__builtin-complex-no-unaudited.json.snap

@@ -7,14 +7,14 @@ expression: json
   "failures": [
     {
       "name": "third-core",
-      "version": "10.0.0",
+      "version": "5.0.0",
       "missing_criteria": [
         "safe-to-deploy"
       ]
     },
     {
       "name": "third-core",
-      "version": "5.0.0",
+      "version": "10.0.0",
       "missing_criteria": [
         "safe-to-deploy"
       ]

src/tests/snapshots/cargo_vet__tests__vet__builtin-simple-unaudited-twins.json.snap

@@ -18,11 +18,11 @@ expression: json
   "vetted_with_exemptions": [
     {
       "name": "third-core",
-      "version": "10.0.0"
+      "version": "5.0.0"
     },
     {
       "name": "third-core",
-      "version": "5.0.0"
+      "version": "10.0.0"
     }
   ]
 }

src/tests/snapshots/cargo_vet__tests__vet__mock-complex-full-audited.json.snap

@@ -7,11 +7,11 @@ expression: json
   "vetted_fully": [
     {
       "name": "third-core",
-      "version": "10.0.0"
+      "version": "5.0.0"
     },
     {
       "name": "third-core",
-      "version": "5.0.0"
+      "version": "10.0.0"
     },
     {
       "name": "thirdA",

src/tests/snapshots/cargo_vet__tests__vet__mock-complex-inited.json.snap

@@ -9,11 +9,11 @@ expression: json
   "vetted_with_exemptions": [
     {
       "name": "third-core",
-      "version": "10.0.0"
+      "version": "5.0.0"
     },
     {
       "name": "third-core",
-      "version": "5.0.0"
+      "version": "10.0.0"
     },
     {
       "name": "thirdA",

src/tests/snapshots/cargo_vet__tests__vet__mock-complex-no-unaudited.json.snap

@@ -7,14 +7,14 @@ expression: json
   "failures": [
     {
       "name": "third-core",
-      "version": "10.0.0",
+      "version": "5.0.0",
       "missing_criteria": [
         "reviewed"
       ]
     },
     {
       "name": "third-core",
-      "version": "5.0.0",
+      "version": "10.0.0",
       "missing_criteria": [
         "reviewed"
       ]