Skip to content

Commit

Permalink
Auto merge of #8824 - ehuss:normalize-source-master, r=alexcrichton
Browse files Browse the repository at this point in the history 
Normalize SourceID in `cargo metadata`.

The SourceID in `cargo metadata` can have different values, but they can be equivalent in Cargo. This results in different serialized forms, which prevents comparing the ID strings. In this particular case, `SourceKind::Git(GitReference::Branch("master"))` is equivalent to `SourceKind::Git(GitReference::DefaultBranch)`, but they serialize differently.

The reason these end up differently is because the `SourceId` for a `Package` is created from the `Dependency` declaration. But the `SourceId` in `Cargo.lock` comes from the deserialized file. If you have an explicit `branch = "master"` in the dependency, then versions prior to 1.47 would *not* include `?branch=master` in `Cargo.lock`. However, since 1.47, internally Cargo will use `GitReference::Branch("master")`.

Conversely, if you have a new `Cargo.lock` (with `?branch=master`), and then *remove* the explicit `branch="master"` from `Cargo.toml`, you'll end up with another mismatch in `cargo metadata`.

The solution here is to use the variant from the `Package` when serializing the resolver in `cargo metadata`. I chose this since the `Package` variant is displayed on other JSON messages (like artifact messages), and I think this is the only place that the resolver variants are exposed (other than `Cargo.lock` itself).

I'm not convinced that this was entirely intended, since there is [code to avoid this](https://github.com/rust-lang/cargo/blob/6a38927551df9bbe2dea340bf92d3e53abccf890/src/cargo/core/resolver/encode.rs#L688-L695), and at the time #8522 landed, I did not realize this would change the V2 lock format. However, it's probably too late to try to reverse that, and I don't think there are any other problems other than this `cargo metadata` inconsistency.

Fixes #8756
  • Loading branch information
@bors
bors committed Nov 2, 2020
2 parents 03137ed + d88ed1c commit c46c06a
Show file tree
Hide file tree
Showing 2 changed files with 224 additions and 3 deletions.
21 changes: 18 additions & 3 deletions src/cargo/ops/cargo_output_metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,21 @@ fn build_resolve_graph_r(
if node_map.contains_key(&pkg_id) {
return;
}
// This normalizes the IDs so that they are consistent between the
// `packages` array and the `resolve` map. This is a bit of a hack to
// compensate for the fact that
// SourceKind::Git(GitReference::Branch("master")) is the same as
// SourceKind::Git(GitReference::DefaultBranch). We want IDs in the JSON
// to be opaque, and compare with basic string equality, so this will
// always prefer the style of ID in the Package instead of the resolver.
// Cargo generally only exposes PackageIds from the Package struct, and
// AFAIK this is the only place where the resolver variant is exposed.
//
// This diverges because the SourceIds created for Packages are built
// based on the Dependency declaration, but the SourceIds in the resolver
// are deserialized from Cargo.lock. Cargo.lock may have been generated by
// an older (or newer!) version of Cargo which uses a different style.
let normalize_id = |id| -> PackageId { *package_map.get_key_value(&id).unwrap().0 };
let features = resolve.features(pkg_id).to_vec();

let deps: Vec<Dep> = resolve
Expand All @@ -203,15 +218,15 @@ fn build_resolve_graph_r(
.and_then(|lib_target| resolve.extern_crate_name(pkg_id, dep_id, lib_target).ok())
.map(|name| Dep {
name,
pkg: dep_id,
pkg: normalize_id(dep_id),
dep_kinds,
})
})
.collect();
let dumb_deps: Vec<PackageId> = deps.iter().map(|dep| dep.pkg).collect();
let dumb_deps: Vec<PackageId> = deps.iter().map(|dep| normalize_id(dep.pkg)).collect();
let to_visit = dumb_deps.clone();
let node = MetadataResolveNode {
id: pkg_id,
id: normalize_id(pkg_id),
dependencies: dumb_deps,
deps,
features,
Expand Down
206 changes: 206 additions & 0 deletions tests/testsuite/git.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3020,3 +3020,209 @@ warning: [..]
)
.run();
}

#[cargo_test]
fn metadata_master_consistency() {
// SourceId consistency in the `cargo metadata` output when `master` is
// explicit or implicit, using new or old Cargo.lock.
let (git_project, git_repo) = git::new_repo("bar", |project| {
project
.file("Cargo.toml", &basic_manifest("bar", "1.0.0"))
.file("src/lib.rs", "")
});
let bar_hash = git_repo.head().unwrap().target().unwrap().to_string();

// Explicit branch="master" with a lock file created before 1.47 (does not contain ?branch=master).
let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = {{ git = "{}", branch = "master" }}
"#,
git_project.url()
),
)
.file(
"Cargo.lock",
&format!(
r#"
[[package]]
name = "bar"
version = "1.0.0"
source = "git+{}#{}"
[[package]]
name = "foo"
version = "0.1.0"
dependencies = [
"bar",
]
"#,
git_project.url(),
bar_hash,
),
)
.file("src/lib.rs", "")
.build();

let metadata = |bar_source| -> String {
r#"
{
"packages": [
{
"name": "bar",
"version": "1.0.0",
"id": "bar 1.0.0 (__BAR_SOURCE__#__BAR_HASH__)",
"license": null,
"license_file": null,
"description": null,
"source": "__BAR_SOURCE__#__BAR_HASH__",
"dependencies": [],
"targets": "{...}",
"features": {},
"manifest_path": "[..]",
"metadata": null,
"publish": null,
"authors": [],
"categories": [],
"keywords": [],
"readme": null,
"repository": null,
"homepage": null,
"documentation": null,
"edition": "2015",
"links": null
},
{
"name": "foo",
"version": "0.1.0",
"id": "foo 0.1.0 [..]",
"license": null,
"license_file": null,
"description": null,
"source": null,
"dependencies": [
{
"name": "bar",
"source": "__BAR_SOURCE__",
"req": "*",
"kind": null,
"rename": null,
"optional": false,
"uses_default_features": true,
"features": [],
"target": null,
"registry": null
}
],
"targets": "{...}",
"features": {},
"manifest_path": "[..]",
"metadata": null,
"publish": null,
"authors": [],
"categories": [],
"keywords": [],
"readme": null,
"repository": null,
"homepage": null,
"documentation": null,
"edition": "2015",
"links": null
}
],
"workspace_members": [
"foo 0.1.0 [..]"
],
"resolve": {
"nodes": [
{
"id": "bar 1.0.0 (__BAR_SOURCE__#__BAR_HASH__)",
"dependencies": [],
"deps": [],
"features": []
},
{
"id": "foo 0.1.0 [..]",
"dependencies": [
"bar 1.0.0 (__BAR_SOURCE__#__BAR_HASH__)"
],
"deps": [
{
"name": "bar",
"pkg": "bar 1.0.0 (__BAR_SOURCE__#__BAR_HASH__)",
"dep_kinds": [
{
"kind": null,
"target": null
}
]
}
],
"features": []
}
],
"root": "foo 0.1.0 [..]"
},
"target_directory": "[..]",
"version": 1,
"workspace_root": "[..]",
"metadata": null
}
"#
.replace("__BAR_SOURCE__", bar_source)
.replace("__BAR_HASH__", &bar_hash)
};

let bar_source = format!("git+{}?branch=master", git_project.url());
p.cargo("metadata").with_json(&metadata(&bar_source)).run();

// Conversely, remove branch="master" from Cargo.toml, but use a new Cargo.lock that has ?branch=master.
let p = project()
.file(
"Cargo.toml",
&format!(
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
bar = {{ git = "{}" }}
"#,
git_project.url()
),
)
.file(
"Cargo.lock",
&format!(
r#"
[[package]]
name = "bar"
version = "1.0.0"
source = "git+{}?branch=master#{}"
[[package]]
name = "foo"
version = "0.1.0"
dependencies = [
"bar",
]
"#,
git_project.url(),
bar_hash
),
)
.file("src/lib.rs", "")
.build();

// No ?branch=master!
let bar_source = format!("git+{}", git_project.url());
p.cargo("metadata").with_json(&metadata(&bar_source)).run();
}

0 comments on commit c46c06a

Please sign in to comment.