diff --git a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt b/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt index 9156e44..d4f5760 100644 --- a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt +++ b/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt @@ -145,7 +145,12 @@ class BazelRule(private val rule: Build.Rule) { ): String { if (isNotMainRepo(ruleInput) && ruleInput.startsWith("@") && - fineGrainedHashExternalRepos.none { ruleInput.startsWith(it) }) { + // Match on the full repo name (up to the `//` boundary), not a bare string + // prefix: with hub-and-spoke repos (e.g. rules_python's `@pip` hub and + // `@pip_` spokes), a bare prefix check makes every spoke label look + // like it belongs to the fine-grained hub, so it is never rewritten to its + // `//external:` seed and changes stop propagating. + fineGrainedHashExternalRepos.none { ruleInput == it || ruleInput.startsWith("$it//") }) { val splitRule = ruleInput.split("//".toRegex()).dropLastWhile { it.isEmpty() }.toTypedArray() if (splitRule.size == 2) { var externalRule = splitRule[0] diff --git a/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt index 5e12cfe..3bb7478 100644 --- a/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelRuleTest.kt @@ -189,6 +189,31 @@ class BazelRuleTest { assertThat(inputs).isEqualTo(listOf("//:legacy_dep")) } + // Hub-and-spoke external repos (e.g. rules_python's `@pip` hub with one `@pip_` spoke + // repo per package): a spoke label shares the hub name as a string prefix but is a different + // repo, so marking the hub fine-grained must not stop spoke inputs from being rewritten to + // their `//external:` synthetic target -- that target is the only node whose hash + // flips when the spoke's pinned version changes. + @Test + fun testFineGrainedRepoNameDoesNotPrefixMatchSpokeRepos() { + val rule = + Rule.newBuilder() + .setRuleClass("alias") + .setName("@pip//numpy:pkg") + .addRuleInput("@pip_numpy//:pkg") + .addRuleInput("@pip//numpy:other") + .build() + + val inputs = + BazelRule(rule) + .ruleInputList(useCquery = false, fineGrainedHashExternalRepos = setOf("@pip")) + + // The spoke input is rewritten to its seed; the hub-internal input stays raw because the + // hub itself is fine-grained. + assertThat(inputs.contains("//external:pip_numpy")).isEqualTo(true) + assertThat(inputs.contains("//external:pip")).isEqualTo(false) + } + // Pins the round-trip behaviour `RuleHasher` relies on: the full encoded string lives in the // hash, and the bare label is what gets looked up in `allRulesMap` / `sourceDigests` and // tracked in `deps`. If the round-trip ever drifts the user-facing JSON would start emitting diff --git a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt index a385aa2..89cf932 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -1929,6 +1929,20 @@ class E2ETest { hasModShowRepo) } + /** + * Skips the calling test unless the local Bazel still loads WORKSPACE files. Bazel 9 removed + * WORKSPACE support outright: `--enable_workspace` is inert there, so a fixture whose repos are + * declared in a WORKSPACE file has no `@repo` to query at all. + */ + private fun assumeBazelSupportsWorkspace() { + val version = getBazelVersion() + org.junit.Assume.assumeNotNull(version) + val v = version!! + org.junit.Assume.assumeTrue( + "Requires a Bazel with WORKSPACE support (current: ${v.first}.${v.second}.${v.third})", + v.first < 9) + } + @Test fun testGenerateHashesIsHermeticAcrossWorkspacePaths() { // Two checkouts of byte-identical sources at different absolute paths -- standing in for the @@ -2689,6 +2703,112 @@ class E2ETest { .isEqualTo(true) } + // ------------------------------------------------------------------------ + // Fine-grained hub name must not prefix-match spoke repos (hub-and-spoke) + // ------------------------------------------------------------------------ + // Hub-and-spoke external repos -- e.g. rules_python's pip_parse in WORKSPACE mode, with a + // `@pip` hub of alias packages and one `@pip_` spoke repo per package -- put the + // change-detection signal in the spoke: bumping a pinned version rewrites the spoke + // repository rule's attrs, so only `//external:pip_` flips. Consumers reference the hub + // alias (`@pip//numpy:pkg`), so the chain to the flipping seed runs through the alias's + // input on the spoke label. Related shape: the alias-wrap chain in issue #197. + // + // The `hub_spoke_external` fixture wires this up with a custom repository rule (WORKSPACE + // mode -- the collision lives on the classic //external: path): + // @pip_numpy -> spoke repo; `version` attr is the pin literal + // @pip -> hub repo; numpy/BUILD alias `pkg` -> @pip_numpy//:lib + // //:consumer -> genrule consuming @pip//numpy:pkg + // + // With `--fineGrainedHashExternalRepos @pip`, `transformRuleInput` used to match + // fine-grained repos by unbounded string prefix, so the alias's spoke input + // (`@pip_numpy//:lib`) looked like it belonged to the fine-grained hub and was never + // rewritten to `//external:pip_numpy` -- the only node whose hash flips on the version + // bump. Consumers were silently dropped from the impacted set. The fix in + // `BazelRule.transformRuleInput` matches repo names up to the `//` boundary. + @Test + fun testHubSpokeVersionBumpImpactsConsumer_fineGrainedHubPrefixCollision() { + // WORKSPACE-only fixture: Bazel 9 dropped WORKSPACE, so `@pip` does not exist there at all. + // The matching logic itself stays pinned on every version by `BazelRuleTest`. + assumeBazelSupportsWorkspace() + + val workspaceA = copyTestWorkspace("hub_spoke_external") + val workspaceB = copyTestWorkspace("hub_spoke_external") + + // Bump only the spoke's pinned version in B -- a repository-rule attribute literal, the + // same signal a real pip lockfile bump produces. + val workspaceFileInB = File(workspaceB, "WORKSPACE") + workspaceFileInB.writeText( + workspaceFileInB.readText().replace("version = \"1.0\"", "version = \"2.0\"")) + + val outputDir = temp.newFolder() + val from = File(outputDir, "starting_hashes.json") + val to = File(outputDir, "final_hashes.json") + val impactedTargetsOutput = File(outputDir, "impacted_targets.txt") + + val cli = CommandLine(BazelDiff()) + + // Only the hub is listed: users name the repo their BUILD files reference and don't + // expect to enumerate every generated spoke behind it. + val fineGrained = "@pip" + + assertThat( + cli.execute( + "generate-hashes", + "-w", + workspaceA.absolutePath, + "-b", + "bazel", + "--fineGrainedHashExternalRepos", + fineGrained, + from.absolutePath)) + .isEqualTo(0) + assertThat( + cli.execute( + "generate-hashes", + "-w", + workspaceB.absolutePath, + "-b", + "bazel", + "--fineGrainedHashExternalRepos", + fineGrained, + to.absolutePath)) + .isEqualTo(0) + assertThat( + cli.execute( + "get-impacted-targets", + "-w", + workspaceB.absolutePath, + "-b", + "bazel", + "-sh", + from.absolutePath, + "-fh", + to.absolutePath, + "-o", + impactedTargetsOutput.absolutePath)) + .isEqualTo(0) + + val impacted = impactedTargetsOutput.readLines().filter { it.isNotBlank() }.toSet() + + // Sanity: the spoke seed must flip on the version bump; if this fails the fixture no + // longer reproduces the signal and the regression assertion below proves nothing. + val spokeSeedImpacted = impacted.any { it == "//external:pip_numpy" } + assertThat(spokeSeedImpacted) + .transform( + "//external:pip_numpy should be impacted by the spoke version bump. Got impacted: $impacted") { + it + } + .isEqualTo(true) + + val consumerImpacted = impacted.any { it == "//:consumer" || it == "@@//:consumer" } + assertThat(consumerImpacted) + .transform( + "//:consumer should be impacted (chain: @pip//numpy:pkg -> @pip_numpy//:lib -> //external:pip_numpy). Got impacted: $impacted") { + it + } + .isEqualTo(true) + } + // ------------------------------------------------------------------------ // Hermetic fine-grained external-repo hashing across workspaces // ------------------------------------------------------------------------ diff --git a/cli/src/test/resources/workspaces/hub_spoke_external/.bazelrc b/cli/src/test/resources/workspaces/hub_spoke_external/.bazelrc new file mode 100644 index 0000000..eb11625 --- /dev/null +++ b/cli/src/test/resources/workspaces/hub_spoke_external/.bazelrc @@ -0,0 +1,4 @@ +# The prefix collision under test lives on the WORKSPACE //external: path; +# run this fixture in WORKSPACE mode. +common --noenable_bzlmod +common --enable_workspace diff --git a/cli/src/test/resources/workspaces/hub_spoke_external/BUILD b/cli/src/test/resources/workspaces/hub_spoke_external/BUILD new file mode 100644 index 0000000..3ad490a --- /dev/null +++ b/cli/src/test/resources/workspaces/hub_spoke_external/BUILD @@ -0,0 +1,8 @@ +# Consumer behind the hub alias: +# //:consumer -> @pip//numpy:pkg (hub alias) -> @pip_numpy//:lib (spoke) +genrule( + name = "consumer", + srcs = ["@pip//numpy:pkg"], + outs = ["consumer.out"], + cmd = "cat $(SRCS) > $@", +) diff --git a/cli/src/test/resources/workspaces/hub_spoke_external/WORKSPACE b/cli/src/test/resources/workspaces/hub_spoke_external/WORKSPACE new file mode 100644 index 0000000..974cb79 --- /dev/null +++ b/cli/src/test/resources/workspaces/hub_spoke_external/WORKSPACE @@ -0,0 +1,13 @@ +workspace(name = "hub_spoke_external_test") + +load("//:fake_pip.bzl", "fake_hub", "fake_spoke") + +fake_spoke( + name = "pip_numpy", + version = "1.0", +) + +fake_hub( + name = "pip", + spoke = "pip_numpy", +) diff --git a/cli/src/test/resources/workspaces/hub_spoke_external/fake_pip.bzl b/cli/src/test/resources/workspaces/hub_spoke_external/fake_pip.bzl new file mode 100644 index 0000000..d1b6002 --- /dev/null +++ b/cli/src/test/resources/workspaces/hub_spoke_external/fake_pip.bzl @@ -0,0 +1,43 @@ +"""Hub-and-spoke external repos modeled after rules_python's pip_parse (WORKSPACE mode). + +- One spoke repo per package whose repository-rule attrs embed the pinned + version, so a version bump flips the //external: seed hash. +- A hub repo of alias packages that consumers reference: + @pip//numpy:pkg -> @pip_numpy//:lib. +""" + +def _fake_spoke_impl(rctx): + rctx.file("WORKSPACE", "workspace(name = \"{}\")\n".format(rctx.name)) + rctx.file( + "BUILD", + "filegroup(\n" + + " name = \"lib\",\n" + + " srcs = [\"payload.txt\"],\n" + + " visibility = [\"//visibility:public\"],\n" + + ")\n", + ) + rctx.file("payload.txt", "version {}\n".format(rctx.attr.version)) + +fake_spoke = repository_rule( + implementation = _fake_spoke_impl, + # The version literal in this attr is the change-detection signal, exactly + # like the requirement string in a real pip spoke repo. + attrs = {"version": attr.string(mandatory = True)}, +) + +def _fake_hub_impl(rctx): + rctx.file("WORKSPACE", "workspace(name = \"{}\")\n".format(rctx.name)) + rctx.file("BUILD", "") + rctx.file( + "numpy/BUILD", + "alias(\n" + + " name = \"pkg\",\n" + + " actual = \"@{}//:lib\",\n".format(rctx.attr.spoke) + + " visibility = [\"//visibility:public\"],\n" + + ")\n", + ) + +fake_hub = repository_rule( + implementation = _fake_hub_impl, + attrs = {"spoke": attr.string(mandatory = True)}, +) diff --git a/src/hash.rs b/src/hash.rs index 023f19c..c615786 100644 --- a/src/hash.rs +++ b/src/hash.rs @@ -669,9 +669,15 @@ fn transform_external_input<'a>( let is_main = input.starts_with("//") || input.starts_with("@//") || input.starts_with("@@//"); if !is_main && input.starts_with('@') - && !fine_grained_repos - .iter() - .any(|repo| input.starts_with(repo)) + // Match on the full repo name (up to the `//` boundary), not a bare string prefix: with + // hub-and-spoke repos (e.g. rules_python's `@pip` hub and `@pip_` spokes), a bare + // prefix check makes every spoke label look like it belongs to the fine-grained hub, so it + // is never rewritten to its `//external:` seed and changes stop propagating. + && !fine_grained_repos.iter().any(|repo| { + input + .strip_prefix(repo.as_str()) + .is_some_and(|rest| rest.is_empty() || rest.starts_with("//")) + }) { if let Some((repo, _)) = input.split_once("//") { return Cow::Owned(format!("//external:{}", repo.trim_start_matches('@'))); @@ -1223,6 +1229,41 @@ mod tests { ); } + // Hub-and-spoke external repos (e.g. rules_python's `@pip` hub with one `@pip_` spoke + // repo per package): a spoke label shares the hub name as a string prefix but is a different + // repo, so marking the hub fine-grained must not stop spoke inputs from being rewritten to + // their `//external:` synthetic target -- that target is the only node whose hash flips + // when the spoke's pinned version changes. + #[test] + fn fine_grained_repo_name_does_not_prefix_match_spoke_repos() { + let fine_grained = BTreeSet::from(["@pip".to_owned()]); + assert_eq!( + transform_external_input("@pip_numpy//:lib", &fine_grained), + "//external:pip_numpy" + ); + assert_eq!( + transform_external_input("@pip//numpy:pkg", &fine_grained), + "@pip//numpy:pkg" + ); + + let rule = Rule { + name: "@pip//numpy:pkg".into(), + rule_class: "alias".into(), + rule_input: vec!["@pip_numpy//:lib".into(), "@pip//numpy:other".into()], + ..Default::default() + }; + // The spoke input keeps its raw label and gains the seed; the hub-internal input stays raw + // because the hub itself is fine-grained. + assert_eq!( + rule_inputs(&rule, false, &fine_grained), + [ + "//external:pip_numpy", + "@pip//numpy:other", + "@pip_numpy//:lib" + ] + ); + } + #[test] fn absolute_external_instantiation_frames_are_not_main_repo_inputs() { let frames = [ diff --git a/tests/e2e/external.rs b/tests/e2e/external.rs index c33eb28..b70d032 100644 --- a/tests/e2e/external.rs +++ b/tests/e2e/external.rs @@ -229,6 +229,40 @@ fn remote_proto_bump_impacts_consumer() { .any(|label| label.ends_with("proto_dep//:greeting_java_proto"))); } +// Hub-and-spoke external repos -- e.g. rules_python's pip_parse in WORKSPACE mode, with a `@pip` +// hub of alias packages and one `@pip_` spoke repo per package -- keep the change-detection +// signal in the spoke: bumping a pinned version rewrites the spoke repository rule's attrs, so only +// `//external:pip_numpy` flips. Users list the hub their BUILD files reference (`@pip`) rather than +// every generated spoke, and the consumer reaches the flipping seed through +// `@pip//numpy:pkg -> @pip_numpy//:lib -> //external:pip_numpy`. Fine-grained matching used to be an +// unbounded string prefix, so the spoke label looked like it belonged to the hub and was never +// rewritten to its seed, silently dropping consumers from the impacted set. +#[test] +fn hub_spoke_version_bump_impacts_consumer_behind_fine_grained_hub() { + // WORKSPACE-only fixture: Bazel 9 dropped WORKSPACE, so `@pip` does not exist there at all. + // The matching logic itself stays pinned on every version by the `hash` unit tests. + if !supports_workspace() { + return; + } + let first = copy_workspace("hub_spoke_external"); + let second = copy_workspace("hub_spoke_external"); + edit(&second.path().join("WORKSPACE"), |text| { + text.replace("version = \"1.0\"", "version = \"2.0\"") + }); + let impacted = diff_local( + first.path(), + second.path(), + &["--fineGrainedHashExternalRepos", "@pip"], + ); + // Sanity: if the spoke seed stops flipping the fixture no longer reproduces the signal and the + // consumer assertion below proves nothing. + assert!(impacted.contains("//external:pip_numpy"), "{impacted:?}"); + assert!( + impacted.contains("//:consumer") || impacted.contains("@@//:consumer"), + "{impacted:?}" + ); +} + fn diff_local( first: &std::path::Path, second: &std::path::Path, diff --git a/tests/e2e/support/mod.rs b/tests/e2e/support/mod.rs index 13f280a..2f0ab82 100644 --- a/tests/e2e/support/mod.rs +++ b/tests/e2e/support/mod.rs @@ -148,6 +148,12 @@ pub fn supports_mod_show_repo() -> bool { version >= (8, 6, 0) && version != (9, 0, 0) } +/// Whether the local Bazel still loads WORKSPACE files. Bazel 9 removed WORKSPACE support +/// outright, so a fixture whose repos are declared in a WORKSPACE file has no `@repo` to query. +pub fn supports_workspace() -> bool { + bazel_version().0 < 9 +} + /// The `bazel-diff` binary under test. /// /// `CARGO_BIN_EXE_*` is a cargo-only compile-time variable, so under Bazel the