Skip to content

fix: canonicalize manifest labels in lockfile digests #3901

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.

Sign up for GitHub

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

Open
brianduff wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: canonicalize manifest labels in lockfile digests #3901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
246 changes: 232 additions & 14 deletions crate_universe/src/lockfile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ use std::process::Command;

use anyhow::{bail, Context as AnyhowContext, Result};
use hex::ToHex;
use serde::{Deserialize, Serialize};
use serde::ser::SerializeMap;
use serde::{Deserialize, Serialize, Serializer};
use sha2::{Digest as Sha2Digest, Sha256};

use crate::config::Config;
use crate::context::Context;
use crate::metadata::Cargo;
use crate::splicing::{SplicingManifest, SplicingMetadata};
use crate::utils::starlark::{Label, Repository};

pub(crate) fn lock_context(
mut context: Context,
Expand Down Expand Up @@ -81,15 +83,15 @@ impl Digest {
cargo_bazel_version,
&cargo_version,
&rustc_version,
),
)?,
None => Self::compute(
context,
config,
&splicing_metadata,
cargo_bazel_version,
&cargo_version,
&rustc_version,
),
)?,
})
}

Expand All @@ -110,10 +112,9 @@ impl Digest {
cargo_bazel_version: &str,
cargo_version: &str,
rustc_version: &str,
) -> Self {
) -> Result<Self> {
// Since this method is private, it should be expected that context is
// always None. This then allows us to have this method not return a
// Result.
// always checksum-free.
debug_assert!(context.checksum.is_none());

let mut hasher = Sha256::new();
Expand All @@ -140,8 +141,15 @@ impl Digest {

// Data collected about Cargo manifests and configs that feed into dependency generation. This file
// is also generated by Bazel behind the scenes based on user inputs.
//
// Manifest labels can differ between root and dependency contexts
// (`//pkg:Cargo.toml` vs `@@repo+//pkg:Cargo.toml`) even when the
// Cargo inputs are otherwise identical. Canonicalize labels only for
// hashing so a checked-in cargo-bazel lockfile stays portable across
// those contexts without changing runtime splicing behavior.
let digest_splicing_metadata = DigestSplicingMetadata::new(splicing_metadata)?;
hasher.update(Digest::compute_single_hash(
&serde_json::to_string(splicing_metadata).unwrap(),
&serde_json::to_string(&digest_splicing_metadata).unwrap(),
"splicing manifest",
));
hasher.update(b"\0");
Expand All @@ -155,7 +163,7 @@ impl Digest {
let hash = hasher.finalize().encode_hex::<String>();
tracing::debug!("Digest hash: {}", hash);

Self(hash)
Ok(Self(hash))
}

pub(crate) fn bin_version(binary: &Path) -> Result<String> {
Expand Down Expand Up @@ -209,6 +217,82 @@ impl Digest {
}
}

#[derive(Serialize)]
struct DigestSplicingMetadata<'a> {
direct_packages: &'a BTreeMap<String, cargo_toml::DependencyDetail>,
// Keep the serialized shape aligned with SplicingMetadata so existing
// digests remain stable when no manifest labels require canonicalization.
// If multiple manifests collapse to the same repo-neutral label, serialize
// them as a stable list for that key instead of discarding any entries.
manifests: DigestManifestMap<'a>,
cargo_config: &'a Option<crate::splicing::cargo_config::CargoConfig>,
}

impl<'a> DigestSplicingMetadata<'a> {
fn new(splicing_metadata: &'a SplicingMetadata) -> Result<Self> {
let mut manifests: BTreeMap<Label, Vec<&'a cargo_toml::Manifest>> = BTreeMap::new();

for (label, manifest) in &splicing_metadata.manifests {
// Repository identity is not semantically relevant to the Cargo
// graph encoded by the lockfile, but package/target paths still
// are, so only drop the repo portion here.
let canonical_label = canonical_manifest_label(label);
manifests.entry(canonical_label).or_default().push(manifest);
}

Ok(Self {
direct_packages: &splicing_metadata.direct_packages,
manifests: DigestManifestMap(manifests),
cargo_config: &splicing_metadata.cargo_config,
})
}
}

struct DigestManifestMap<'a>(BTreeMap<Label, Vec<&'a cargo_toml::Manifest>>);

impl Serialize for DigestManifestMap<'_> {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
let mut map = serializer.serialize_map(Some(self.0.len()))?;

for (label, manifests) in &self.0 {
if manifests.len() == 1 {
map.serialize_entry(label, manifests[0])?;
continue;
}

let mut sorted_manifests = manifests.clone();
sorted_manifests.sort_by(|left, right| {
serde_json::to_string(left)
.unwrap()
.cmp(&serde_json::to_string(right).unwrap())
});
map.serialize_entry(label, &sorted_manifests)?;
}

map.end()
}
}

fn canonical_manifest_label(label: &Label) -> Label {
match label {
Label::Relative { target } => Label::Relative {
target: target.clone(),
},
Label::Absolute {
package, target, ..
} => Label::Absolute {
// Preserve package and target so distinct workspace/package paths
// still hash differently; only canonical repo names are removed.
repository: Repository::Local,
package: package.clone(),
target: target.clone(),
},
}
}

impl PartialEq<str> for Digest {
fn eq(&self, other: &str) -> bool {
self.0 == other
Expand All @@ -225,11 +309,13 @@ impl PartialEq<String> for Digest {
mod test {
use crate::config::{CrateAnnotations, CrateNameAndVersionReq};
use crate::splicing::cargo_config::{AdditionalRegistry, CargoConfig, Registry};
use crate::utils::starlark::Label;
use crate::utils::target_triple::TargetTriple;

use super::*;

use std::collections::BTreeSet;
use std::str::FromStr;

#[test]
fn simple_digest() {
Expand All @@ -244,7 +330,8 @@ mod test {
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
);
)
.unwrap();

assert_eq!(
Digest("edd73970897c01af3bb0e6c9d62f572203dd38a03c189dcca555d463990aa086".to_owned()),
Expand Down Expand Up @@ -289,7 +376,8 @@ mod test {
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
);
)
.unwrap();

assert_eq!(
Digest("8a4c1b3bb4c2d6c36e27565e71a13d54cff9490696a492c66a3a37bdd3893edf".to_owned()),
Expand Down Expand Up @@ -320,7 +408,8 @@ mod test {
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
);
)
.unwrap();

assert_eq!(
Digest("1e01331686ba1f26f707dc098cd9d21c39d6ccd8e46be03329bb2470d3833e15".to_owned()),
Expand Down Expand Up @@ -369,14 +458,141 @@ mod test {
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
);
)
.unwrap();

assert_eq!(
Digest("45ccf7109db2d274420fac521f4736a1fb55450ec60e6df698e1be4dc2c89fad".to_owned()),
digest,
);
}

#[test]
fn digest_ignores_manifest_repository_names() {
let context = Context::default();
let config = Config::default();
let manifest = cargo_toml::Manifest::from_str(
r#"
[package]
name = "lockfile-repro"
version = "0.0.1"
edition = "2021"
"#,
)
.unwrap();

// Same manifest content, but one label is evaluated from the root
// workspace and the other from a dependent module's external repo.
let local = SplicingMetadata {
manifests: BTreeMap::from([(
Label::from_str("//:Cargo.toml").unwrap(),
manifest.clone(),
)]),
..SplicingMetadata::default()
};
let dependency = SplicingMetadata {
manifests: BTreeMap::from([(
Label::from_str("@@published_ruleset+//:Cargo.toml").unwrap(),
manifest,
)]),
..SplicingMetadata::default()
};

let local_digest = Digest::compute(
&context,
&config,
&local,
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
)
.unwrap();

let dependency_digest = Digest::compute(
&context,
&config,
&dependency,
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
)
.unwrap();

assert_eq!(local_digest, dependency_digest);
}

#[test]
fn digest_stable_with_manifest_label_collisions() {
let context = Context::default();
let config = Config::default();
let first_manifest = cargo_toml::Manifest::from_str(
r#"
[package]
name = "collision-check-a"
version = "0.0.1"
edition = "2021"
"#,
)
.unwrap();
let second_manifest = cargo_toml::Manifest::from_str(
r#"
[package]
name = "collision-check-b"
version = "0.0.2"
edition = "2021"
"#,
)
.unwrap();

let splicing_metadata = SplicingMetadata {
manifests: BTreeMap::from([
(
Label::from_str("@first_workspace//pkg:Cargo.toml").unwrap(),
first_manifest.clone(),
),
(
Label::from_str("@@second_workspace+//pkg:Cargo.toml").unwrap(),
second_manifest.clone(),
),
]),
..SplicingMetadata::default()
};
let reversed_splicing_metadata = SplicingMetadata {
manifests: BTreeMap::from([
(
Label::from_str("@@second_workspace+//pkg:Cargo.toml").unwrap(),
second_manifest,
),
(
Label::from_str("@first_workspace//pkg:Cargo.toml").unwrap(),
first_manifest,
),
]),
..SplicingMetadata::default()
};

let digest = Digest::compute(
&context,
&config,
&splicing_metadata,
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
)
.unwrap();
let reversed_digest = Digest::compute(
&context,
&config,
&reversed_splicing_metadata,
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
)
.unwrap();

assert_eq!(digest, reversed_digest);
}

#[test]
fn digest_stable_with_crlf_cargo_config() {
let context = Context::default();
Expand Down Expand Up @@ -413,7 +629,8 @@ mod test {
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
);
)
.unwrap();

let digest_lf = Digest::compute(
&context,
Expand All @@ -422,7 +639,8 @@ mod test {
"0.1.0",
"cargo 1.57.0 (b2e52d7ca 2021-10-21)",
"rustc 1.57.0 (f1edd0429 2021-11-29)",
);
)
.unwrap();

assert_eq!(
digest_crlf, digest_lf,
Expand Down
2 changes: 1 addition & 1 deletion examples/crate_universe/cargo_aliases/cargo-bazel-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion examples/crate_universe/cargo_conditional_deps/cargo-bazel-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion examples/crate_universe/cargo_workspace/cargo-bazel-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion examples/crate_universe/multi_package/cargo-bazel-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion examples/crate_universe/using_cxx/cxxbridge-cmd.cargo-bazel-lock.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"checksum": "ab8b093d4ba7e3bca34a3fc81413c70271914e1b01ed42d22db41c6ea5e9dab6",
"checksum": "1eeb80f1d0fa732cc8155ca4d18d9338651532ba8437ce17eb9422bf99d437f4",
"crates": {
"anstyle 1.0.1": {
"name": "anstyle",
Expand Down