Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
7 changes: 7 additions & 0 deletions cli/src/main/kotlin/com/bazel_diff/bazel/BazelRule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ class BazelRule(private val rule: Build.Rule) {

val name: String = rule.name

/**
* Macro instantiation frames (`"<path>:<line>:<col>: <fn>"`) naming the BUILD/`.bzl` files that
* produced this rule. Empty unless the query ran with `--proto:instantiation_stack`.
*/
val instantiationStack: List<String>
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.
Expand Down
13 changes: 8 additions & 5 deletions cli/src/main/kotlin/com/bazel_diff/hash/BuildGraphHasher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
50 changes: 45 additions & 5 deletions cli/src/main/kotlin/com/bazel_diff/hash/RuleHasher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<String, ByteArray>()
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<Path>): ByteArray? {
val stack = rule.instantiationStack
if (stack.isEmpty()) return null
val bzlPaths = sortedSetOf<String>()
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(
Expand Down Expand Up @@ -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.
Expand Down
146 changes: 146 additions & 0 deletions cli/src/test/kotlin/com/bazel_diff/e2e/E2ETest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> {
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<String>, 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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module(name = "cross_file_macro_rule", version = "0.0.0")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
workspace(name = "cross_file_macro_rule")
Original file line number Diff line number Diff line change
@@ -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 > $@")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports_files(["rule.bzl", "macro.bzl"])
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module(name = "rule_bzl_overtrigger", version = "0.0.0")
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
workspace(name = "rule_bzl_overtrigger")
Original file line number Diff line number Diff line change
@@ -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"])
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
seed
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports_files(["thing.bzl"])
Original file line number Diff line number Diff line change
@@ -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)
Loading