From 496cc84302b8a53b68a4d8310fcd35e085b78acb Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 10:41:07 -0700 Subject: [PATCH 01/23] feat(targethasher): collapse bzlmod external targets to repo-level hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For bzlmod repos (@@repo+...//...), source file and generated file targets are now pre-hashed using a single hash derived from the canonical repo name before the DFS traversal in HashRecursively. This mirrors the existing //external:repo collapsing for legacy WORKSPACE repos but adapted for bzlmod's @@repo//... naming convention. The canonical bzlmod repo name encodes the module version and content hash (e.g. "rules_python++pip+third_party_python_base_311_torch_..._a6ebbe51"), so hashing the name produces a stable, content-aware representative hash that changes when the repo content changes. On uber-one (2.49M targets, 82% external bzlmod source files): - Collapsed 2,042,237 targets in 1.84s - HashRecursively: 899s → 38s (23.7x speedup), single-threaded - Total compute: 969s → 108s (9x speedup) Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 73 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 2610ad1f..0fa81f5f 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -386,6 +386,69 @@ func HashExternalTargets(ctx context.Context, r *buildpb.QueryResult, targets ma return nil } +// bzlmodRepoName extracts the canonical bzlmod repo name from a target label. +// For "@@rules_python++pip+foo//pkg:target" it returns "rules_python++pip+foo". +// Returns "" for non-bzlmod targets. +func bzlmodRepoName(targetName string) string { + if !strings.HasPrefix(targetName, "@@") { + return "" + } + // Strip leading "@@", then find "//" separator. + rest := targetName[2:] + idx := strings.Index(rest, "//") + if idx <= 0 { + return "" + } + return rest[:idx] +} + +// collapseBzlmodExternalTargets pre-hashes all bzlmod external source file +// and generated file targets using a single hash derived from the canonical +// repo name. This mirrors the legacy WORKSPACE //external:repo collapsing +// but for bzlmod's @@repo//... naming. Rule targets are left alone so their +// real dependency edges are preserved. +// +// The canonical bzlmod repo name encodes the module version and content hash +// (e.g. "rules_python++pip+third_party_python_base_311_torch_...._a6ebbe51"), +// so hashing the name itself produces a stable, content-aware representative +// hash that changes when the repo content changes. +func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string]) int { + // Build per-repo hashes lazily. + repoHashes := make(map[string][]byte) + collapsed := 0 + + for name, target := range targets { + repo := bzlmodRepoName(name) + if repo == "" { + continue + } + if fullHashRepos.Contains(repo) { + continue + } + if target.RuleType != SourceFileType && target.RuleType != GeneratedFileType { + continue + } + // Already hashed (shouldn't happen at this point, but be safe). + if target.Hash != nil { + continue + } + + h, ok := repoHashes[repo] + if !ok { + rh := newHash() + rh.Write([]byte(repo)) + h = rh.Sum(nil) + repoHashes[repo] = h + } + + target.Hash = h + target.HashWithoutDeps = h + collapsed++ + } + + return collapsed +} + // GetTopologicalRootsAndIdentifyBuildableRoots returns a list of topological roots and marks buildable roots in the target graph func GetTopologicalRootsAndIdentifyBuildableRoots(targets map[string]*Target) []string { // get targets that cannot be root, i.e. dependencies of some other targets @@ -434,6 +497,16 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, return EmptyResult(), err } } + + // For bzlmod repos, collapse external source/generated file targets to a + // single per-repo hash derived from the canonical repo name (which encodes + // the version and content hash in bzlmod). This avoids visiting millions of + // individual pip-wheel files during the DFS — the same optimization that + // legacy WORKSPACE gets via //external:repo collapsing. + if useBzlmod { + collapseBzlmodExternalTargets(targets, fullHashRepos) + } + // get topological roots and update buildable roots info roots := GetTopologicalRootsAndIdentifyBuildableRoots(targets) From 986eea4373c8e4a348c9ff4ebef6cc2a5d204ad5 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 12:06:59 -0700 Subject: [PATCH 02/23] Fix collapseBzlmodExternalTargets to respect excluded_files regex The collapse ran before HashRecursively and didn't check excludedRegex, so excluded external targets (e.g. rules_python sdist/whl patterns) got a repo-name hash instead of the standard empty-hash treatment. Now excluded targets are skipped by the collapse and fall through to HashRecursively for proper exclusion handling. Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 0fa81f5f..2a451727 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -412,7 +412,7 @@ func bzlmodRepoName(targetName string) string { // (e.g. "rules_python++pip+third_party_python_base_311_torch_...._a6ebbe51"), // so hashing the name itself produces a stable, content-aware representative // hash that changes when the repo content changes. -func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string]) int { +func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) int { // Build per-repo hashes lazily. repoHashes := make(map[string][]byte) collapsed := 0 @@ -432,6 +432,12 @@ func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set if target.Hash != nil { continue } + // Let excluded targets fall through to HashRecursively where they + // get the standard empty-hash treatment, keeping the same semantics + // as legacy WORKSPACE exclusion. + if isExcluded(name, excludedRegex) { + continue + } h, ok := repoHashes[repo] if !ok { @@ -504,7 +510,7 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, // individual pip-wheel files during the DFS — the same optimization that // legacy WORKSPACE gets via //external:repo collapsing. if useBzlmod { - collapseBzlmodExternalTargets(targets, fullHashRepos) + collapseBzlmodExternalTargets(targets, fullHashRepos, excludedRegex) } // get topological roots and update buildable roots info From e46077c42542d07e20f4aeeac21fb536b0677c72 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 13:40:01 -0700 Subject: [PATCH 03/23] Consolidate bzlmod/WORKSPACE external handling into else block and add tests Move collapseBzlmodExternalTargets into the else branch of the !useBzlmod check since the two paths are mutually exclusive. Remove the collapsed count return value. Add unit tests for bzlmodRepoName and collapseBzlmodExternalTargets covering: collapse of source/generated files, rule target skip, fullHashRepos skip, excluded regex skip, different repos get different hashes, already-hashed skip, and an end-to-end fromProto test verifying excluded bzlmod targets get empty hash. Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 27 +++---- core/targethasher/graph_test.go | 129 ++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 17 deletions(-) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 2a451727..978538d2 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -412,10 +412,8 @@ func bzlmodRepoName(targetName string) string { // (e.g. "rules_python++pip+third_party_python_base_311_torch_...._a6ebbe51"), // so hashing the name itself produces a stable, content-aware representative // hash that changes when the repo content changes. -func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) int { - // Build per-repo hashes lazily. +func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) { repoHashes := make(map[string][]byte) - collapsed := 0 for name, target := range targets { repo := bzlmodRepoName(name) @@ -428,7 +426,6 @@ func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set if target.RuleType != SourceFileType && target.RuleType != GeneratedFileType { continue } - // Already hashed (shouldn't happen at this point, but be safe). if target.Hash != nil { continue } @@ -449,10 +446,7 @@ func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set target.Hash = h target.HashWithoutDeps = h - collapsed++ } - - return collapsed } // GetTopologicalRootsAndIdentifyBuildableRoots returns a list of topological roots and marks buildable roots in the target graph @@ -496,20 +490,19 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, return EmptyResult(), err } - // add external rule targets (//external:*) to the same map and hash them - // no need for bzlmod because there's no //external:* rules, we will hash external source as is if !useBzlmod { + // Legacy WORKSPACE: add external rule targets (//external:*) to the + // map and hash them. No //external:* rules exist under bzlmod. if err := HashExternalTargets(ctx, r, targets, hasher, workspaceroot, fullHashRepos, warns, useBzlmod); err != nil { return EmptyResult(), err } - } - - // For bzlmod repos, collapse external source/generated file targets to a - // single per-repo hash derived from the canonical repo name (which encodes - // the version and content hash in bzlmod). This avoids visiting millions of - // individual pip-wheel files during the DFS — the same optimization that - // legacy WORKSPACE gets via //external:repo collapsing. - if useBzlmod { + } else { + // Bzlmod: collapse external source/generated file targets to a + // single per-repo hash derived from the canonical repo name (which + // encodes the version and content hash). This avoids visiting + // millions of individual pip-wheel files during the DFS — the same + // optimization that legacy WORKSPACE gets via //external:repo + // collapsing. collapseBzlmodExternalTargets(targets, fullHashRepos, excludedRegex) } diff --git a/core/targethasher/graph_test.go b/core/targethasher/graph_test.go index f3cebd5c..abba2b6a 100644 --- a/core/targethasher/graph_test.go +++ b/core/targethasher/graph_test.go @@ -382,6 +382,135 @@ func assertEqualTargetHash(t *testing.T, expected, actual Target) { assert.True(t, cmp.Equal(expected, actual, opt, ignore), cmp.Diff(expected, actual, opt)) } +func TestBzlmodRepoName(t *testing.T) { + tests := []struct { + name string + target string + want string + }{ + {"bzlmod source file", "@@rules_python++pip+foo//pkg:file.py", "rules_python++pip+foo"}, + {"bzlmod rule", "@@io_bazel_rules_go//go:def.bzl", "io_bazel_rules_go"}, + {"internal target", "//src/pkg:lib", ""}, + {"single @", "@repo//pkg:target", ""}, + {"no double slash", "@@repo", ""}, + {"empty repo name", "@@//pkg:target", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, bzlmodRepoName(tt.target)) + }) + } +} + +func TestCollapseBzlmodExternalTargets(t *testing.T) { + t.Run("collapses source and generated files", func(t *testing.T) { + targets := map[string]*Target{ + "@@repo_a//pkg:file1.py": {Name: "@@repo_a//pkg:file1.py", RuleType: SourceFileType, External: true}, + "@@repo_a//pkg:file2.py": {Name: "@@repo_a//pkg:file2.py", RuleType: SourceFileType, External: true}, + "@@repo_a//pkg:gen.go": {Name: "@@repo_a//pkg:gen.go", RuleType: GeneratedFileType, External: true}, + "//src:main": {Name: "//src:main", RuleType: "go_binary"}, + } + + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + + // All three external files should share the same repo-derived hash. + assert.NotNil(t, targets["@@repo_a//pkg:file1.py"].Hash) + assert.Equal(t, targets["@@repo_a//pkg:file1.py"].Hash, targets["@@repo_a//pkg:file2.py"].Hash) + assert.Equal(t, targets["@@repo_a//pkg:file1.py"].Hash, targets["@@repo_a//pkg:gen.go"].Hash) + + // Internal target should be untouched. + assert.Nil(t, targets["//src:main"].Hash) + }) + + t.Run("skips rule targets", func(t *testing.T) { + targets := map[string]*Target{ + "@@repo_a//pkg:lib": {Name: "@@repo_a//pkg:lib", RuleType: "go_library", External: true}, + } + + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + assert.Nil(t, targets["@@repo_a//pkg:lib"].Hash) + }) + + t.Run("skips fullHashRepos", func(t *testing.T) { + targets := map[string]*Target{ + "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, + } + + collapseBzlmodExternalTargets(targets, set.NewSet("", "repo_a"), nil) + assert.Nil(t, targets["@@repo_a//pkg:file.py"].Hash) + }) + + t.Run("skips excluded targets", func(t *testing.T) { + targets := map[string]*Target{ + "@@rules_python++pip+foo//pkg:file.whl": {Name: "@@rules_python++pip+foo//pkg:file.whl", RuleType: SourceFileType, External: true}, + "@@rules_python++pip+foo//pkg:module.py": {Name: "@@rules_python++pip+foo//pkg:module.py", RuleType: SourceFileType, External: true}, + } + excluded := []*regexp.Regexp{regexp.MustCompile(`\.whl$`)} + + collapseBzlmodExternalTargets(targets, set.NewSet(""), excluded) + + // .whl file should NOT be collapsed — left for HashRecursively to exclude. + assert.Nil(t, targets["@@rules_python++pip+foo//pkg:file.whl"].Hash) + // .py file should be collapsed normally. + assert.NotNil(t, targets["@@rules_python++pip+foo//pkg:module.py"].Hash) + }) + + t.Run("different repos get different hashes", func(t *testing.T) { + targets := map[string]*Target{ + "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, + "@@repo_b//pkg:file.py": {Name: "@@repo_b//pkg:file.py", RuleType: SourceFileType, External: true}, + } + + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + assert.NotEqual(t, targets["@@repo_a//pkg:file.py"].Hash, targets["@@repo_b//pkg:file.py"].Hash) + }) + + t.Run("already hashed targets are skipped", func(t *testing.T) { + existing := []byte{1, 2, 3} + targets := map[string]*Target{ + "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true, Hash: existing}, + } + + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + assert.Equal(t, existing, targets["@@repo_a//pkg:file.py"].Hash) + }) +} + +func TestFromProtoExcludedBzlmodTargetGetsEmptyHash(t *testing.T) { + // End-to-end: an excluded bzlmod source file should get empty hash, + // not a repo-name hash from the collapse. + qr := &buildpb.QueryResult{ + Target: []*buildpb.Target{ + { + Type: buildpb.Target_RULE.Enum(), + Rule: &buildpb.Rule{ + Name: StringPtr("//pkg:app"), + RuleClass: StringPtr("go_binary"), + RuleInput: []string{"@@dep++v1//lib:file.whl"}, + }, + }, + { + Type: buildpb.Target_SOURCE_FILE.Enum(), + SourceFile: &buildpb.SourceFile{Name: StringPtr("@@dep++v1//lib:file.whl")}, + }, + { + Type: buildpb.Target_SOURCE_FILE.Enum(), + SourceFile: &buildpb.SourceFile{Name: StringPtr("@@dep++v1//lib:module.py")}, + }, + }, + } + + excluded := []*regexp.Regexp{regexp.MustCompile(`\.whl$`)} + result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), excluded, true) + require.NoError(t, err) + + // Excluded .whl target should have empty hash (not a repo-name hash). + assert.Empty(t, result.Targets["@@dep++v1//lib:file.whl"].Hash) + // Non-excluded .py in same repo should still have a hash from collapse. + // (noOpHasher zeros all hashes, but the target should still exist) + assert.Contains(t, result.Targets, "@@dep++v1//lib:module.py") +} + func Test_fromProto(t *testing.T) { ctrl := gomock.NewController(t) ctx := context.WithValue(context.Background(), struct{}{}, "source-hash") From f419ccf2121c80a236c8eb2af52a80f9a369e9ee Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 15:44:33 -0700 Subject: [PATCH 04/23] Use Bazel marker file hashes for bzlmod external target collapsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of hashing the repo name string (which doesn't change for repos like protobuf+ when the version is bumped), read the content hash from Bazel's marker files at $(output_base)/external/@repo.marker. The marker hash is derived from the repository rule's inputs (URL, sha256, patches) and changes on any upgrade. If no marker file exists for a repo, skip collapsing entirely and let HashRecursively hash the actual file content — this ensures correctness for repos we can't identify as content-addressed. Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 31 ++++++---- core/targethasher/graph_test.go | 59 +++++++++++++------ graphrunner/markers.go | 100 ++++++++++++++++++++++++++++++++ graphrunner/native.go | 15 +++++ 4 files changed, 175 insertions(+), 30 deletions(-) create mode 100644 graphrunner/markers.go diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 978538d2..7efdf5d0 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -78,6 +78,12 @@ type HashConfig struct { // AllTargetsFiles lists repo-relative paths whose hashes should be // extracted from KnownSourceHashes into Result.AllTargetsFileHashes. AllTargetsFiles []string + // RepoMarkerHashes maps canonical bzlmod repo names to content hashes + // read from Bazel's marker files ($(output_base)/external/@repo.marker). + // When set, collapseBzlmodExternalTargets uses these instead of hashing + // the repo name string, so that repos without a content hash suffix in + // their name (e.g. "protobuf+") still get a hash that changes on upgrade. + RepoMarkerHashes map[string][]byte } // Target contains information about the hash for a single target @@ -155,7 +161,7 @@ func FromProto(ctx context.Context, r *buildpb.QueryResult, workspaceroot string result, err := fromProto(ctx, r, &diskHashHelper{ workspaceroot: workspaceroot, knownFileHashes: hashConfig.KnownSourceHashes, - }, workspaceroot, fullHashRepos, set.NewSet(hashConfig.SequentialHashTargets...), excludedRegex, hashConfig.UseBzlmod) + }, workspaceroot, fullHashRepos, set.NewSet(hashConfig.SequentialHashTargets...), excludedRegex, hashConfig.UseBzlmod, hashConfig.RepoMarkerHashes) if err != nil { return result, err } @@ -176,7 +182,7 @@ func FromProto(ctx context.Context, r *buildpb.QueryResult, workspaceroot string // FromProtoNoHash calculates a DAG graph based on a query result. It does not calculate hashes for targets. func FromProtoNoHash(ctx context.Context, r *buildpb.QueryResult) (Result, error) { - return fromProto(ctx, r, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false) + return fromProto(ctx, r, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false, nil) } // for external targets, url and urls attributes could cause non-deterministic hash values, @@ -412,7 +418,7 @@ func bzlmodRepoName(targetName string) string { // (e.g. "rules_python++pip+third_party_python_base_311_torch_...._a6ebbe51"), // so hashing the name itself produces a stable, content-aware representative // hash that changes when the repo content changes. -func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) { +func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) { repoHashes := make(map[string][]byte) for name, target := range targets { @@ -429,18 +435,21 @@ func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set if target.Hash != nil { continue } - // Let excluded targets fall through to HashRecursively where they - // get the standard empty-hash treatment, keeping the same semantics - // as legacy WORKSPACE exclusion. if isExcluded(name, excludedRegex) { continue } h, ok := repoHashes[repo] if !ok { - rh := newHash() - rh.Write([]byte(repo)) - h = rh.Sum(nil) + if markerHash, hasMarker := repoMarkerHashes[repo]; hasMarker && len(markerHash) > 0 { + rh := newHash() + rh.Write(markerHash) + h = rh.Sum(nil) + } else { + // No marker file for this repo — skip collapsing and + // let HashRecursively hash the actual file content. + continue + } repoHashes[repo] = h } @@ -482,7 +491,7 @@ func GetTopologicalRootsAndIdentifyBuildableRoots(targets map[string]*Target) [] return roots } -func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, workspaceroot string, fullHashRepos set.Set[string], sequentialHashTargets set.Set[string], excludedRegex []*regexp.Regexp, useBzlmod bool) (Result, error) { +func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, workspaceroot string, fullHashRepos set.Set[string], sequentialHashTargets set.Set[string], excludedRegex []*regexp.Regexp, useBzlmod bool, repoMarkerHashes map[string][]byte) (Result, error) { warns := make(map[string]error) // Build target graph with dependencies, but without hash and root information. targets, err := GetInternalTargetsWithoutHashAndRootInfo(ctx, r) @@ -503,7 +512,7 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, // millions of individual pip-wheel files during the DFS — the same // optimization that legacy WORKSPACE gets via //external:repo // collapsing. - collapseBzlmodExternalTargets(targets, fullHashRepos, excludedRegex) + collapseBzlmodExternalTargets(targets, fullHashRepos, excludedRegex, repoMarkerHashes) } // get topological roots and update buildable roots info diff --git a/core/targethasher/graph_test.go b/core/targethasher/graph_test.go index abba2b6a..b5c9c27a 100644 --- a/core/targethasher/graph_test.go +++ b/core/targethasher/graph_test.go @@ -79,7 +79,7 @@ func TestContextCancellation(t *testing.T) { qr := &buildpb.QueryResult{ Target: []*buildpb.Target{&buildpb.Target{}}, } - result, err := fromProto(ctx, qr, nil, "", set.NewSet[string](), set.NewSet[string](), nil, false) + result, err := fromProto(ctx, qr, nil, "", set.NewSet[string](), set.NewSet[string](), nil, false, nil) assert.Equal(t, EmptyResult(), result) assert.ErrorIs(t, err, context.Canceled) @@ -107,7 +107,7 @@ func TestFromProtoSimpleRule(t *testing.T) { }, } - result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false) + result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false, nil) require.NoError(t, err) assert.Len(t, result.Targets, 1) @@ -138,7 +138,7 @@ func TestFromProtoWithDependencies(t *testing.T) { }, } - result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false) + result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false, nil) require.NoError(t, err) assert.Len(t, result.Targets, 2) @@ -176,7 +176,7 @@ func TestFromProtoWithExcludedRegex(t *testing.T) { // Exclude targets matching "//vendor:.*" excludedRegex := []*regexp.Regexp{regexp.MustCompile("//vendor:.*")} - result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), excludedRegex, false) + result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), excludedRegex, false, nil) require.NoError(t, err) assert.Len(t, result.Targets, 2) @@ -291,7 +291,7 @@ func TestFromProtoWithGeneratedFile(t *testing.T) { }, } - result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false) + result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), nil, false, nil) require.NoError(t, err) assert.Len(t, result.Targets, 2) @@ -403,7 +403,15 @@ func TestBzlmodRepoName(t *testing.T) { } func TestCollapseBzlmodExternalTargets(t *testing.T) { - t.Run("collapses source and generated files", func(t *testing.T) { + markerA := []byte{0xaa, 0xbb, 0xcc} + markerB := []byte{0xdd, 0xee, 0xff} + markers := map[string][]byte{ + "repo_a": markerA, + "repo_b": markerB, + "rules_python++pip+foo": []byte{0x11, 0x22}, + } + + t.Run("collapses with marker hash", func(t *testing.T) { targets := map[string]*Target{ "@@repo_a//pkg:file1.py": {Name: "@@repo_a//pkg:file1.py", RuleType: SourceFileType, External: true}, "@@repo_a//pkg:file2.py": {Name: "@@repo_a//pkg:file2.py", RuleType: SourceFileType, External: true}, @@ -411,23 +419,38 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "//src:main": {Name: "//src:main", RuleType: "go_binary"}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) - // All three external files should share the same repo-derived hash. assert.NotNil(t, targets["@@repo_a//pkg:file1.py"].Hash) assert.Equal(t, targets["@@repo_a//pkg:file1.py"].Hash, targets["@@repo_a//pkg:file2.py"].Hash) assert.Equal(t, targets["@@repo_a//pkg:file1.py"].Hash, targets["@@repo_a//pkg:gen.go"].Hash) - - // Internal target should be untouched. assert.Nil(t, targets["//src:main"].Hash) }) + t.Run("skips repos without marker (falls through to HashRecursively)", func(t *testing.T) { + targets := map[string]*Target{ + "@@no_marker_repo//pkg:file.py": {Name: "@@no_marker_repo//pkg:file.py", RuleType: SourceFileType, External: true}, + } + + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) + assert.Nil(t, targets["@@no_marker_repo//pkg:file.py"].Hash) + }) + + t.Run("skips repos when no markers provided at all", func(t *testing.T) { + targets := map[string]*Target{ + "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, + } + + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, nil) + assert.Nil(t, targets["@@repo_a//pkg:file.py"].Hash) + }) + t.Run("skips rule targets", func(t *testing.T) { targets := map[string]*Target{ "@@repo_a//pkg:lib": {Name: "@@repo_a//pkg:lib", RuleType: "go_library", External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) assert.Nil(t, targets["@@repo_a//pkg:lib"].Hash) }) @@ -436,7 +459,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet("", "repo_a"), nil) + collapseBzlmodExternalTargets(targets, set.NewSet("", "repo_a"), nil, markers) assert.Nil(t, targets["@@repo_a//pkg:file.py"].Hash) }) @@ -447,11 +470,9 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { } excluded := []*regexp.Regexp{regexp.MustCompile(`\.whl$`)} - collapseBzlmodExternalTargets(targets, set.NewSet(""), excluded) + collapseBzlmodExternalTargets(targets, set.NewSet(""), excluded, markers) - // .whl file should NOT be collapsed — left for HashRecursively to exclude. assert.Nil(t, targets["@@rules_python++pip+foo//pkg:file.whl"].Hash) - // .py file should be collapsed normally. assert.NotNil(t, targets["@@rules_python++pip+foo//pkg:module.py"].Hash) }) @@ -461,7 +482,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_b//pkg:file.py": {Name: "@@repo_b//pkg:file.py", RuleType: SourceFileType, External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) assert.NotEqual(t, targets["@@repo_a//pkg:file.py"].Hash, targets["@@repo_b//pkg:file.py"].Hash) }) @@ -471,7 +492,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true, Hash: existing}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil) + collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) assert.Equal(t, existing, targets["@@repo_a//pkg:file.py"].Hash) }) } @@ -501,7 +522,7 @@ func TestFromProtoExcludedBzlmodTargetGetsEmptyHash(t *testing.T) { } excluded := []*regexp.Regexp{regexp.MustCompile(`\.whl$`)} - result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), excluded, true) + result, err := fromProto(context.Background(), qr, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string](), excluded, true, nil) require.NoError(t, err) // Excluded .whl target should have empty hash (not a repo-name hash). @@ -528,7 +549,7 @@ func Test_fromProto(t *testing.T) { q, err := bazel.FromFile("testdata/test.proto.bin") require.NoError(t, err) - a, err := fromProto(ctx, q, mockHasher, "", set.NewSet[string](), set.NewSet[string](), nil, true) + a, err := fromProto(ctx, q, mockHasher, "", set.NewSet[string](), set.NewSet[string](), nil, true, nil) require.NoError(t, err) assert.Empty(t, a.Warnings) diff --git a/graphrunner/markers.go b/graphrunner/markers.go new file mode 100644 index 00000000..e8644dda --- /dev/null +++ b/graphrunner/markers.go @@ -0,0 +1,100 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package graphrunner + +import ( + "bufio" + "bytes" + "context" + "encoding/hex" + "os" + "path/filepath" + "strings" + + "github.com/uber/tango/core/execcmd" +) + +// readRepoMarkerHashes reads Bazel's external repo marker files and returns +// a map from canonical repo name to the marker's content hash. Each marker +// file's first line is a hex-encoded hash of the repository rule's inputs +// (URL, sha256, patches, etc.) that changes whenever the repo is upgraded. +func readRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand string) (map[string][]byte, error) { + outputBase, err := bazelOutputBase(ctx, workspacePath, bazelCommand) + if err != nil { + return nil, err + } + + markerDir := filepath.Join(outputBase, "external") + entries, err := os.ReadDir(markerDir) + if err != nil { + return nil, err + } + + hashes := make(map[string][]byte, len(entries)) + for _, e := range entries { + name := e.Name() + if !strings.HasSuffix(name, ".marker") { + continue + } + // Marker files are named @repo_name.marker — strip prefix and suffix. + repo := strings.TrimPrefix(strings.TrimSuffix(name, ".marker"), "@") + if repo == "" { + continue + } + + h, err := readMarkerFirstLine(filepath.Join(markerDir, name)) + if err != nil { + continue + } + hashes[repo] = h + } + + return hashes, nil +} + +// readMarkerFirstLine reads the first line of a marker file and decodes +// the hex hash. Returns the raw bytes, or an error if the file can't be +// read or the first line isn't valid hex. +func readMarkerFirstLine(path string) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() + + scanner := bufio.NewScanner(f) + if !scanner.Scan() { + return nil, scanner.Err() + } + line := strings.TrimSpace(scanner.Text()) + if line == "" { + return nil, nil + } + return hex.DecodeString(line) +} + +// bazelOutputBase runs `bazel info output_base` and returns the path. +func bazelOutputBase(ctx context.Context, workspacePath, bazelCommand string) (string, error) { + if bazelCommand == "" { + bazelCommand = "bazel" + } + cmd := execcmd.CommandContext(ctx, bazelCommand, "info", "output_base") + cmd.Dir = workspacePath + out, err := cmd.Output() + if err != nil { + return "", err + } + return string(bytes.TrimSpace(out)), nil +} diff --git a/graphrunner/native.go b/graphrunner/native.go index bb2cde9b..4b2234ef 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -96,12 +96,27 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) // mutate the shared configuration's backing array. excludedRegex := append([]string(nil), g.config.ExcludedFiles...) excludedRegex = append(excludedRegex, g.extraExcludedFiles...) + + // Read marker files for bzlmod repos so the collapse uses content-aware + // hashes instead of just the repo name string. + var repoMarkerHashes map[string][]byte + if bzlmodEnabled { + markerStart := time.Now() + repoMarkerHashes, err = readRepoMarkerHashes(ctx, ws.Path(), g.config.BazelCommandPath) + g.emitter.DurationHistogram(_opCompute, "marker_read_duration", metrics.FastDurationBuckets).RecordDuration(time.Since(markerStart)) + if err != nil { + // Non-fatal: fall back to repo-name hashing if markers can't be read. + repoMarkerHashes = nil + } + } + hashConfig := targethasher.HashConfig{ KnownSourceHashes: knownSourceHashes, FullHashRepos: g.config.FullHashRepos, ExcludedRegex: excludedRegex, UseBzlmod: bzlmodEnabled, AllTargetsFiles: g.config.AllTargetsFiles, + RepoMarkerHashes: repoMarkerHashes, } hashStart := time.Now() From fedc0e2842c5b02ad4af1f6ecc7fa8459e86be68 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 16:14:19 -0700 Subject: [PATCH 05/23] Return error when marker files can't be read instead of silent fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If marker file reading fails entirely (output_base unreachable, external dir missing), that's a real problem — silently falling back to no collapsing would cause a 16-minute run with no indication why. Co-Authored-By: Claude Sonnet 5 --- graphrunner/native.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/graphrunner/native.go b/graphrunner/native.go index 4b2234ef..dd2746e9 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -16,6 +16,7 @@ package graphrunner import ( "context" + "fmt" "time" "github.com/uber-go/tally" @@ -105,8 +106,7 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) repoMarkerHashes, err = readRepoMarkerHashes(ctx, ws.Path(), g.config.BazelCommandPath) g.emitter.DurationHistogram(_opCompute, "marker_read_duration", metrics.FastDurationBuckets).RecordDuration(time.Since(markerStart)) if err != nil { - // Non-fatal: fall back to repo-name hashing if markers can't be read. - repoMarkerHashes = nil + return targethasher.EmptyResult(), fmt.Errorf("read repo marker hashes: %w", err) } } From b76503795a420826ba865668f11929cbd5127110 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 16:29:20 -0700 Subject: [PATCH 06/23] Address Uber Go style guide issues - Propagate errors from marker file reads instead of silently skipping - Wrap errors with context (fmt.Errorf %w) per guide's error handling - Use defer func() { _ = f.Close() }() to signal intent on read-only fd - Handle scanner.Err() nil case explicitly for empty marker files - Early return from collapseBzlmodExternalTargets when no markers provided - Remove oversized capacity hint on hashes map Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 5 ++++- graphrunner/markers.go | 25 +++++++++++++++---------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 7efdf5d0..0e035a15 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -419,8 +419,11 @@ func bzlmodRepoName(targetName string) string { // so hashing the name itself produces a stable, content-aware representative // hash that changes when the repo content changes. func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) { - repoHashes := make(map[string][]byte) + if len(repoMarkerHashes) == 0 { + return + } + repoHashes := make(map[string][]byte) for name, target := range targets { repo := bzlmodRepoName(name) if repo == "" { diff --git a/graphrunner/markers.go b/graphrunner/markers.go index e8644dda..49908faa 100644 --- a/graphrunner/markers.go +++ b/graphrunner/markers.go @@ -19,6 +19,7 @@ import ( "bytes" "context" "encoding/hex" + "fmt" "os" "path/filepath" "strings" @@ -33,22 +34,21 @@ import ( func readRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand string) (map[string][]byte, error) { outputBase, err := bazelOutputBase(ctx, workspacePath, bazelCommand) if err != nil { - return nil, err + return nil, fmt.Errorf("bazel output base: %w", err) } markerDir := filepath.Join(outputBase, "external") entries, err := os.ReadDir(markerDir) if err != nil { - return nil, err + return nil, fmt.Errorf("read marker dir %s: %w", markerDir, err) } - hashes := make(map[string][]byte, len(entries)) + hashes := make(map[string][]byte) for _, e := range entries { name := e.Name() if !strings.HasSuffix(name, ".marker") { continue } - // Marker files are named @repo_name.marker — strip prefix and suffix. repo := strings.TrimPrefix(strings.TrimSuffix(name, ".marker"), "@") if repo == "" { continue @@ -56,6 +56,9 @@ func readRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand strin h, err := readMarkerFirstLine(filepath.Join(markerDir, name)) if err != nil { + return nil, fmt.Errorf("read marker for repo %s: %w", repo, err) + } + if len(h) == 0 { continue } hashes[repo] = h @@ -64,19 +67,21 @@ func readRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand strin return hashes, nil } -// readMarkerFirstLine reads the first line of a marker file and decodes -// the hex hash. Returns the raw bytes, or an error if the file can't be -// read or the first line isn't valid hex. +// readMarkerFirstLine reads the first line of a marker file and hex-decodes +// it into raw bytes. Returns (nil, nil) for empty files. func readMarkerFirstLine(path string) ([]byte, error) { f, err := os.Open(path) if err != nil { return nil, err } - defer f.Close() + defer func() { _ = f.Close() }() scanner := bufio.NewScanner(f) if !scanner.Scan() { - return nil, scanner.Err() + if err := scanner.Err(); err != nil { + return nil, err + } + return nil, nil } line := strings.TrimSpace(scanner.Text()) if line == "" { @@ -94,7 +99,7 @@ func bazelOutputBase(ctx context.Context, workspacePath, bazelCommand string) (s cmd.Dir = workspacePath out, err := cmd.Output() if err != nil { - return "", err + return "", fmt.Errorf("run bazel info output_base: %w", err) } return string(bytes.TrimSpace(out)), nil } From 4b2693389fa784e1eeeb1a96820191b81c69c577 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 16:42:32 -0700 Subject: [PATCH 07/23] Pass OutputBase via NativeGraphRunnerParams instead of Bazel interface The output base is a runtime value resolved by the CLI, not a concern of the targethasher or the Bazel query interface. Pass it through NativeGraphRunnerParams so the graph runner reads marker files directly from the filesystem without coupling to the Bazel client. Co-Authored-By: Claude Sonnet 5 --- graphrunner/markers.go | 25 +------------------------ graphrunner/native.go | 12 +++++++++--- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/graphrunner/markers.go b/graphrunner/markers.go index 49908faa..c1db0167 100644 --- a/graphrunner/markers.go +++ b/graphrunner/markers.go @@ -16,27 +16,18 @@ package graphrunner import ( "bufio" - "bytes" - "context" "encoding/hex" "fmt" "os" "path/filepath" "strings" - - "github.com/uber/tango/core/execcmd" ) // readRepoMarkerHashes reads Bazel's external repo marker files and returns // a map from canonical repo name to the marker's content hash. Each marker // file's first line is a hex-encoded hash of the repository rule's inputs // (URL, sha256, patches, etc.) that changes whenever the repo is upgraded. -func readRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand string) (map[string][]byte, error) { - outputBase, err := bazelOutputBase(ctx, workspacePath, bazelCommand) - if err != nil { - return nil, fmt.Errorf("bazel output base: %w", err) - } - +func readRepoMarkerHashes(outputBase string) (map[string][]byte, error) { markerDir := filepath.Join(outputBase, "external") entries, err := os.ReadDir(markerDir) if err != nil { @@ -89,17 +80,3 @@ func readMarkerFirstLine(path string) ([]byte, error) { } return hex.DecodeString(line) } - -// bazelOutputBase runs `bazel info output_base` and returns the path. -func bazelOutputBase(ctx context.Context, workspacePath, bazelCommand string) (string, error) { - if bazelCommand == "" { - bazelCommand = "bazel" - } - cmd := execcmd.CommandContext(ctx, bazelCommand, "info", "output_base") - cmd.Dir = workspacePath - out, err := cmd.Output() - if err != nil { - return "", fmt.Errorf("run bazel info output_base: %w", err) - } - return string(bytes.TrimSpace(out)), nil -} diff --git a/graphrunner/native.go b/graphrunner/native.go index dd2746e9..d539612e 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -33,6 +33,7 @@ type nativeGraphRunner struct { git git.Interface config config.RepositoryConfig extraExcludedFiles []string + outputBase string emitter *metrics.Emitter } @@ -41,7 +42,11 @@ type NativeGraphRunnerParams struct { GitClient git.Interface Config config.RepositoryConfig ExtraExcludedFiles []string - Scope tally.Scope + // OutputBase is the Bazel output base directory, used to read marker + // files for bzlmod external repo collapsing. When empty, marker-based + // collapsing is skipped. + OutputBase string + Scope tally.Scope } // graph runner takes in a bazel query request and computes the graph @@ -51,6 +56,7 @@ func NewNativeGraphRunner(p NativeGraphRunnerParams) GraphRunner { git: p.GitClient, config: p.Config, extraExcludedFiles: p.ExtraExcludedFiles, + outputBase: p.OutputBase, emitter: metrics.New(p.Scope).SubScope("graph_runner"), } } @@ -101,9 +107,9 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) // Read marker files for bzlmod repos so the collapse uses content-aware // hashes instead of just the repo name string. var repoMarkerHashes map[string][]byte - if bzlmodEnabled { + if bzlmodEnabled && g.outputBase != "" { markerStart := time.Now() - repoMarkerHashes, err = readRepoMarkerHashes(ctx, ws.Path(), g.config.BazelCommandPath) + repoMarkerHashes, err = readRepoMarkerHashes(g.outputBase) g.emitter.DurationHistogram(_opCompute, "marker_read_duration", metrics.FastDurationBuckets).RecordDuration(time.Since(markerStart)) if err != nil { return targethasher.EmptyResult(), fmt.Errorf("read repo marker hashes: %w", err) From c761c91d55bc6e0338323ff96e90bea5e6f8349d Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 16:45:42 -0700 Subject: [PATCH 08/23] Require OutputBase for bzlmod repos instead of silently skipping Without this, a missing OutputBase silently disables collapsing and the run takes 16 minutes with no indication why. Co-Authored-By: Claude Sonnet 5 --- graphrunner/native.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/graphrunner/native.go b/graphrunner/native.go index d539612e..ab08142f 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -107,7 +107,10 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) // Read marker files for bzlmod repos so the collapse uses content-aware // hashes instead of just the repo name string. var repoMarkerHashes map[string][]byte - if bzlmodEnabled && g.outputBase != "" { + if bzlmodEnabled { + if g.outputBase == "" { + return targethasher.EmptyResult(), fmt.Errorf("output_base is required for bzlmod repos") + } markerStart := time.Now() repoMarkerHashes, err = readRepoMarkerHashes(g.outputBase) g.emitter.DurationHistogram(_opCompute, "marker_read_duration", metrics.FastDurationBuckets).RecordDuration(time.Since(markerStart)) From 6cf661dc4ba2d3f7a9f420a792d2e5d8d5da7b8b Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 16:49:35 -0700 Subject: [PATCH 09/23] Simplify marker reading: hash the whole file instead of parsing hex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No need to parse the marker file format. Just sha1(file_content) — the content is deterministic for a given repo rule invocation, so the hash is stable as long as the dependency doesn't change. Co-Authored-By: Claude Sonnet 5 --- graphrunner/markers.go | 39 ++++++++------------------------------- 1 file changed, 8 insertions(+), 31 deletions(-) diff --git a/graphrunner/markers.go b/graphrunner/markers.go index c1db0167..5206ff51 100644 --- a/graphrunner/markers.go +++ b/graphrunner/markers.go @@ -15,8 +15,7 @@ package graphrunner import ( - "bufio" - "encoding/hex" + "crypto/sha1" "fmt" "os" "path/filepath" @@ -24,9 +23,9 @@ import ( ) // readRepoMarkerHashes reads Bazel's external repo marker files and returns -// a map from canonical repo name to the marker's content hash. Each marker -// file's first line is a hex-encoded hash of the repository rule's inputs -// (URL, sha256, patches, etc.) that changes whenever the repo is upgraded. +// a map from canonical repo name to a hash of the marker file's content. +// Each marker file captures the repository rule's inputs (URL, sha256, +// patches, etc.) and changes whenever the repo is upgraded. func readRepoMarkerHashes(outputBase string) (map[string][]byte, error) { markerDir := filepath.Join(outputBase, "external") entries, err := os.ReadDir(markerDir) @@ -45,38 +44,16 @@ func readRepoMarkerHashes(outputBase string) (map[string][]byte, error) { continue } - h, err := readMarkerFirstLine(filepath.Join(markerDir, name)) + content, err := os.ReadFile(filepath.Join(markerDir, name)) if err != nil { return nil, fmt.Errorf("read marker for repo %s: %w", repo, err) } - if len(h) == 0 { + if len(content) == 0 { continue } - hashes[repo] = h + h := sha1.Sum(content) + hashes[repo] = h[:] } return hashes, nil } - -// readMarkerFirstLine reads the first line of a marker file and hex-decodes -// it into raw bytes. Returns (nil, nil) for empty files. -func readMarkerFirstLine(path string) ([]byte, error) { - f, err := os.Open(path) - if err != nil { - return nil, err - } - defer func() { _ = f.Close() }() - - scanner := bufio.NewScanner(f) - if !scanner.Scan() { - if err := scanner.Err(); err != nil { - return nil, err - } - return nil, nil - } - line := strings.TrimSpace(scanner.Text()) - if line == "" { - return nil, nil - } - return hex.DecodeString(line) -} From 81c3dba03509172472132a133226ff07818372a3 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 10 Sep 2026 16:57:45 -0700 Subject: [PATCH 10/23] Hash only the first line of marker files The first line is the repo rule input hash, stable across runs for the same dependency version. The remaining lines include ENV variables that could differ between CI environments without a dependency change, causing false positive hash changes. Co-Authored-By: Claude Sonnet 5 --- graphrunner/markers.go | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/graphrunner/markers.go b/graphrunner/markers.go index 5206ff51..a2ba559f 100644 --- a/graphrunner/markers.go +++ b/graphrunner/markers.go @@ -15,7 +15,8 @@ package graphrunner import ( - "crypto/sha1" + "bufio" + "encoding/hex" "fmt" "os" "path/filepath" @@ -44,16 +45,39 @@ func readRepoMarkerHashes(outputBase string) (map[string][]byte, error) { continue } - content, err := os.ReadFile(filepath.Join(markerDir, name)) + h, err := readMarkerHash(filepath.Join(markerDir, name)) if err != nil { return nil, fmt.Errorf("read marker for repo %s: %w", repo, err) } - if len(content) == 0 { + if len(h) == 0 { continue } - h := sha1.Sum(content) - hashes[repo] = h[:] + hashes[repo] = h } return hashes, nil } + +// readMarkerHash reads the first line of a marker file and hex-decodes +// it into raw bytes. The first line is a hash of the repository rule's +// inputs and is stable across runs for the same dependency version. +func readMarkerHash(path string) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + + scanner := bufio.NewScanner(f) + if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return nil, err + } + return nil, nil + } + line := strings.TrimSpace(scanner.Text()) + if line == "" { + return nil, nil + } + return hex.DecodeString(line) +} From 4528d39a12d2e8b1ee040e1a5c49ac714f850a89 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 09:22:17 -0700 Subject: [PATCH 11/23] Move OutputBase to Bazel interface, resolve after query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The output base is now provided by the bazel client module via Bazel.OutputBase(ctx), using the same resolved bazel command and workspace configuration as ExecuteQuery. The graph runner calls it after the query completes and passes the path to readRepoMarkerHashes. Removes OutputBase from NativeGraphRunnerParams — callers no longer need to resolve it themselves. Co-Authored-By: Claude Sonnet 5 --- core/bazel/bazel.go | 23 +++++++++++++++++++++++ core/bazel/bazelmock/bazelmock.go | 15 +++++++++++++++ graphrunner/native.go | 15 +++++---------- graphrunner/native_test.go | 2 +- 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index 9180fa8e..148f493d 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -24,6 +24,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "time" buildpb "github.com/bazelbuild/buildtools/build_proto" @@ -53,6 +54,8 @@ type QueryResponse struct { type Bazel interface { ExecuteQuery(ctx context.Context, req *QueryRequest) (*QueryResponse, error) + // OutputBase returns the Bazel output base directory for this workspace. + OutputBase(ctx context.Context) (string, error) } // BazelClient is a client for interacting with Bazel. @@ -109,6 +112,26 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { }, nil } +// OutputBase runs `bazel info output_base` and returns the path. +func (b *BazelClient) OutputBase(ctx context.Context) (string, error) { + cmd := b.execCommandContext(ctx, b.bazelCommand, "info", "output_base") + stdout, err := cmd.StdoutPipe() + if err != nil { + return "", fmt.Errorf("stdout pipe: %w", err) + } + if err := cmd.Start(); err != nil { + return "", fmt.Errorf("start bazel info: %w", err) + } + out, err := io.ReadAll(stdout) + if err != nil { + return "", fmt.Errorf("read output: %w", err) + } + if err := cmd.Wait(); err != nil { + return "", fmt.Errorf("bazel info output_base: %w", err) + } + return strings.TrimSpace(string(out)), nil +} + // detectBazelExecutable returns the path to a bazelisk binary. // If bazelCommand is explicitly provided, it is used as-is. // Otherwise, bazelisk is downloaded from GitHub into a local cache directory. diff --git a/core/bazel/bazelmock/bazelmock.go b/core/bazel/bazelmock/bazelmock.go index 9882710d..18dd2aee 100644 --- a/core/bazel/bazelmock/bazelmock.go +++ b/core/bazel/bazelmock/bazelmock.go @@ -55,3 +55,18 @@ func (mr *MockBazelMockRecorder) ExecuteQuery(ctx, req any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExecuteQuery", reflect.TypeOf((*MockBazel)(nil).ExecuteQuery), ctx, req) } + +// OutputBase mocks base method. +func (m *MockBazel) OutputBase(ctx context.Context) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "OutputBase", ctx) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// OutputBase indicates an expected call of OutputBase. +func (mr *MockBazelMockRecorder) OutputBase(ctx any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OutputBase", reflect.TypeOf((*MockBazel)(nil).OutputBase), ctx) +} diff --git a/graphrunner/native.go b/graphrunner/native.go index ab08142f..e7892b6d 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -33,7 +33,6 @@ type nativeGraphRunner struct { git git.Interface config config.RepositoryConfig extraExcludedFiles []string - outputBase string emitter *metrics.Emitter } @@ -42,11 +41,7 @@ type NativeGraphRunnerParams struct { GitClient git.Interface Config config.RepositoryConfig ExtraExcludedFiles []string - // OutputBase is the Bazel output base directory, used to read marker - // files for bzlmod external repo collapsing. When empty, marker-based - // collapsing is skipped. - OutputBase string - Scope tally.Scope + Scope tally.Scope } // graph runner takes in a bazel query request and computes the graph @@ -56,7 +51,6 @@ func NewNativeGraphRunner(p NativeGraphRunnerParams) GraphRunner { git: p.GitClient, config: p.Config, extraExcludedFiles: p.ExtraExcludedFiles, - outputBase: p.OutputBase, emitter: metrics.New(p.Scope).SubScope("graph_runner"), } } @@ -108,11 +102,12 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) // hashes instead of just the repo name string. var repoMarkerHashes map[string][]byte if bzlmodEnabled { - if g.outputBase == "" { - return targethasher.EmptyResult(), fmt.Errorf("output_base is required for bzlmod repos") + outputBase, outputBaseErr := g.bazel.OutputBase(ctx) + if outputBaseErr != nil { + return targethasher.EmptyResult(), fmt.Errorf("bazel output base: %w", outputBaseErr) } markerStart := time.Now() - repoMarkerHashes, err = readRepoMarkerHashes(g.outputBase) + repoMarkerHashes, err = readRepoMarkerHashes(outputBase) g.emitter.DurationHistogram(_opCompute, "marker_read_duration", metrics.FastDurationBuckets).RecordDuration(time.Since(markerStart)) if err != nil { return targethasher.EmptyResult(), fmt.Errorf("read repo marker hashes: %w", err) diff --git a/graphrunner/native_test.go b/graphrunner/native_test.go index 363c19c1..a5761126 100644 --- a/graphrunner/native_test.go +++ b/graphrunner/native_test.go @@ -44,10 +44,10 @@ func TestCompute_CallsBazelAndReturnsResult(t *testing.T) { }, }, }}}, nil) + bazelMock.EXPECT().OutputBase(gomock.Any()).Return(t.TempDir(), nil) gr := NewNativeGraphRunner(NativeGraphRunnerParams{ BazelClient: bazelMock, GitClient: gitMock, - // leave HashConfig zero; not asserted here }) ws := workspace.NewWorkspace(workspace.WorkspaceParams{ Path: "/tmp/ws", From d1c7440fa16b83b159fe236af18b5b5481eca4d9 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 09:27:28 -0700 Subject: [PATCH 12/23] Extract shouldCollapse helper, update copyright year Simplify the guard clauses in collapseBzlmodExternalTargets into a single shouldCollapse predicate. Update copyright to 2026 for new file. Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 40 +++++++++++++++++--------------------- graphrunner/markers.go | 2 +- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 0e035a15..38ba654f 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -418,41 +418,37 @@ func bzlmodRepoName(targetName string) string { // (e.g. "rules_python++pip+third_party_python_base_311_torch_...._a6ebbe51"), // so hashing the name itself produces a stable, content-aware representative // hash that changes when the repo content changes. +func shouldCollapse(target *Target, repo string, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) bool { + if repo == "" || fullHashRepos.Contains(repo) { + return false + } + if target.RuleType != SourceFileType && target.RuleType != GeneratedFileType { + return false + } + return target.Hash == nil && !isExcluded(target.Name, excludedRegex) +} + func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) { if len(repoMarkerHashes) == 0 { return } repoHashes := make(map[string][]byte) - for name, target := range targets { - repo := bzlmodRepoName(name) - if repo == "" { - continue - } - if fullHashRepos.Contains(repo) { - continue - } - if target.RuleType != SourceFileType && target.RuleType != GeneratedFileType { - continue - } - if target.Hash != nil { - continue - } - if isExcluded(name, excludedRegex) { + for _, target := range targets { + repo := bzlmodRepoName(target.Name) + if !shouldCollapse(target, repo, fullHashRepos, excludedRegex) { continue } h, ok := repoHashes[repo] if !ok { - if markerHash, hasMarker := repoMarkerHashes[repo]; hasMarker && len(markerHash) > 0 { - rh := newHash() - rh.Write(markerHash) - h = rh.Sum(nil) - } else { - // No marker file for this repo — skip collapsing and - // let HashRecursively hash the actual file content. + markerHash, hasMarker := repoMarkerHashes[repo] + if !hasMarker || len(markerHash) == 0 { continue } + rh := newHash() + rh.Write(markerHash) + h = rh.Sum(nil) repoHashes[repo] = h } diff --git a/graphrunner/markers.go b/graphrunner/markers.go index a2ba559f..77487355 100644 --- a/graphrunner/markers.go +++ b/graphrunner/markers.go @@ -1,4 +1,4 @@ -// Copyright (c) 2025 Uber Technologies, Inc. +// Copyright (c) 2026 Uber Technologies, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. From 5ac76f3d9f2e5e479c6a68480edd9f289ab14ed8 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 09:32:00 -0700 Subject: [PATCH 13/23] Fix documentation to match current implementation - shouldCollapse: describe marker-based pre-hashing, not repo name hashing - collapseBzlmodExternalTargets: document marker file usage and fallback - RepoMarkerHashes: clarify these are repo rule input hashes, not content hashes - readRepoMarkerHashes: clarify it reads the first line, not the whole file - fromProto call site: update inline comment to reflect marker-based collapse - Separate shouldCollapse return into explicit if statements Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 50 +++++++++++++++++++++----------------- graphrunner/markers.go | 6 ++--- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 38ba654f..b258e8ca 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -78,11 +78,12 @@ type HashConfig struct { // AllTargetsFiles lists repo-relative paths whose hashes should be // extracted from KnownSourceHashes into Result.AllTargetsFileHashes. AllTargetsFiles []string - // RepoMarkerHashes maps canonical bzlmod repo names to content hashes - // read from Bazel's marker files ($(output_base)/external/@repo.marker). - // When set, collapseBzlmodExternalTargets uses these instead of hashing - // the repo name string, so that repos without a content hash suffix in - // their name (e.g. "protobuf+") still get a hash that changes on upgrade. + // RepoMarkerHashes maps canonical bzlmod repo names to repo rule input + // hashes read from Bazel's marker files ($(output_base)/external/@repo.marker). + // Used by collapseBzlmodExternalTargets to pre-hash external source files + // without reading their content from disk. The marker hash changes on any + // dependency upgrade, so repos whose canonical name stays the same across + // versions (e.g. "protobuf+") are still correctly detected as changed. RepoMarkerHashes map[string][]byte } @@ -408,16 +409,10 @@ func bzlmodRepoName(targetName string) string { return rest[:idx] } -// collapseBzlmodExternalTargets pre-hashes all bzlmod external source file -// and generated file targets using a single hash derived from the canonical -// repo name. This mirrors the legacy WORKSPACE //external:repo collapsing -// but for bzlmod's @@repo//... naming. Rule targets are left alone so their -// real dependency edges are preserved. -// -// The canonical bzlmod repo name encodes the module version and content hash -// (e.g. "rules_python++pip+third_party_python_base_311_torch_...._a6ebbe51"), -// so hashing the name itself produces a stable, content-aware representative -// hash that changes when the repo content changes. +// shouldCollapse reports whether a bzlmod external target should be +// pre-hashed using the repo's marker file hash instead of hashing its +// file content during the DFS. Only source and generated files are +// collapsed; rule targets are left alone so dependency edges are preserved. func shouldCollapse(target *Target, repo string, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) bool { if repo == "" || fullHashRepos.Contains(repo) { return false @@ -425,9 +420,22 @@ func shouldCollapse(target *Target, repo string, fullHashRepos set.Set[string], if target.RuleType != SourceFileType && target.RuleType != GeneratedFileType { return false } - return target.Hash == nil && !isExcluded(target.Name, excludedRegex) + if target.Hash != nil { + return false + } + if isExcluded(target.Name, excludedRegex) { + return false + } + return true } +// collapseBzlmodExternalTargets pre-hashes bzlmod external source and +// generated file targets using hashes from Bazel's marker files. This is +// the bzlmod equivalent of legacy WORKSPACE //external:repo collapsing. +// Marker file hashes change on any dependency upgrade, so the collapsed +// hash is content-aware even for repos whose canonical name stays the same +// across versions (e.g. "protobuf+"). Repos without a marker are skipped +// and fall through to HashRecursively for per-file content hashing. func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) { if len(repoMarkerHashes) == 0 { return @@ -505,12 +513,10 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, return EmptyResult(), err } } else { - // Bzlmod: collapse external source/generated file targets to a - // single per-repo hash derived from the canonical repo name (which - // encodes the version and content hash). This avoids visiting - // millions of individual pip-wheel files during the DFS — the same - // optimization that legacy WORKSPACE gets via //external:repo - // collapsing. + // Bzlmod: collapse external source/generated file targets using + // Bazel marker file hashes. Avoids visiting millions of individual + // pip-wheel files during the DFS — the bzlmod equivalent of legacy + // WORKSPACE //external:repo collapsing. collapseBzlmodExternalTargets(targets, fullHashRepos, excludedRegex, repoMarkerHashes) } diff --git a/graphrunner/markers.go b/graphrunner/markers.go index 77487355..31b96abd 100644 --- a/graphrunner/markers.go +++ b/graphrunner/markers.go @@ -24,9 +24,9 @@ import ( ) // readRepoMarkerHashes reads Bazel's external repo marker files and returns -// a map from canonical repo name to a hash of the marker file's content. -// Each marker file captures the repository rule's inputs (URL, sha256, -// patches, etc.) and changes whenever the repo is upgraded. +// a map from canonical repo name to the repo rule input hash (the first +// line of each marker file). This hash changes whenever the repo is +// upgraded (different URL, sha256, patches, etc.). func readRepoMarkerHashes(outputBase string) (map[string][]byte, error) { markerDir := filepath.Join(outputBase, "external") entries, err := os.ReadDir(markerDir) From 439fc7b1c5229f277912d67e331846a828ef120a Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 09:47:49 -0700 Subject: [PATCH 14/23] Guard against nil targets, empty output_base, and invalid inputs - shouldCollapse: nil-check target pointer before accessing fields - OutputBase: return error if bazel info returns empty path - readRepoMarkerHashes: validate outputBase is non-empty Co-Authored-By: Claude Sonnet 5 --- core/bazel/bazel.go | 6 +++++- core/targethasher/graph.go | 3 +++ graphrunner/markers.go | 3 +++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index 148f493d..69b1d246 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -129,7 +129,11 @@ func (b *BazelClient) OutputBase(ctx context.Context) (string, error) { if err := cmd.Wait(); err != nil { return "", fmt.Errorf("bazel info output_base: %w", err) } - return strings.TrimSpace(string(out)), nil + result := strings.TrimSpace(string(out)) + if result == "" { + return "", fmt.Errorf("bazel info output_base returned empty path") + } + return result, nil } // detectBazelExecutable returns the path to a bazelisk binary. diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index b258e8ca..a75749eb 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -414,6 +414,9 @@ func bzlmodRepoName(targetName string) string { // file content during the DFS. Only source and generated files are // collapsed; rule targets are left alone so dependency edges are preserved. func shouldCollapse(target *Target, repo string, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) bool { + if target == nil { + return false + } if repo == "" || fullHashRepos.Contains(repo) { return false } diff --git a/graphrunner/markers.go b/graphrunner/markers.go index 31b96abd..2cc8b691 100644 --- a/graphrunner/markers.go +++ b/graphrunner/markers.go @@ -28,6 +28,9 @@ import ( // line of each marker file). This hash changes whenever the repo is // upgraded (different URL, sha256, patches, etc.). func readRepoMarkerHashes(outputBase string) (map[string][]byte, error) { + if outputBase == "" { + return nil, fmt.Errorf("output_base is empty") + } markerDir := filepath.Join(outputBase, "external") entries, err := os.ReadDir(markerDir) if err != nil { From 908d9697bb91e504695477dad8ad806c18300410 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 10:06:25 -0700 Subject: [PATCH 15/23] Move markers.go to core/targethasher, revert Bazel interface change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marker file reading belongs with the hashing logic in core/targethasher. The graph runner calls targethasher.ReadRepoMarkerHashes which resolves the bazel command via bazel.DetectBazelExecutable (now exported) and runs bazel info output_base itself — no changes to the Bazel interface. Co-Authored-By: Claude Sonnet 5 --- core/bazel/bazel.go | 33 ++------------ core/bazel/bazelmock/bazelmock.go | 14 ------ {graphrunner => core/targethasher}/markers.go | 43 +++++++++++++++---- graphrunner/native.go | 6 +-- graphrunner/native_test.go | 1 - 5 files changed, 39 insertions(+), 58 deletions(-) rename {graphrunner => core/targethasher}/markers.go (59%) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index 69b1d246..b230d742 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -24,7 +24,6 @@ import ( "os" "path/filepath" "runtime" - "strings" "time" buildpb "github.com/bazelbuild/buildtools/build_proto" @@ -54,8 +53,6 @@ type QueryResponse struct { type Bazel interface { ExecuteQuery(ctx context.Context, req *QueryRequest) (*QueryResponse, error) - // OutputBase returns the Bazel output base directory for this workspace. - OutputBase(ctx context.Context) (string, error) } // BazelClient is a client for interacting with Bazel. @@ -96,7 +93,7 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { if timeout == 0 { timeout = _queryTimeout } - bazelCommand, err := detectBazelExecutable(ctx, p.BazelCommand) + bazelCommand, err := DetectBazelExecutable(ctx, p.BazelCommand) if err != nil { return nil, fmt.Errorf("detect bazel executable: %w", err) } @@ -112,34 +109,10 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { }, nil } -// OutputBase runs `bazel info output_base` and returns the path. -func (b *BazelClient) OutputBase(ctx context.Context) (string, error) { - cmd := b.execCommandContext(ctx, b.bazelCommand, "info", "output_base") - stdout, err := cmd.StdoutPipe() - if err != nil { - return "", fmt.Errorf("stdout pipe: %w", err) - } - if err := cmd.Start(); err != nil { - return "", fmt.Errorf("start bazel info: %w", err) - } - out, err := io.ReadAll(stdout) - if err != nil { - return "", fmt.Errorf("read output: %w", err) - } - if err := cmd.Wait(); err != nil { - return "", fmt.Errorf("bazel info output_base: %w", err) - } - result := strings.TrimSpace(string(out)) - if result == "" { - return "", fmt.Errorf("bazel info output_base returned empty path") - } - return result, nil -} - -// detectBazelExecutable returns the path to a bazelisk binary. +// DetectBazelExecutable returns the path to a bazel binary. // If bazelCommand is explicitly provided, it is used as-is. // Otherwise, bazelisk is downloaded from GitHub into a local cache directory. -func detectBazelExecutable(ctx context.Context, bazelCommand string) (string, error) { +func DetectBazelExecutable(ctx context.Context, bazelCommand string) (string, error) { if bazelCommand != "" { return bazelCommand, nil } diff --git a/core/bazel/bazelmock/bazelmock.go b/core/bazel/bazelmock/bazelmock.go index 18dd2aee..a1ef1554 100644 --- a/core/bazel/bazelmock/bazelmock.go +++ b/core/bazel/bazelmock/bazelmock.go @@ -56,17 +56,3 @@ func (mr *MockBazelMockRecorder) ExecuteQuery(ctx, req any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExecuteQuery", reflect.TypeOf((*MockBazel)(nil).ExecuteQuery), ctx, req) } -// OutputBase mocks base method. -func (m *MockBazel) OutputBase(ctx context.Context) (string, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "OutputBase", ctx) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// OutputBase indicates an expected call of OutputBase. -func (mr *MockBazelMockRecorder) OutputBase(ctx any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OutputBase", reflect.TypeOf((*MockBazel)(nil).OutputBase), ctx) -} diff --git a/graphrunner/markers.go b/core/targethasher/markers.go similarity index 59% rename from graphrunner/markers.go rename to core/targethasher/markers.go index 2cc8b691..131206c3 100644 --- a/graphrunner/markers.go +++ b/core/targethasher/markers.go @@ -12,25 +12,37 @@ // See the License for the specific language governing permissions and // limitations under the License. -package graphrunner +package targethasher import ( "bufio" + "context" "encoding/hex" "fmt" "os" "path/filepath" "strings" + + "github.com/uber/tango/core/bazel" + "github.com/uber/tango/core/execcmd" ) -// readRepoMarkerHashes reads Bazel's external repo marker files and returns -// a map from canonical repo name to the repo rule input hash (the first -// line of each marker file). This hash changes whenever the repo is -// upgraded (different URL, sha256, patches, etc.). -func readRepoMarkerHashes(outputBase string) (map[string][]byte, error) { - if outputBase == "" { - return nil, fmt.Errorf("output_base is empty") +// ReadRepoMarkerHashes resolves the Bazel output base, then reads the +// external repo marker files and returns a map from canonical repo name +// to the repo rule input hash (the first line of each marker file). +// This hash changes whenever the repo is upgraded (different URL, +// sha256, patches, etc.). +func ReadRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand string) (map[string][]byte, error) { + resolvedCommand, err := bazel.DetectBazelExecutable(ctx, bazelCommand) + if err != nil { + return nil, fmt.Errorf("detect bazel: %w", err) + } + + outputBase, err := bazelOutputBase(ctx, workspacePath, resolvedCommand) + if err != nil { + return nil, fmt.Errorf("bazel output base: %w", err) } + markerDir := filepath.Join(outputBase, "external") entries, err := os.ReadDir(markerDir) if err != nil { @@ -84,3 +96,18 @@ func readMarkerHash(path string) ([]byte, error) { } return hex.DecodeString(line) } + +// bazelOutputBase runs `bazel info output_base` and returns the path. +func bazelOutputBase(ctx context.Context, workspacePath, bazelCommand string) (string, error) { + cmd := execcmd.CommandContext(ctx, bazelCommand, "info", "output_base") + cmd.Dir = workspacePath + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("bazel info output_base: %w", err) + } + result := strings.TrimSpace(string(out)) + if result == "" { + return "", fmt.Errorf("bazel info output_base returned empty path") + } + return result, nil +} diff --git a/graphrunner/native.go b/graphrunner/native.go index e7892b6d..9b8a5a7c 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -102,12 +102,8 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) // hashes instead of just the repo name string. var repoMarkerHashes map[string][]byte if bzlmodEnabled { - outputBase, outputBaseErr := g.bazel.OutputBase(ctx) - if outputBaseErr != nil { - return targethasher.EmptyResult(), fmt.Errorf("bazel output base: %w", outputBaseErr) - } markerStart := time.Now() - repoMarkerHashes, err = readRepoMarkerHashes(outputBase) + repoMarkerHashes, err = targethasher.ReadRepoMarkerHashes(ctx, ws.Path(), g.config.BazelCommandPath) g.emitter.DurationHistogram(_opCompute, "marker_read_duration", metrics.FastDurationBuckets).RecordDuration(time.Since(markerStart)) if err != nil { return targethasher.EmptyResult(), fmt.Errorf("read repo marker hashes: %w", err) diff --git a/graphrunner/native_test.go b/graphrunner/native_test.go index a5761126..b057471c 100644 --- a/graphrunner/native_test.go +++ b/graphrunner/native_test.go @@ -44,7 +44,6 @@ func TestCompute_CallsBazelAndReturnsResult(t *testing.T) { }, }, }}}, nil) - bazelMock.EXPECT().OutputBase(gomock.Any()).Return(t.TempDir(), nil) gr := NewNativeGraphRunner(NativeGraphRunnerParams{ BazelClient: bazelMock, GitClient: gitMock, From b5d6608fd838d606725abadc867e8d3ac332cba6 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 10:37:44 -0700 Subject: [PATCH 16/23] Move bazelOutputBase to core/bazel as exported OutputBase OutputBase resolves the bazel executable internally via DetectBazelExecutable, so callers no longer need to handle detection separately. Updated BUILD.bazel files via gazelle. Co-Authored-By: Claude Sonnet 5 --- core/bazel/bazel.go | 21 +++++++++++++++++++++ core/targethasher/BUILD.bazel | 2 ++ core/targethasher/markers.go | 22 +--------------------- graphrunner/BUILD.bazel | 1 + graphrunner/native_test.go | 4 ++++ 5 files changed, 29 insertions(+), 21 deletions(-) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index b230d742..f525fb87 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -24,6 +24,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "time" buildpb "github.com/bazelbuild/buildtools/build_proto" @@ -182,3 +183,23 @@ func ensureBazelisk(ctx context.Context) (_ string, retErr error) { } return dest, nil } + +// OutputBase resolves the bazel executable and runs `bazel info output_base`, +// returning the absolute path to the output base directory. +func OutputBase(ctx context.Context, workspacePath, bazelCommand string) (string, error) { + resolved, err := DetectBazelExecutable(ctx, bazelCommand) + if err != nil { + return "", fmt.Errorf("detect bazel: %w", err) + } + cmd := execcmd.CommandContext(ctx, resolved, "info", "output_base") + cmd.Dir = workspacePath + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("bazel info output_base: %w", err) + } + result := strings.TrimSpace(string(out)) + if result == "" { + return "", fmt.Errorf("bazel info output_base returned empty path") + } + return result, nil +} diff --git a/core/targethasher/BUILD.bazel b/core/targethasher/BUILD.bazel index ea505f89..7b304093 100644 --- a/core/targethasher/BUILD.bazel +++ b/core/targethasher/BUILD.bazel @@ -4,11 +4,13 @@ go_library( name = "targethasher", srcs = [ "graph.go", + "markers.go", "sourcehasher.go", ], importpath = "github.com/uber/tango/core/targethasher", visibility = ["//visibility:public"], deps = [ + "//core/bazel", "@com_github_bazelbuild_buildtools//build_proto", "@com_github_bazelbuild_buildtools//labels", "@com_github_deckarep_golang_set_v2//:golang-set", diff --git a/core/targethasher/markers.go b/core/targethasher/markers.go index 131206c3..df788f83 100644 --- a/core/targethasher/markers.go +++ b/core/targethasher/markers.go @@ -24,7 +24,6 @@ import ( "strings" "github.com/uber/tango/core/bazel" - "github.com/uber/tango/core/execcmd" ) // ReadRepoMarkerHashes resolves the Bazel output base, then reads the @@ -33,12 +32,7 @@ import ( // This hash changes whenever the repo is upgraded (different URL, // sha256, patches, etc.). func ReadRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand string) (map[string][]byte, error) { - resolvedCommand, err := bazel.DetectBazelExecutable(ctx, bazelCommand) - if err != nil { - return nil, fmt.Errorf("detect bazel: %w", err) - } - - outputBase, err := bazelOutputBase(ctx, workspacePath, resolvedCommand) + outputBase, err := bazel.OutputBase(ctx, workspacePath, bazelCommand) if err != nil { return nil, fmt.Errorf("bazel output base: %w", err) } @@ -97,17 +91,3 @@ func readMarkerHash(path string) ([]byte, error) { return hex.DecodeString(line) } -// bazelOutputBase runs `bazel info output_base` and returns the path. -func bazelOutputBase(ctx context.Context, workspacePath, bazelCommand string) (string, error) { - cmd := execcmd.CommandContext(ctx, bazelCommand, "info", "output_base") - cmd.Dir = workspacePath - out, err := cmd.Output() - if err != nil { - return "", fmt.Errorf("bazel info output_base: %w", err) - } - result := strings.TrimSpace(string(out)) - if result == "" { - return "", fmt.Errorf("bazel info output_base returned empty path") - } - return result, nil -} diff --git a/graphrunner/BUILD.bazel b/graphrunner/BUILD.bazel index 99f8b0a7..3adf7fd0 100644 --- a/graphrunner/BUILD.bazel +++ b/graphrunner/BUILD.bazel @@ -26,6 +26,7 @@ go_test( srcs = ["native_test.go"], embed = [":graphrunner"], deps = [ + "//config", "//core/bazel", "//core/bazel/bazelmock", "//core/git/gitmock", diff --git a/graphrunner/native_test.go b/graphrunner/native_test.go index b057471c..9e04f4e7 100644 --- a/graphrunner/native_test.go +++ b/graphrunner/native_test.go @@ -21,6 +21,7 @@ import ( buildpb "github.com/bazelbuild/buildtools/build_proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/tango/config" "github.com/uber/tango/core/bazel" "github.com/uber/tango/core/bazel/bazelmock" gitmock "github.com/uber/tango/core/git/gitmock" @@ -28,6 +29,8 @@ import ( "go.uber.org/mock/gomock" ) +func boolPtr(b bool) *bool { return &b } + func TestCompute_CallsBazelAndReturnsResult(t *testing.T) { ctrl := gomock.NewController(t) bazelMock := bazelmock.NewMockBazel(ctrl) @@ -47,6 +50,7 @@ func TestCompute_CallsBazelAndReturnsResult(t *testing.T) { gr := NewNativeGraphRunner(NativeGraphRunnerParams{ BazelClient: bazelMock, GitClient: gitMock, + Config: config.RepositoryConfig{BzlmodEnabled: boolPtr(false)}, }) ws := workspace.NewWorkspace(workspace.WorkspaceParams{ Path: "/tmp/ws", From 6ac21c216d3df2121cde2fc3a62861b403316c8c Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 10:50:22 -0700 Subject: [PATCH 17/23] Remove trailing blank lines to fix lint Co-Authored-By: Claude Sonnet 5 --- core/bazel/bazelmock/bazelmock.go | 1 - core/targethasher/markers.go | 1 - 2 files changed, 2 deletions(-) diff --git a/core/bazel/bazelmock/bazelmock.go b/core/bazel/bazelmock/bazelmock.go index a1ef1554..9882710d 100644 --- a/core/bazel/bazelmock/bazelmock.go +++ b/core/bazel/bazelmock/bazelmock.go @@ -55,4 +55,3 @@ func (mr *MockBazelMockRecorder) ExecuteQuery(ctx, req any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExecuteQuery", reflect.TypeOf((*MockBazel)(nil).ExecuteQuery), ctx, req) } - diff --git a/core/targethasher/markers.go b/core/targethasher/markers.go index df788f83..27643799 100644 --- a/core/targethasher/markers.go +++ b/core/targethasher/markers.go @@ -90,4 +90,3 @@ func readMarkerHash(path string) ([]byte, error) { } return hex.DecodeString(line) } - From c81f9f5a18780e2578c33b22897c3d142c637022 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 11:04:30 -0700 Subject: [PATCH 18/23] Unexport detectBazelExecutable, now only used within core/bazel Co-Authored-By: Claude Sonnet 5 --- core/bazel/bazel.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index f525fb87..9202d3c2 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -94,7 +94,7 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { if timeout == 0 { timeout = _queryTimeout } - bazelCommand, err := DetectBazelExecutable(ctx, p.BazelCommand) + bazelCommand, err := detectBazelExecutable(ctx, p.BazelCommand) if err != nil { return nil, fmt.Errorf("detect bazel executable: %w", err) } @@ -110,10 +110,10 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { }, nil } -// DetectBazelExecutable returns the path to a bazel binary. +// detectBazelExecutable returns the path to a bazel binary. // If bazelCommand is explicitly provided, it is used as-is. // Otherwise, bazelisk is downloaded from GitHub into a local cache directory. -func DetectBazelExecutable(ctx context.Context, bazelCommand string) (string, error) { +func detectBazelExecutable(ctx context.Context, bazelCommand string) (string, error) { if bazelCommand != "" { return bazelCommand, nil } @@ -187,7 +187,7 @@ func ensureBazelisk(ctx context.Context) (_ string, retErr error) { // OutputBase resolves the bazel executable and runs `bazel info output_base`, // returning the absolute path to the output base directory. func OutputBase(ctx context.Context, workspacePath, bazelCommand string) (string, error) { - resolved, err := DetectBazelExecutable(ctx, bazelCommand) + resolved, err := detectBazelExecutable(ctx, bazelCommand) if err != nil { return "", fmt.Errorf("detect bazel: %w", err) } From 3b8841e34f8bc53b844de8d13d3a7df7845bf38e Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 11:14:15 -0700 Subject: [PATCH 19/23] Fix detectBazelExecutable comment to say bazelisk Co-Authored-By: Claude Sonnet 5 --- core/bazel/bazel.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index 9202d3c2..7391331c 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -110,7 +110,7 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { }, nil } -// detectBazelExecutable returns the path to a bazel binary. +// detectBazelExecutable returns the path to a bazelisk binary. // If bazelCommand is explicitly provided, it is used as-is. // Otherwise, bazelisk is downloaded from GitHub into a local cache directory. func detectBazelExecutable(ctx context.Context, bazelCommand string) (string, error) { From 393c771288098ab18ef14eda20d8592edcc7cc10 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 11:22:53 -0700 Subject: [PATCH 20/23] Add test for bzlmod collapse path in graphrunner Uses a fake bazel script and marker files on disk to exercise the end-to-end bzlmod collapse without a real bazel installation. Co-Authored-By: Claude Sonnet 5 --- graphrunner/native_test.go | 74 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/graphrunner/native_test.go b/graphrunner/native_test.go index 9e04f4e7..fe1584b4 100644 --- a/graphrunner/native_test.go +++ b/graphrunner/native_test.go @@ -16,6 +16,9 @@ package graphrunner import ( "context" + "encoding/hex" + "os" + "path/filepath" "testing" buildpb "github.com/bazelbuild/buildtools/build_proto" @@ -65,6 +68,77 @@ func TestCompute_CallsBazelAndReturnsResult(t *testing.T) { assert.Equal(t, ruleClass, res.Targets[ruleName].Rule.GetRuleClass()) } +func TestCompute_BzlmodCollapsesExternalTargets(t *testing.T) { + // Set up a fake output base with marker files so the bzlmod + // collapse path can read them without a real bazel installation. + outputBase := t.TempDir() + markerDir := filepath.Join(outputBase, "external") + require.NoError(t, os.MkdirAll(markerDir, 0o755)) + + markerHash := hex.EncodeToString([]byte("fake-hash-for-test")) + require.NoError(t, os.WriteFile( + filepath.Join(markerDir, "@myrepo.marker"), + []byte(markerHash+"\n"), + 0o644, + )) + + // Create a fake bazel script that prints the output base. + fakeBazel := filepath.Join(t.TempDir(), "fake-bazel") + require.NoError(t, os.WriteFile(fakeBazel, []byte("#!/bin/sh\necho "+outputBase+"\n"), 0o755)) + + ctrl := gomock.NewController(t) + bazelMock := bazelmock.NewMockBazel(ctrl) + gitMock := gitmock.NewMockInterface(ctrl) + gitMock.EXPECT().FileHashes(gomock.Any(), gomock.Any()).Return(map[string][]byte{}, nil) + + srcName := "@@myrepo//pkg:file.go" + srcType := "source file" + ruleName := "//:a" + ruleClass := "go_library" + bazelMock.EXPECT().ExecuteQuery(gomock.Any(), gomock.Any()).Return(&bazel.QueryResponse{Result: &buildpb.QueryResult{Target: []*buildpb.Target{ + { + Type: buildpb.Target_SOURCE_FILE.Enum(), + SourceFile: &buildpb.SourceFile{ + Name: &srcName, + }, + }, + { + Type: buildpb.Target_RULE.Enum(), + Rule: &buildpb.Rule{ + Name: &ruleName, + RuleClass: &ruleClass, + }, + }, + }}}, nil) + + gr := NewNativeGraphRunner(NativeGraphRunnerParams{ + BazelClient: bazelMock, + GitClient: gitMock, + Config: config.RepositoryConfig{ + BzlmodEnabled: boolPtr(true), + BazelCommandPath: fakeBazel, + }, + }) + ws := workspace.NewWorkspace(workspace.WorkspaceParams{ + Path: t.TempDir(), + }) + + res, err := gr.Compute(context.Background(), ws) + require.NoError(t, err) + require.NotNil(t, res) + + // The external source file should have been collapsed with a hash + // derived from the marker file, not left nil. + extTarget, ok := res.Targets[srcName] + require.True(t, ok, "external target %q should be in results", srcName) + assert.NotNil(t, extTarget.Hash, "collapsed external target should have a hash") + assert.Equal(t, srcType, extTarget.RuleType) + + // The internal rule target should also be present. + _, ok = res.Targets[ruleName] + assert.True(t, ok, "internal target %q should be in results", ruleName) +} + func TestCompute_PropagatesError(t *testing.T) { ctrl := gomock.NewController(t) bazelMock := bazelmock.NewMockBazel(ctrl) From 7791ceedbcdceb46bee0801386bf6852afcfde6a Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 13:35:24 -0700 Subject: [PATCH 21/23] Hash full marker file (excluding ENV lines) for patch correctness The first line of a marker file only hashes the repo rule's declarative inputs (URL, version, patch paths) but not the actual patch file contents. FILE: lines contain per-file SHA-256 content hashes that change when patches are modified. Hash all stable lines (first line + FILE lines, skip ENV lines) so patch content changes are detected. Co-Authored-By: Claude Sonnet 5 --- core/targethasher/markers.go | 39 +++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/core/targethasher/markers.go b/core/targethasher/markers.go index 27643799..3805fb70 100644 --- a/core/targethasher/markers.go +++ b/core/targethasher/markers.go @@ -17,7 +17,6 @@ package targethasher import ( "bufio" "context" - "encoding/hex" "fmt" "os" "path/filepath" @@ -28,9 +27,9 @@ import ( // ReadRepoMarkerHashes resolves the Bazel output base, then reads the // external repo marker files and returns a map from canonical repo name -// to the repo rule input hash (the first line of each marker file). -// This hash changes whenever the repo is upgraded (different URL, -// sha256, patches, etc.). +// to a hash derived from the stable lines of each marker file. +// This hash changes whenever the repo's version, URL, or patch file +// contents change. ENV lines are excluded for cross-environment stability. func ReadRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand string) (map[string][]byte, error) { outputBase, err := bazel.OutputBase(ctx, workspacePath, bazelCommand) if err != nil { @@ -67,9 +66,12 @@ func ReadRepoMarkerHashes(ctx context.Context, workspacePath, bazelCommand strin return hashes, nil } -// readMarkerHash reads the first line of a marker file and hex-decodes -// it into raw bytes. The first line is a hash of the repository rule's -// inputs and is stable across runs for the same dependency version. +// readMarkerHash computes a hash from the stable lines of a marker file. +// The first line is a hash of the repo rule's declarative inputs (URL, +// version, patch paths) but does NOT include the content hashes of patch +// files. Those appear on FILE: lines alongside their SHA-256 content +// hashes. ENV: lines are skipped because environment variables can differ +// between CI environments and would cause unnecessary hash instability. func readMarkerHash(path string) ([]byte, error) { f, err := os.Open(path) if err != nil { @@ -77,16 +79,25 @@ func readMarkerHash(path string) ([]byte, error) { } defer func() { _ = f.Close() }() + h := newHash() + hasContent := false scanner := bufio.NewScanner(f) - if !scanner.Scan() { - if err := scanner.Err(); err != nil { - return nil, err + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue } - return nil, nil + if strings.HasPrefix(line, "ENV:") { + continue + } + h.Write([]byte(line)) + hasContent = true + } + if err := scanner.Err(); err != nil { + return nil, err } - line := strings.TrimSpace(scanner.Text()) - if line == "" { + if !hasContent { return nil, nil } - return hex.DecodeString(line) + return h.Sum(nil), nil } From 0d1cf055e101c9c80f7efa5125699502a04be2d9 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 14:51:36 -0700 Subject: [PATCH 22/23] Address PR review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename shouldCollapse → shouldCollapseToBzlmodRepo - Rename collapseBzlmodExternalTargets → HashExternalTargetsBzlmod to mirror legacy HashExternalTargets - Error when bzlmod enabled but no marker hashes found - Update doc comments with patch handling details Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 37 +++++++++++++++++++-------------- core/targethasher/graph_test.go | 18 ++++++++-------- graphrunner/native.go | 3 +++ 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index a75749eb..3b6c72fd 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -80,7 +80,7 @@ type HashConfig struct { AllTargetsFiles []string // RepoMarkerHashes maps canonical bzlmod repo names to repo rule input // hashes read from Bazel's marker files ($(output_base)/external/@repo.marker). - // Used by collapseBzlmodExternalTargets to pre-hash external source files + // Used by HashExternalTargetsBzlmod to pre-hash external source files // without reading their content from disk. The marker hash changes on any // dependency upgrade, so repos whose canonical name stays the same across // versions (e.g. "protobuf+") are still correctly detected as changed. @@ -409,11 +409,11 @@ func bzlmodRepoName(targetName string) string { return rest[:idx] } -// shouldCollapse reports whether a bzlmod external target should be -// pre-hashed using the repo's marker file hash instead of hashing its -// file content during the DFS. Only source and generated files are -// collapsed; rule targets are left alone so dependency edges are preserved. -func shouldCollapse(target *Target, repo string, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) bool { +// shouldCollapseToBzlmodRepo reports whether a bzlmod external target should +// be pre-hashed using the repo's marker file hash instead of hashing its +// file content during the DFS. Only source and generated files are collapsed; +// rule targets are left alone so dependency edges are preserved. +func shouldCollapseToBzlmodRepo(target *Target, repo string, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp) bool { if target == nil { return false } @@ -432,14 +432,19 @@ func shouldCollapse(target *Target, repo string, fullHashRepos set.Set[string], return true } -// collapseBzlmodExternalTargets pre-hashes bzlmod external source and -// generated file targets using hashes from Bazel's marker files. This is -// the bzlmod equivalent of legacy WORKSPACE //external:repo collapsing. -// Marker file hashes change on any dependency upgrade, so the collapsed -// hash is content-aware even for repos whose canonical name stays the same -// across versions (e.g. "protobuf+"). Repos without a marker are skipped -// and fall through to HashRecursively for per-file content hashing. -func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) { +// HashExternalTargetsBzlmod pre-hashes bzlmod external source and +// generated file targets using hashes derived from Bazel's marker files. +// This is the bzlmod equivalent of legacy WORKSPACE HashExternalTargets. +// +// Marker files track both the repo rule's declarative inputs (version, +// URL, integrity) and per-file SHA-256 content hashes for local patches +// applied via single_version_override. readMarkerHash hashes all stable +// lines (skipping ENV) so the collapsed hash changes on dependency +// upgrades AND patch content modifications. +// +// Repos without a marker are skipped and fall through to HashRecursively +// for per-file content hashing. +func HashExternalTargetsBzlmod(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) { if len(repoMarkerHashes) == 0 { return } @@ -447,7 +452,7 @@ func collapseBzlmodExternalTargets(targets map[string]*Target, fullHashRepos set repoHashes := make(map[string][]byte) for _, target := range targets { repo := bzlmodRepoName(target.Name) - if !shouldCollapse(target, repo, fullHashRepos, excludedRegex) { + if !shouldCollapseToBzlmodRepo(target, repo, fullHashRepos, excludedRegex) { continue } @@ -520,7 +525,7 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, // Bazel marker file hashes. Avoids visiting millions of individual // pip-wheel files during the DFS — the bzlmod equivalent of legacy // WORKSPACE //external:repo collapsing. - collapseBzlmodExternalTargets(targets, fullHashRepos, excludedRegex, repoMarkerHashes) + HashExternalTargetsBzlmod(targets, fullHashRepos, excludedRegex, repoMarkerHashes) } // get topological roots and update buildable roots info diff --git a/core/targethasher/graph_test.go b/core/targethasher/graph_test.go index b5c9c27a..c0c34e6a 100644 --- a/core/targethasher/graph_test.go +++ b/core/targethasher/graph_test.go @@ -402,7 +402,7 @@ func TestBzlmodRepoName(t *testing.T) { } } -func TestCollapseBzlmodExternalTargets(t *testing.T) { +func TestHashExternalTargetsBzlmod(t *testing.T) { markerA := []byte{0xaa, 0xbb, 0xcc} markerB := []byte{0xdd, 0xee, 0xff} markers := map[string][]byte{ @@ -419,7 +419,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "//src:main": {Name: "//src:main", RuleType: "go_binary"}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) + HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) assert.NotNil(t, targets["@@repo_a//pkg:file1.py"].Hash) assert.Equal(t, targets["@@repo_a//pkg:file1.py"].Hash, targets["@@repo_a//pkg:file2.py"].Hash) @@ -432,7 +432,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@no_marker_repo//pkg:file.py": {Name: "@@no_marker_repo//pkg:file.py", RuleType: SourceFileType, External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) + HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) assert.Nil(t, targets["@@no_marker_repo//pkg:file.py"].Hash) }) @@ -441,7 +441,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, nil) + HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, nil) assert.Nil(t, targets["@@repo_a//pkg:file.py"].Hash) }) @@ -450,7 +450,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_a//pkg:lib": {Name: "@@repo_a//pkg:lib", RuleType: "go_library", External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) + HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) assert.Nil(t, targets["@@repo_a//pkg:lib"].Hash) }) @@ -459,7 +459,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet("", "repo_a"), nil, markers) + HashExternalTargetsBzlmod(targets, set.NewSet("", "repo_a"), nil, markers) assert.Nil(t, targets["@@repo_a//pkg:file.py"].Hash) }) @@ -470,7 +470,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { } excluded := []*regexp.Regexp{regexp.MustCompile(`\.whl$`)} - collapseBzlmodExternalTargets(targets, set.NewSet(""), excluded, markers) + HashExternalTargetsBzlmod(targets, set.NewSet(""), excluded, markers) assert.Nil(t, targets["@@rules_python++pip+foo//pkg:file.whl"].Hash) assert.NotNil(t, targets["@@rules_python++pip+foo//pkg:module.py"].Hash) @@ -482,7 +482,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_b//pkg:file.py": {Name: "@@repo_b//pkg:file.py", RuleType: SourceFileType, External: true}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) + HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) assert.NotEqual(t, targets["@@repo_a//pkg:file.py"].Hash, targets["@@repo_b//pkg:file.py"].Hash) }) @@ -492,7 +492,7 @@ func TestCollapseBzlmodExternalTargets(t *testing.T) { "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true, Hash: existing}, } - collapseBzlmodExternalTargets(targets, set.NewSet(""), nil, markers) + HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) assert.Equal(t, existing, targets["@@repo_a//pkg:file.py"].Hash) }) } diff --git a/graphrunner/native.go b/graphrunner/native.go index 9b8a5a7c..0b8fcf2c 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -108,6 +108,9 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) if err != nil { return targethasher.EmptyResult(), fmt.Errorf("read repo marker hashes: %w", err) } + if len(repoMarkerHashes) == 0 { + return targethasher.EmptyResult(), fmt.Errorf("bzlmod enabled but no repo marker hashes found") + } } hashConfig := targethasher.HashConfig{ From 924158945d1d0708fea4bed16d1cabdf57f7ffd3 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 11 Sep 2026 15:06:58 -0700 Subject: [PATCH 23/23] Error when bzlmod repo has targets but no marker file After bazel query completes, every referenced external repo should have a marker file. A missing marker indicates something is wrong with the output base rather than a normal fallback scenario. Co-Authored-By: Claude Sonnet 5 --- core/targethasher/graph.go | 16 ++++++++++------ core/targethasher/graph_test.go | 23 ++++++++++++----------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 3b6c72fd..f37f4330 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -442,11 +442,12 @@ func shouldCollapseToBzlmodRepo(target *Target, repo string, fullHashRepos set.S // lines (skipping ENV) so the collapsed hash changes on dependency // upgrades AND patch content modifications. // -// Repos without a marker are skipped and fall through to HashRecursively -// for per-file content hashing. -func HashExternalTargetsBzlmod(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) { +// Every external repo referenced in the query result should have a +// marker file after bazel query completes. Returns an error if a +// collapsible repo is missing its marker. +func HashExternalTargetsBzlmod(targets map[string]*Target, fullHashRepos set.Set[string], excludedRegex []*regexp.Regexp, repoMarkerHashes map[string][]byte) error { if len(repoMarkerHashes) == 0 { - return + return nil } repoHashes := make(map[string][]byte) @@ -460,7 +461,7 @@ func HashExternalTargetsBzlmod(targets map[string]*Target, fullHashRepos set.Set if !ok { markerHash, hasMarker := repoMarkerHashes[repo] if !hasMarker || len(markerHash) == 0 { - continue + return fmt.Errorf("bzlmod repo %q has targets in query but no marker file", repo) } rh := newHash() rh.Write(markerHash) @@ -471,6 +472,7 @@ func HashExternalTargetsBzlmod(targets map[string]*Target, fullHashRepos set.Set target.Hash = h target.HashWithoutDeps = h } + return nil } // GetTopologicalRootsAndIdentifyBuildableRoots returns a list of topological roots and marks buildable roots in the target graph @@ -525,7 +527,9 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, // Bazel marker file hashes. Avoids visiting millions of individual // pip-wheel files during the DFS — the bzlmod equivalent of legacy // WORKSPACE //external:repo collapsing. - HashExternalTargetsBzlmod(targets, fullHashRepos, excludedRegex, repoMarkerHashes) + if err := HashExternalTargetsBzlmod(targets, fullHashRepos, excludedRegex, repoMarkerHashes); err != nil { + return EmptyResult(), err + } } // get topological roots and update buildable roots info diff --git a/core/targethasher/graph_test.go b/core/targethasher/graph_test.go index c0c34e6a..94ecd897 100644 --- a/core/targethasher/graph_test.go +++ b/core/targethasher/graph_test.go @@ -419,7 +419,7 @@ func TestHashExternalTargetsBzlmod(t *testing.T) { "//src:main": {Name: "//src:main", RuleType: "go_binary"}, } - HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) + require.NoError(t, HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers)) assert.NotNil(t, targets["@@repo_a//pkg:file1.py"].Hash) assert.Equal(t, targets["@@repo_a//pkg:file1.py"].Hash, targets["@@repo_a//pkg:file2.py"].Hash) @@ -427,21 +427,22 @@ func TestHashExternalTargetsBzlmod(t *testing.T) { assert.Nil(t, targets["//src:main"].Hash) }) - t.Run("skips repos without marker (falls through to HashRecursively)", func(t *testing.T) { + t.Run("errors on repos without marker", func(t *testing.T) { targets := map[string]*Target{ "@@no_marker_repo//pkg:file.py": {Name: "@@no_marker_repo//pkg:file.py", RuleType: SourceFileType, External: true}, } - HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) - assert.Nil(t, targets["@@no_marker_repo//pkg:file.py"].Hash) + err := HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) + require.Error(t, err) + assert.Contains(t, err.Error(), "no_marker_repo") }) - t.Run("skips repos when no markers provided at all", func(t *testing.T) { + t.Run("no-op when no markers provided at all", func(t *testing.T) { targets := map[string]*Target{ "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, } - HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, nil) + require.NoError(t, HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, nil)) assert.Nil(t, targets["@@repo_a//pkg:file.py"].Hash) }) @@ -450,7 +451,7 @@ func TestHashExternalTargetsBzlmod(t *testing.T) { "@@repo_a//pkg:lib": {Name: "@@repo_a//pkg:lib", RuleType: "go_library", External: true}, } - HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) + require.NoError(t, HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers)) assert.Nil(t, targets["@@repo_a//pkg:lib"].Hash) }) @@ -459,7 +460,7 @@ func TestHashExternalTargetsBzlmod(t *testing.T) { "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true}, } - HashExternalTargetsBzlmod(targets, set.NewSet("", "repo_a"), nil, markers) + require.NoError(t, HashExternalTargetsBzlmod(targets, set.NewSet("", "repo_a"), nil, markers)) assert.Nil(t, targets["@@repo_a//pkg:file.py"].Hash) }) @@ -470,7 +471,7 @@ func TestHashExternalTargetsBzlmod(t *testing.T) { } excluded := []*regexp.Regexp{regexp.MustCompile(`\.whl$`)} - HashExternalTargetsBzlmod(targets, set.NewSet(""), excluded, markers) + require.NoError(t, HashExternalTargetsBzlmod(targets, set.NewSet(""), excluded, markers)) assert.Nil(t, targets["@@rules_python++pip+foo//pkg:file.whl"].Hash) assert.NotNil(t, targets["@@rules_python++pip+foo//pkg:module.py"].Hash) @@ -482,7 +483,7 @@ func TestHashExternalTargetsBzlmod(t *testing.T) { "@@repo_b//pkg:file.py": {Name: "@@repo_b//pkg:file.py", RuleType: SourceFileType, External: true}, } - HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) + require.NoError(t, HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers)) assert.NotEqual(t, targets["@@repo_a//pkg:file.py"].Hash, targets["@@repo_b//pkg:file.py"].Hash) }) @@ -492,7 +493,7 @@ func TestHashExternalTargetsBzlmod(t *testing.T) { "@@repo_a//pkg:file.py": {Name: "@@repo_a//pkg:file.py", RuleType: SourceFileType, External: true, Hash: existing}, } - HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers) + require.NoError(t, HashExternalTargetsBzlmod(targets, set.NewSet(""), nil, markers)) assert.Equal(t, existing, targets["@@repo_a//pkg:file.py"].Hash) }) }