diff --git a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt b/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt index a4aa273..327fcfe 100644 --- a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt +++ b/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt @@ -211,6 +211,12 @@ class BazelQueryService( } else { add("streamed_proto") } + // Each rule's macro instantiation stack; RuleHasher.ruleBzlSeed uses it to attribute a + // loaded `.bzl` to the rules a macro produced, not every target in the package. The + // starlark cquery path emits labels only, so it does not apply there. + if (!(useCquery && outputCompatibleTargets)) { + add("--proto:instantiation_stack") + } if (!useCquery) { add("--order_output=no") } 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 7413239..36ba37b 100644 --- a/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt +++ b/cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt @@ -89,6 +89,13 @@ class BazelRule(private val rule: Build.Rule) { val name: String = rule.name + /** + * Macro instantiation frames (`"::: "`) naming the BUILD/`.bzl` files that + * produced this rule. Empty unless the query ran with `--proto:instantiation_stack`. + */ + val instantiationStack: List + get() = rule.instantiationStackList + /** * The values of this rule's `tags` attribute, or an empty set when the attribute is absent. Bazel * emits user-declared tags as a `STRING_LIST` attribute named `tags` in the query proto. diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt index d8b011f..bc5bc77 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt @@ -70,11 +70,14 @@ class BuildGraphHasher(private val bazelClient: BazelClient) : KoinComponent { } val targetHashStartNanos = System.nanoTime() val seedForFilepaths = runBlocking(Dispatchers.IO) { createSeedForFilepaths(seedFilepaths) } - // Attribute each BUILD file's loaded `.bzl` digests to the package that loads them, so a - // `.bzl` edit only re-hashes targets in packages that actually `load()` it -- not every - // target in the workspace (issue #365). A package that loads no tracked `.bzl` gets nothing - // mixed in, keeping its targets' hashes byte-for-byte stable. - val packageBzlSeeds = createPackageBzlSeeds(allTargets, modifiedFilepaths) + // With macro instantiation stacks (`--proto:instantiation_stack`), RuleHasher.ruleBzlSeed + // attributes each `.bzl` to the rules a macro produced, so the coarse package seed (and its + // source-file over-invalidation) is dropped. Fall back to it when stacks are absent (#365). + val hasInstantiationStacks = + allTargets.any { it is BazelTarget.Rule && it.rule.instantiationStack.isNotEmpty() } + val packageBzlSeeds = + if (hasInstantiationStacks) emptyMap() + else createPackageBzlSeeds(allTargets, modifiedFilepaths) val result = hashAllTargets( seedForFilepaths, diff --git a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt index 2df0055..3fc4f9b 100644 --- a/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt +++ b/cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt @@ -6,6 +6,7 @@ import com.bazel_diff.bazel.decodeConfiguredRuleInputLabel import com.bazel_diff.log.Logger import com.google.common.annotations.VisibleForTesting import java.nio.file.Path +import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.ConcurrentMap import org.koin.core.component.KoinComponent import org.koin.core.component.inject @@ -20,6 +21,46 @@ class RuleHasher( private val logger: Logger by inject() private val sourceFileHasher: SourceFileHasher by inject() + // main-repo `.bzl` path -> content digest; EMPTY marks a non-main-repo/missing file (skip). + private val bzlDigestCache = ConcurrentHashMap() + private val EMPTY = ByteArray(0) + + /** + * Per-rule `.bzl` seed: digests of the main-repo `.bzl` files in this rule's macro instantiation + * stack, so a macro edit re-hashes only the rules that macro produced (issue #365). + * `$rule_implementation_hash` (in [BazelRule.digest]) already covers a rule class's own + * definition `.bzl`. Returns null only when the rule has no stack (`--proto:instantiation_stack` + * off) so the caller falls back to the package seed; a macro-less rule returns a stable constant, + * so the caller does NOT fall back. + */ + private fun ruleBzlSeed(rule: BazelRule, modifiedFilepaths: Set): ByteArray? { + val stack = rule.instantiationStack + if (stack.isEmpty()) return null + val bzlPaths = sortedSetOf() + for (frame in stack) { + val path = frame.substringBefore(":") // "tools/x.bzl:12:3: macro" -> "tools/x.bzl" + if ((path.endsWith(".bzl") || path.endsWith(".scl")) && + !path.startsWith("external/") && + !path.startsWith("@") && + !path.startsWith("../")) { + bzlPaths.add(path) + } + } + return sha256 { + for (path in bzlPaths) { + val digest = + bzlDigestCache.computeIfAbsent(path) { + sourceFileHasher.softDigest( + BazelSourceFileTarget("//$it", ByteArray(0)), modifiedFilepaths) ?: EMPTY + } + if (digest.isNotEmpty()) { + safePutBytes(path.toByteArray()) + safePutBytes(digest) + } + } + } + } + @VisibleForTesting class CircularDependencyException(message: String) : Exception(message) private fun raiseCircularDependency( @@ -67,11 +108,10 @@ class RuleHasher( targetSha256(trackDepLabels) { putDirectBytes(rule.digest(ignoredAttrs)) putDirectBytes(seedHash) - // Mix in the `.bzl` seed for this rule's own package only. Each rule always looks up - // its own package (not the caller's), so this stays consistent under the memoized, - // depth-first recursion below and a macro edit re-hashes only the packages that - // `load()` it (issue #365). - putDirectBytes(packageBzlSeeds[labelToPackage(rule.name)]) + // Per-rule macro seed (see ruleBzlSeed), else the package-wide fallback. Each rule + // resolves its own seed, so this is stable under the memoized recursion below (#365). + putDirectBytes( + ruleBzlSeed(rule, modifiedFilepaths) ?: packageBzlSeeds[labelToPackage(rule.name)]) // Mixed into the *direct* digest (not transitively) so the tagged target is classified // as DIRECT-impacted for distance metrics; it still bubbles into the overall digest, so // any rdeps are conservatively re-hashed too. 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 aa9afb6..4809519 100644 --- a/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt @@ -2743,6 +2743,152 @@ class E2ETest { .isEqualTo(true) } + // Editing an EXISTING widely-loaded rule `.bzl` must be scoped too, not just newly-added macros + // (#365). Fixture `rule_bzl_overtrigger`: `//app` loads `//defs:thing.bzl` for one target + // (`//app:t`) alongside unrelated native targets (`:native_gen`, `:fg`, `:data.txt`). Editing + // `thing.bzl` must impact `//app:t` (its instantiation-stack seed + `$rule_implementation_hash`) + // but MUST NOT impact the native targets that only share its package. + @Test + fun testEditingRuleBzlDoesNotOverInvalidateSamePackageTargets() { + val workspaceA = copyTestWorkspace("rule_bzl_overtrigger") + val workspaceB = copyTestWorkspace("rule_bzl_overtrigger") + + // Change thing.bzl's rule impl without changing any emitted attribute of `thing`. + val bzlInB = File(workspaceB, "defs/thing.bzl") + val original = bzlInB.readText() + val mutated = + original.replace( + "ctx.actions.write(out, \"thing\")", "ctx.actions.write(out, \"thing\") # edited") + assertThat(mutated != original).isEqualTo(true) + bzlInB.writeText(mutated) + + 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()) + assertThat( + cli.execute( + "generate-hashes", "-w", workspaceA.absolutePath, "-b", "bazel", from.absolutePath)) + .isEqualTo(0) + assertThat( + cli.execute( + "generate-hashes", "-w", workspaceB.absolutePath, "-b", "bazel", 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() + + // The target that instantiates the rule from thing.bzl must be impacted. + assertThat(impacted.any { it.endsWith("//app:t") }) + .transform("//app:t should be impacted by a thing.bzl edit; got: $impacted") { it } + .isEqualTo(true) + + // Unrelated native targets sharing the package must NOT be impacted (the over-invalidation). + val overInvalidated = + impacted.filter { + it.endsWith("//app:native_gen") || + it.endsWith("//app:g.txt") || + it.endsWith("//app:fg") || + it.endsWith("//app:data.txt") + } + assertThat(overInvalidated.isEmpty()) + .transform( + "Editing thing.bzl must not impact unrelated //app targets; over-invalidated: $overInvalidated") { + it + } + .isEqualTo(true) + } + + // Cross-file macro/rule: `//app` loads `//defs:macro.bzl`, which loads `//defs:rule.bzl`, for one + // target (`//app:s`). Editing EITHER file must impact `//app:s` -- the macro via its + // instantiation-stack seed, the rule via `$rule_implementation_hash` -- so the scoping does not + // under-invalidate. `//app:unrelated_gen` (a sibling native target) must stay out. + private fun crossFileImpacted(mutate: (File) -> Unit): Set { + val workspaceA = copyTestWorkspace("cross_file_macro_rule") + val workspaceB = copyTestWorkspace("cross_file_macro_rule") + mutate(workspaceB) + + val outputDir = temp.newFolder() + val from = File(outputDir, "starting_hashes.json") + val to = File(outputDir, "final_hashes.json") + val impacted = File(outputDir, "impacted_targets.txt") + + val cli = CommandLine(BazelDiff()) + assertThat( + cli.execute( + "generate-hashes", "-w", workspaceA.absolutePath, "-b", "bazel", from.absolutePath)) + .isEqualTo(0) + assertThat( + cli.execute( + "generate-hashes", "-w", workspaceB.absolutePath, "-b", "bazel", to.absolutePath)) + .isEqualTo(0) + assertThat( + cli.execute( + "get-impacted-targets", + "-w", + workspaceB.absolutePath, + "-b", + "bazel", + "-sh", + from.absolutePath, + "-fh", + to.absolutePath, + "-o", + impacted.absolutePath)) + .isEqualTo(0) + return impacted.readLines().filter { it.isNotBlank() }.toSet() + } + + private fun assertCrossFileConsumerScoped(impacted: Set, edited: String) { + assertThat(impacted.any { it.endsWith("//app:s") }) + .transform("//app:s should be impacted by the $edited edit; got: $impacted") { it } + .isEqualTo(true) + assertThat(impacted.none { it.endsWith("//app:unrelated_gen") || it.endsWith("//app:u.txt") }) + .transform("$edited edit must not impact //app:unrelated_gen; got: $impacted") { it } + .isEqualTo(true) + } + + @Test + fun testEditingCrossFileMacroImpactsConsumerNotSiblings() { + val impacted = crossFileImpacted { ws -> + val f = File(ws, "defs/macro.bzl") + f.writeText( + f.readText() + .replace( + "def split(name, **kwargs):", + "def split(name, **kwargs):\n print(\"split: \" + name)")) + } + assertCrossFileConsumerScoped(impacted, "defs/macro.bzl") + } + + @Test + fun testEditingCrossFileRuleImpactsConsumerNotSiblings() { + val impacted = crossFileImpacted { ws -> + val f = File(ws, "defs/rule.bzl") + f.writeText( + f.readText() + .replace( + "ctx.actions.write(out, \"split\")", + "ctx.actions.write(out, \"split\") # edited")) + } + assertCrossFileConsumerScoped(impacted, "defs/rule.bzl") + } + /** * Reads a `generate-hashes` output file into a flat `target -> hash` map, tolerating both the * bzlmod "new format" (`{"hashes": {...}, "metadata": {...}}`) and the legacy flat format diff --git a/cli/src/test/resources/workspaces/cross_file_macro_rule/MODULE.bazel b/cli/src/test/resources/workspaces/cross_file_macro_rule/MODULE.bazel new file mode 100644 index 0000000..0ee39a4 --- /dev/null +++ b/cli/src/test/resources/workspaces/cross_file_macro_rule/MODULE.bazel @@ -0,0 +1 @@ +module(name = "cross_file_macro_rule", version = "0.0.0") diff --git a/cli/src/test/resources/workspaces/cross_file_macro_rule/WORKSPACE b/cli/src/test/resources/workspaces/cross_file_macro_rule/WORKSPACE new file mode 100644 index 0000000..a754bd0 --- /dev/null +++ b/cli/src/test/resources/workspaces/cross_file_macro_rule/WORKSPACE @@ -0,0 +1 @@ +workspace(name = "cross_file_macro_rule") diff --git a/cli/src/test/resources/workspaces/cross_file_macro_rule/app/BUILD b/cli/src/test/resources/workspaces/cross_file_macro_rule/app/BUILD new file mode 100644 index 0000000..bae05e7 --- /dev/null +++ b/cli/src/test/resources/workspaces/cross_file_macro_rule/app/BUILD @@ -0,0 +1,7 @@ +load("//defs:macro.bzl", "split") + +# Cross-file macro -> rule: must flip when EITHER defs/macro.bzl or defs/rule.bzl is edited. +split(name = "s") + +# Unrelated native target in the same package: must NOT flip on either edit. +genrule(name = "unrelated_gen", outs = ["u.txt"], cmd = "echo u > $@") diff --git a/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/BUILD b/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/BUILD new file mode 100644 index 0000000..2fc211c --- /dev/null +++ b/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/BUILD @@ -0,0 +1 @@ +exports_files(["rule.bzl", "macro.bzl"]) diff --git a/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/macro.bzl b/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/macro.bzl new file mode 100644 index 0000000..c6d9167 --- /dev/null +++ b/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/macro.bzl @@ -0,0 +1,8 @@ +"""The macro, in a SEPARATE file from the rule it instantiates. + +Editing it must flip consumers via the instantiation-stack seed (this file is in the call chain).""" + +load(":rule.bzl", "split_rule") + +def split(name, **kwargs): + split_rule(name = name, **kwargs) diff --git a/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/rule.bzl b/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/rule.bzl new file mode 100644 index 0000000..493fe99 --- /dev/null +++ b/cli/src/test/resources/workspaces/cross_file_macro_rule/defs/rule.bzl @@ -0,0 +1,11 @@ +"""The rule class, in a SEPARATE file from the macro that instantiates it. + +Editing it must flip consumers via `$rule_implementation_hash` (its definition environment), +even though it is not in the instantiation call stack.""" + +def _split_impl(ctx): + out = ctx.actions.declare_file(ctx.label.name + ".out") + ctx.actions.write(out, "split") + return [DefaultInfo(files = depset([out]))] + +split_rule = rule(implementation = _split_impl) diff --git a/cli/src/test/resources/workspaces/rule_bzl_overtrigger/MODULE.bazel b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/MODULE.bazel new file mode 100644 index 0000000..8b8f3bc --- /dev/null +++ b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/MODULE.bazel @@ -0,0 +1 @@ +module(name = "rule_bzl_overtrigger", version = "0.0.0") diff --git a/cli/src/test/resources/workspaces/rule_bzl_overtrigger/WORKSPACE b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/WORKSPACE new file mode 100644 index 0000000..8a85fff --- /dev/null +++ b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/WORKSPACE @@ -0,0 +1 @@ +workspace(name = "rule_bzl_overtrigger") diff --git a/cli/src/test/resources/workspaces/rule_bzl_overtrigger/app/BUILD b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/app/BUILD new file mode 100644 index 0000000..6ddd9d2 --- /dev/null +++ b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/app/BUILD @@ -0,0 +1,9 @@ +load("//defs:thing.bzl", "thing") + +# Uses the rule from thing.bzl -> MUST flip when thing.bzl is edited. +thing(name = "t") + +# Unrelated native targets in the same package -> MUST NOT flip on a thing.bzl edit. +genrule(name = "native_gen", outs = ["g.txt"], cmd = "echo x > $@") +filegroup(name = "fg", srcs = ["data.txt"]) +exports_files(["data.txt"]) diff --git a/cli/src/test/resources/workspaces/rule_bzl_overtrigger/app/data.txt b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/app/data.txt new file mode 100644 index 0000000..e31de1f --- /dev/null +++ b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/app/data.txt @@ -0,0 +1 @@ +seed diff --git a/cli/src/test/resources/workspaces/rule_bzl_overtrigger/defs/BUILD b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/defs/BUILD new file mode 100644 index 0000000..7391ac8 --- /dev/null +++ b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/defs/BUILD @@ -0,0 +1 @@ +exports_files(["thing.bzl"]) diff --git a/cli/src/test/resources/workspaces/rule_bzl_overtrigger/defs/thing.bzl b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/defs/thing.bzl new file mode 100644 index 0000000..10ff176 --- /dev/null +++ b/cli/src/test/resources/workspaces/rule_bzl_overtrigger/defs/thing.bzl @@ -0,0 +1,16 @@ +"""A Starlark rule `_thing` (+ co-located macro `thing`) whose definition digest is `$rule_implementation_hash`. + +Editing this file must impact targets that instantiate `thing`, but MUST NOT +impact unrelated native targets that merely share a package with one -- that +over-invalidation is what packageBzlSeeds caused before instantiation-stack +attribution.""" + +def _thing_impl(ctx): + out = ctx.actions.declare_file(ctx.label.name + ".out") + ctx.actions.write(out, "thing") + return [DefaultInfo(files = depset([out]))] + +_thing = rule(implementation = _thing_impl) + +def thing(name, **kwargs): + _thing(name = name, **kwargs)