Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes in cargo metadata PackageId format leading to test failures #602

Closed
mystor opened this issue Apr 1, 2024 · 0 comments · Fixed by #603
Closed

Changes in cargo metadata PackageId format leading to test failures #602

mystor opened this issue Apr 1, 2024 · 0 comments · Fixed by #603

Comments

@mystor
Copy link
Collaborator

mystor commented Apr 1, 2024

It appears that somewhat recently cargo metadata changed the format of the package_id field in its JSON output (presumably rust-lang/cargo#12914). Significantly, this changed the way that packages are sorted in internal data structures, such that while previously crate 1.1.1 and crate 1.1.1@git:XXX would have sorted adjacent to one-another, they now sort further apart.

cargo-vet/src/resolver.rs

Lines 452 to 454 in abb7411

// 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);

The new format for this field is a PackageIdSpec, which is supposedly going to be a more stable format going forwards. This format change also will have broken one case where we did inspect the internal format of the PackageId (despite it being documented as opaque).

cargo-vet/src/resolver.rs

Lines 214 to 217 in abb7411

/// Don't serialize path package ids, not stable across systems
fn pkgid_unstable(pkgid: &PackageId) -> bool {
pkgid.repr.contains("(path+file:/")
}

The file URI in this case now looks more like path+file:///path/to/example#0.1.0 rather than example 0.1.0 (path+file:///path/to/example), so will no longer match the contains check.

In order to keep tests passing with both older and newer versions of rustc, we'll likely need to tweak how we sort packages to avoid using package_id for sorting when possible. In addition, there are some commands where the output contains the package id, specifically the dump-graph test, which will likely need to be updated in some way - likely by removing the unstable PackageId check, and instead never serializing package IDs.

mystor added a commit to mystor/cargo-vet that referenced this issue Apr 1, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant