diff --git a/README.md b/README.md index 445c7f51..e79a4cfd 100644 --- a/README.md +++ b/README.md @@ -751,6 +751,9 @@ A few inputs are normalized or extended on the way in: hashed without its `url`/`urls`, since a mirror or registry move cannot change what it fetches. The main-repository files its attributes name -- patches, build files, lock files -- are hashed by content, so a changed patch is a changed repository. + Registry patches, overlay files and the module's `MODULE.bazel` carry their own integrity, so + their URLs are left out wherever that integrity is set; the integrity is still hashed. A module + from a `file://%workspace%/...` registry therefore hashes the same in every checkout. Bzlmod repositories are read from `bazel mod show_repo`, asked for by name in as many invocations as the platform's command-line limit allows, or in one `--all_repos` invocation when that would take several. The limit is 8191 characters wherever `cmd.exe` is involved -- diff --git a/src/bazel.rs b/src/bazel.rs index 1ee6d4c7..2810ce41 100644 --- a/src/bazel.rs +++ b/src/bazel.rs @@ -1873,9 +1873,10 @@ fn repository_local_input_labels(repository: &Repository) -> BTreeSet { .collect() } -/// Digest of a repository's own definition: its rule, its attributes (less the download URLs -/// when a `sha256`/`integrity` pins what they fetch, as the hasher treats them), and the local -/// tree and main-repository files behind them. +/// Digest of a repository's own definition: its rule, its attributes, and the local tree and +/// main-repository files behind them. Like the target hasher, it leaves out URLs whose content an +/// integrity pins: `url`/`urls` under `sha256`/`integrity`, and the URLs +/// `without_integrity_pinned_urls` drops. fn repository_definition_hash(repository: &Repository, workspace: &Path) -> String { let pinned = repository.attribute.iter().any(|attribute| { matches!(attribute.name.as_str(), "sha256" | "integrity") @@ -1893,7 +1894,10 @@ fn repository_definition_hash(repository: &Repository, workspace: &Path) -> Stri continue; } encoded.clear(); - attribute.encode_length_delimited(&mut encoded); + match crate::hash::without_integrity_pinned_urls(attribute, &repository.attribute) { + Some(stripped) => stripped.encode_length_delimited(&mut encoded), + None => attribute.encode_length_delimited(&mut encoded), + } hasher.update(&encoded); } hasher.update([0]); @@ -3432,6 +3436,31 @@ exit 1 repository_definition_hash(&unpinned, workspace.path()), repository_definition_hash(&moved, workspace.path()) ); + + // The digest also ignores the checkout path in a `file://%workspace%` registry's patch + // URLs, which their integrity pins; a changed integrity still changes it. + let patched = |root: &str, integrity: &str| Repository { + repo_rule_name: Some("http_archive".into()), + attribute: vec![Attribute { + name: "remote_patches".into(), + r#type: attribute::Discriminator::StringDict, + string_dict_value: vec![crate::proto::blaze_query::StringDictEntry { + key: format!("file://{root}/registry/patches/fix.patch"), + value: integrity.into(), + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }; + assert_eq!( + repository_definition_hash(&patched("/one", "sha256-p"), workspace.path()), + repository_definition_hash(&patched("/two", "sha256-p"), workspace.path()) + ); + assert_ne!( + repository_definition_hash(&patched("/one", "sha256-p"), workspace.path()), + repository_definition_hash(&patched("/one", "sha256-q"), workspace.path()) + ); } /// Fake Bazel for the extension-repository flow: a root that imports `rules_go` and a diff --git a/src/hash.rs b/src/hash.rs index 025e3175..43ed5259 100644 --- a/src/hash.rs +++ b/src/hash.rs @@ -877,7 +877,7 @@ impl EncodeSink for Sha256Sink<'_> { /// Repository rules (`//external:*`) that pin their download with `sha256` or `integrity` fetch /// byte-identical content from any URL, so a mirror or registry move must not impact every -/// target that depends on the repository. Only such rules get their URL attributes skipped. +/// target that depends on the repository. Only such rules get their `url`/`urls` skipped. fn pins_download_content(rule: &Rule) -> bool { rule.name.starts_with("//external:") && rule.attribute.iter().any(|attribute| { @@ -889,6 +889,64 @@ fn pins_download_content(rule: &Rule) -> bool { }) } +/// Registry patches, overlay files and the module's `MODULE.bazel` each carry their own integrity +/// (the `remote_patches` values, `remote_file_integrity`, `remote_module_file_integrity`), so, +/// like a pinned `url`/`urls`, their URLs cannot change what is fetched. A +/// `file://%workspace%/...` registry expands to the checkout's absolute path, so hashing these +/// URLs would give identical checkouts at different paths different hashes. Returns a copy of +/// `attribute` without the URLs that an integrity in `repository_attributes` pins, or `None` when +/// `attribute` is none of `remote_patches`, `remote_file_urls` and `remote_module_file_urls`. +pub(crate) fn without_integrity_pinned_urls( + attribute: &crate::proto::blaze_query::Attribute, + repository_attributes: &[crate::proto::blaze_query::Attribute], +) -> Option { + if !matches!( + attribute.name.as_str(), + "remote_patches" | "remote_file_urls" | "remote_module_file_urls" + ) { + return None; + } + let mut stripped = attribute.clone(); + match attribute.name.as_str() { + "remote_patches" => { + // Entries keep their order, which is the order the patches apply in. + for entry in &mut stripped.string_dict_value { + if !entry.value.is_empty() { + entry.key.clear(); + } + } + } + "remote_module_file_urls" => { + let pinned = repository_attributes.iter().any(|other| { + other.name == "remote_module_file_integrity" + && other + .string_value + .as_deref() + .is_some_and(|value| !value.is_empty()) + }); + if pinned { + stripped.string_list_value.clear(); + } + } + "remote_file_urls" => { + let pinned = repository_attributes + .iter() + .filter(|other| other.name == "remote_file_integrity") + .flat_map(|other| &other.string_dict_value) + .filter(|entry| !entry.value.is_empty()) + .map(|entry| entry.key.as_str()) + .collect::>(); + for entry in &mut stripped.string_list_dict_value { + if pinned.contains(entry.key.as_str()) { + entry.value.clear(); + } + } + } + _ => return None, + } + Some(stripped) +} + fn rule_definition_digest(rule: &Rule, ignored: &HashSet) -> DigestBytes { let mut hasher = Sha256::new(); hasher.update(rule.rule_class.as_bytes()); @@ -897,6 +955,10 @@ fn rule_definition_digest(rule: &Rule, ignored: &HashSet) -> DigestBytes hasher.update(environment_hash.as_bytes()); } let skip_urls = pins_download_content(rule); + let repository_attributes = rule + .name + .starts_with("//external:") + .then_some(rule.attribute.as_slice()); let mut size_cache = SizeCache::new(); let attributes_are_sorted = rule .attribute @@ -904,13 +966,27 @@ fn rule_definition_digest(rule: &Rule, ignored: &HashSet) -> DigestBytes .all(|pair| pair[0].name <= pair[1].name); if attributes_are_sorted { for attribute in &rule.attribute { - hash_attribute(attribute, ignored, skip_urls, &mut size_cache, &mut hasher); + hash_attribute( + attribute, + ignored, + skip_urls, + repository_attributes, + &mut size_cache, + &mut hasher, + ); } } else { let mut attributes = rule.attribute.iter().collect::>(); attributes.sort_by_key(|attribute| &attribute.name); for attribute in attributes { - hash_attribute(attribute, ignored, skip_urls, &mut size_cache, &mut hasher); + hash_attribute( + attribute, + ignored, + skip_urls, + repository_attributes, + &mut size_cache, + &mut hasher, + ); } } hasher.finalize().into() @@ -927,6 +1003,7 @@ fn hash_attribute( attribute: &crate::proto::blaze_query::Attribute, ignored: &HashSet, skip_urls: bool, + repository_attributes: Option<&[crate::proto::blaze_query::Attribute]>, size_cache: &mut SizeCache, hasher: &mut Sha256, ) { @@ -936,6 +1013,12 @@ fn hash_attribute( if skip_urls && matches!(attribute.name.as_str(), "url" | "urls") { return; } + if let Some(stripped) = repository_attributes + .and_then(|attributes| without_integrity_pinned_urls(attribute, attributes)) + { + stripped.encode_with_cache(size_cache, &mut Sha256Sink(hasher)); + return; + } attribute.encode_with_cache(size_cache, &mut Sha256Sink(hasher)); } @@ -2096,6 +2179,7 @@ mod tests { &attribute, &HashSet::new(), false, + None, &mut SizeCache::new(), &mut included, ); @@ -2104,6 +2188,7 @@ mod tests { &attribute, &HashSet::from(["cmd".to_owned()]), false, + None, &mut SizeCache::new(), &mut ignored, ); @@ -2908,6 +2993,146 @@ mod tests { ); } + #[test] + fn external_rule_digest_ignores_registry_urls_pinned_by_their_integrity() { + use crate::proto::blaze_query::{StringDictEntry, StringListDictEntry}; + + fn dict(name: &str, entries: Vec<(String, String)>) -> Attribute { + Attribute { + name: name.into(), + r#type: attribute::Discriminator::StringDict, + string_dict_value: entries + .into_iter() + .map(|(key, value)| StringDictEntry { + key, + value, + ..Default::default() + }) + .collect(), + ..Default::default() + } + } + // A registry inside the workspace, as Bazel expands `file://%workspace%/registry`. + fn registry_rule( + root: &str, + patches: &[(&str, &str)], + overlay_integrity: &str, + module_integrity: &str, + ) -> Rule { + Rule { + name: "//external:dep".into(), + rule_class: "http_archive".into(), + attribute: vec![ + dict( + "remote_file_integrity", + vec![("BUILD.bazel".into(), overlay_integrity.into())], + ), + Attribute { + name: "remote_file_urls".into(), + r#type: attribute::Discriminator::StringListDict, + string_list_dict_value: vec![StringListDictEntry { + key: "BUILD.bazel".into(), + value: vec![format!("file://{root}/registry/overlay/BUILD.bazel")], + ..Default::default() + }], + ..Default::default() + }, + Attribute { + name: "remote_module_file_integrity".into(), + r#type: attribute::Discriminator::String, + string_value: Some(module_integrity.into()), + ..Default::default() + }, + Attribute { + name: "remote_module_file_urls".into(), + r#type: attribute::Discriminator::StringList, + string_list_value: vec![format!("file://{root}/registry/MODULE.bazel")], + ..Default::default() + }, + dict( + "remote_patches", + patches + .iter() + .map(|(name, integrity)| { + ( + format!("file://{root}/registry/patches/{name}"), + (*integrity).into(), + ) + }) + .collect(), + ), + ], + ..Default::default() + } + } + let none = HashSet::new(); + let patches = [("a.patch", "sha256-a"), ("b.patch", "sha256-b")]; + let at = |root: &str, patches: &[(&str, &str)], overlay: &str, module: &str| { + rule_definition_digest(®istry_rule(root, patches, overlay, module), &none) + }; + let here = at( + "/checkouts/one", + &patches, + "sha256-overlay", + "sha256-module", + ); + assert_eq!( + here, + at( + "/checkouts/two", + &patches, + "sha256-overlay", + "sha256-module" + ) + ); + + // The pins still count: a changed or reordered patch, or a changed overlay file or + // MODULE.bazel, is a different repository. + let changed_patch = [("a.patch", "sha256-a2"), ("b.patch", "sha256-b")]; + let reordered = [("b.patch", "sha256-b"), ("a.patch", "sha256-a")]; + for changed in [ + at( + "/checkouts/one", + &changed_patch, + "sha256-overlay", + "sha256-module", + ), + at( + "/checkouts/one", + &reordered, + "sha256-overlay", + "sha256-module", + ), + at( + "/checkouts/one", + &patches, + "sha256-overlay2", + "sha256-module", + ), + at( + "/checkouts/one", + &patches, + "sha256-overlay", + "sha256-module2", + ), + ] { + assert_ne!(here, changed); + } + + // A URL that its integrity does not pin still counts, for each kind on its own. + let unpinned_patch = [("a.patch", ""), ("b.patch", "sha256-b")]; + for (patches, overlay, module) in [ + (&unpinned_patch[..], "sha256-overlay", "sha256-module"), + (&patches[..], "", "sha256-module"), + (&patches[..], "sha256-overlay", ""), + ] { + assert_ne!( + at("/checkouts/one", patches, overlay, module), + at("/checkouts/two", patches, overlay, module) + ); + } + } + #[test] fn soft_digest_rejects_directories() { let workspace = tempfile::tempdir().unwrap(); diff --git a/tests/e2e/regressions.rs b/tests/e2e/regressions.rs index ece7958e..555ded72 100644 --- a/tests/e2e/regressions.rs +++ b/tests/e2e/regressions.rs @@ -363,6 +363,65 @@ fn single_version_override_patch_change_reaches_consumer_cquery() { ]); } +/// Regression for https://github.com/Tinder/bazel-diff/issues/544: the local registry lives inside +/// the workspace (`write_local_registry` registers it by its absolute path, which is what +/// `%workspace%` expands to), so the `file://` URLs of `dep`'s registry patch and `MODULE.bazel` +/// name each checkout's absolute path. Two identical checkouts at different paths must impact +/// nothing, while a changed registry patch, which changes its integrity, still reaches +/// `//:consumer`. +#[test] +fn registry_urls_under_the_workspace_do_not_depend_on_its_path() { + if !supports_mod_show_repo() { + return; + } + // A fresh copy per state instead of editing one in place: a Bazel server caches a local + // registry's files and would not see an edited patch. + let first = copy_workspace("single_version_override_patch"); + let second = copy_workspace("single_version_override_patch"); + let changed = copy_workspace("single_version_override_patch"); + for (workspace, content) in [ + (first.path(), "v1"), + (second.path(), "v1"), + (changed.path(), "v2"), + ] { + write_local_registry(workspace); + add_registry_patch(workspace, content); + } + let impacted = diff_workspaces(first.path(), second.path(), &[]); + assert!(impacted.is_empty(), "{impacted:?}"); + + let impacted = diff_workspaces(first.path(), changed.path(), &[]); + assert!( + impacted + .iter() + .any(|label| label == "//:consumer" || label == "@@//:consumer"), + "{impacted:?}" + ); + assert!(!impacted.contains("//:control"), "{impacted:?}"); +} + +/// Adds a patch that creates `registry.txt` containing `content` to `dep`'s registry entry (the +/// `source.json` that `write_local_registry` writes). +fn add_registry_patch(workspace: &Path, content: &str) { + use sha2::{Digest, Sha256}; + + let module = workspace.join("registry/modules/dep/1.0"); + let patch = format!("--- /dev/null\n+++ b/registry.txt\n@@ -0,0 +1 @@\n+{content}\n"); + fs::create_dir_all(module.join("patches")).unwrap(); + fs::write(module.join("patches/registry.patch"), &patch).unwrap(); + let source_path = module.join("source.json"); + let source = fs::read_to_string(&source_path).unwrap(); + let source = source.trim_end().strip_suffix('}').unwrap(); + fs::write( + &source_path, + format!( + "{source}, \"patches\": {{\"registry.patch\": \"sha256-{}\"}}, \"patch_strip\": 1}}\n", + base64(&Sha256::digest(patch.as_bytes())) + ), + ) + .unwrap(); +} + /// Turns the `single_version_override_patch` fixture into a working Bzlmod registry: zips /// `dep_src/` into the archive the registry's `source.json` points at (a `file://` URL and its /// integrity depend on the temp path, so neither is checked in) and adds the registry to the diff --git a/tools/e2e/rust_e2e_cases.bzl b/tools/e2e/rust_e2e_cases.bzl index 30b23440..7aa5203a 100644 --- a/tools/e2e/rust_e2e_cases.bzl +++ b/tools/e2e/rust_e2e_cases.bzl @@ -187,6 +187,10 @@ RUST_E2E_SUITES = [ "name": "regressions::referenced_failing_extension_fails_without_keep_going", "timeout": "moderate", }, + { + "name": "regressions::registry_urls_under_the_workspace_do_not_depend_on_its_path", + "timeout": "moderate", + }, { "name": "regressions::rule_bzl_change_is_scoped_to_consumer", "timeout": "moderate",