Fix #259 / #227 .bzl macro changes now invalidate targets#342
Merged
Conversation
ef6c1d5 to
6764f81
Compare
…date callers Both issues describe the same underlying gap: when a BUILD file `load()`s a .bzl macro, editing the macro body is not reflected in `bazel-diff get-impacted-targets`. The user in #259 noticed this after upgrading to Bazel 7. The user in #227 hit it with a shared `all_gke_service.bzl` macro loaded by many BUILD files. In both cases, none of the targets that call the macro are reported as impacted. This adds a minimal in-tree workspace `macro_invalidation` plus an `@Ignore`d E2E test: - `miniature.bzl` defines a `miniature(name, src)` macro that wraps `native.genrule`. - `BUILD` does `load(":miniature.bzl", "miniature")` and calls the macro to produce `//:logo_miniature`. - The test generate-hashes against the original workspace, mutates only `miniature.bzl` (changing the genrule cmd), generate-hashes again, then asserts `//:logo_miniature` shows up in `get-impacted-targets`. Today the assertion fails because bazel-diff does not capture .bzl file contents as part of a Rule's transitive hash. Drop the @ignore once that's fixed. Refs #259 Refs #227 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bazel pre-7 populated Build.Rule.skylark_environment_hash_code in the query proto so any change to a .bzl file loaded by a rule's BUILD file naturally bubbled into that rule's hash. Bazel 7+ leaves that field empty, which is the root cause of #259 and #227: editing a macro body (e.g. adding `print()`) no longer invalidated any caller because the emitted rule attrs were identical. Fix: BuildGraphHasher now walks every queried SourceFile target's subincludeList -- the `Build.SourceFile.subinclude` proto field, which already lists every .bzl the BUILD file loaded -- softDigests each main-repo .bzl, and mixes the union of digests into the workspace-wide seed. The seed mix-in is conditional on at least one main-repo .bzl actually being hashed. This keeps the seed (and every target's hash) byte-for-byte stable for workspaces that don't load any tracked .bzl files, preserving cached hashes from before this change. The fix is intentionally conservative -- a single .bzl edit re-hashes every target -- because .bzl edits are rare and silently missing them is the worse failure mode. Per-package precision would require mapping each rule to its package's BUILD file, which isn't a direct dep relationship in the query proto. Also updates the macro_invalidation reproducer to use a `print()`-only mutation (matching the user's exact example in #259), removes the @ignore, and renames the test to a regression test. Refs #259 Refs #227 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b551472 to
dfaf94d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #259 and the duplicate #227.
Root cause. Bazel pre-7 populated
Build.Rule.skylark_environment_hash_codein the query proto, so any change to a.bzlfile loaded by a rule's BUILD file naturally bubbled into that rule's hash. Bazel 7+ leaves that field empty, so an edit like addingprint()to a macro body no longer invalidated any caller — the emitted rule attrs were byte-identical.Fix.
BuildGraphHashernow walks every queriedSourceFiletarget'ssubincludeList(theBuild.SourceFile.subincludeproto field, which already lists every.bzlthe BUILD file loaded),softDigestseach main-repo.bzl, and mixes the union of digests into the workspace-wide seed.Key properties:
.bzlwas actually hashed, so the seed (and every target's hash) is byte-for-byte stable for workspaces that don't load any tracked.bzlfiles. Cached hashes from before this change are preserved for those users..bzlfiles are ignored (@repo//...,@@canonical//...) becauseSourceFileHasher.softDigestreturns null for non-main-repo labels. This keeps the seed stable across BCR fetches that don't actually change repo contents..bzledit re-hashes every target..bzledits are rare; silently missing them is the worse failure mode. Per-package precision would require mapping each rule to its package's BUILD file, which isn't a direct dep relationship in the query proto.Also updates the
macro_invalidationreproducer to use aprint()-only mutation (the exact example from the issue) — the originalcmd =mutation already propagated via rule attrs and wasn't the real bug.@Ignoreremoved; test renamed to a regression test.Test plan
bazel build //cli:bazel-diff //cli:cli-test-libsucceeds.bazel test //cli:BuildGraphHasherTest→ PASSED (verifies the conditional seed mix-in doesn't shift hashes for workspaces with no tracked.bzlfiles).bazel test //cli:E2ETest --test_filter=testMacroBzlChangeImpactsCallers_regressionForIssue259And227→ PASSED in 15.6s (the reproducer that used to fail now succeeds).testE2E$|testE2EWithNoKeepGoing|testMacroBzlChangeImpactsCallers|testBzlmodLocalPathOverride|testModuleBazelComment|testGenerateHashesWithCquery|testFineGrainedHashBzlMod|testBzlmodShowRepo|testExcludeExternalTargets|testCquery|testDetermineBazelVersion|testE2EIncludingTargetType|testE2EWithTargetType) → PASSED in 123s. The four Maven-fixture tests (testFineGrainedHashExternalRepo,testUseCqueryWithExternalDependencyChange,testUseCqueryWithAndroidCodeChange,testBzlmodTransitiveDepsCquery) fail locally withCannot find Java binary bin/javafrom coursier — environmental (the bazel test sandbox doesn't propagateJAVA_HOMEto the nested bazel invocations that runcoursier). CI's setup-java step passes them.print("miniature: " + name)tominiature.bzlproduces//:logo_miniaturein the impacted-targets list (previously it produced an empty list).🤖 Generated with Claude Code