diff --git a/MODULE.bazel b/MODULE.bazel index d6105595..5519f7f8 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -31,6 +31,7 @@ go_deps.from_file(go_mod = "//:go.mod") use_repo( go_deps, "com_github_bazelbuild_buildtools", + "com_github_deckarep_golang_set_v2", "com_github_goccy_go_yaml", "com_github_gofrs_flock", "com_github_gogo_protobuf", diff --git a/core/bazelrunner/native.go b/core/bazelrunner/native.go index 57aa01ca..fb18290e 100644 --- a/core/bazelrunner/native.go +++ b/core/bazelrunner/native.go @@ -56,7 +56,8 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) FullHashRepos: g.config.FullHashRepos, ExcludedFiles: g.config.ExcludedFiles, } - res, err := targethasher.FromProto(ctx, queryResult.Result, ws.Path(), hashConfig, g.config.BzlmodEnabled) + // TODO: add bzlmod support + res, err := targethasher.FromProto(ctx, queryResult.Result, ws.Path(), hashConfig) if err != nil { return targethasher.EmptyResult(), err } diff --git a/core/targethasher/BUILD.bazel b/core/targethasher/BUILD.bazel index e7ab20e6..015ba2c3 100644 --- a/core/targethasher/BUILD.bazel +++ b/core/targethasher/BUILD.bazel @@ -9,18 +9,24 @@ go_library( importpath = "github.com/uber/tango/core/targethasher", visibility = ["//visibility:public"], deps = [ - "//tangopb", "@com_github_bazelbuild_buildtools//build_proto", "@com_github_bazelbuild_buildtools//labels", + "@com_github_deckarep_golang_set_v2//:golang-set", ], ) go_test( name = "targethasher_test", - srcs = ["sourcehasher_test.go"], + srcs = [ + "graph_test.go", + "sourcehasher_test.go", + ], embed = [":targethasher"], deps = [ "@com_github_bazelbuild_buildtools//build_proto", + "@com_github_deckarep_golang_set_v2//:golang-set", + "@com_github_google_go_cmp//cmp", + "@com_github_google_go_cmp//cmp/cmpopts", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", ], diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 5bb0edf7..6e179839 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -3,13 +3,21 @@ package targethasher import ( "context" "fmt" + "path/filepath" + "regexp" "sort" "strings" buildpb "github.com/bazelbuild/buildtools/build_proto" + "github.com/bazelbuild/buildtools/labels" + set "github.com/deckarep/golang-set/v2" ) const ( + // With bazel query, repository rules defined in the WORKSPACE file + // are queryable with this prefix. See: + // https://docs.bazel.build/versions/master/query.html#external-repos + externalWorkspaceRulePrefix = "//external:" // SourceFileType is a bazel query pattern for referring to source files in // queries. See: https://docs.bazel.build/versions/master/query.html#kind @@ -25,31 +33,20 @@ const ( // GeneratedFileType is a bazel query pattern for referring to generated files // in queries. See: https://docs.bazel.build/versions/master/query.html#kind GeneratedFileType = "generated file" -) - -// Result contains a graph of targethashes calculated based on a bazel query. -type Result struct { - - // TargetNames is a topoplogically ordered list of target names. - TargetNames []string - // Targets contains a map of target names to Target for each target - Targets map[string]Target - - // ExternalTargets contains a map of external target names to Target - ExternalTargets map[string]Target + UnknownRuleType = "unknown rule" +) - // Warnings contains lists of possible issues encountered while trying to - // calculate target hashes. These can happen in a few cases: - // - // - Bazel targets specify a dependency on a directory, rather than source - // files. In this case the build system can only track changes in the - // directory itself, not its contents. This could impact the accuracy of - // the targethash but is not considered fatal as the build system cannot - // track this correctly either. - // - // For more info see https://docs.bazel.build/versions/master/build-ref.html#label_directory. - Warnings map[string]error +// HashConfig fine tunes behaviors during target hashing. +type HashConfig struct { + // KnownSourceHashes can be used in-place of generating a hash from disk for a given source file. The values typically + // can be produced by a VCS (e.g. `git ls-tree`). + KnownSourceHashes map[string][]byte + // FullHashRepos is the list of repositories that requires hashing each individual files. By default, the hash + // for a repository rule is used as the hash for all files in the repository. Hashing individual files is more + // accurate, but slower. + FullHashRepos []string + ExcludedFiles []string // TODO: rename to ExcludedRegex } // Target contains information about the hash for a single target @@ -72,47 +69,60 @@ type Target struct { Attributes []*buildpb.Attribute } +// Result contains a graph of targethashes calculated based on a bazel query. +type Result struct { + + // TargetNames is a topoplogically ordered list of target names. + TargetNames []string + + // Targets contains a map of target names to Target for each target. + // This includes both internal targets and external rule targets (//external:*). + Targets map[string]*Target + + // Warnings contains lists of possible issues encountered while trying to + // calculate target hashes. These can happen in a few cases: + // + // - Bazel targets specify a dependency on a directory, rather than source + // files. In this case the build system can only track changes in the + // directory itself, not its contents. This could impact the accuracy of + // the targethash but is not considered fatal as the build system cannot + // track this correctly either. + // + // For more info see https://docs.bazel.build/versions/master/build-ref.html#label_directory. + Warnings map[string]error +} + // EmptyResult returns a result for no targets. func EmptyResult() Result { return Result{ - Targets: make(map[string]Target), + Targets: make(map[string]*Target), } } // FromProto calculates a target hash graph based on a query result and workspace root // Because `bazel query --output=proto --order_output=full` is very expensive, we make all the // required computations in this function, so Bazel can be executed with `--order_output=no`. -func FromProto(ctx context.Context, r *buildpb.QueryResult, workspaceroot string, hashConfig HashConfig, bzlmodEnabled bool) (Result, error) { +func FromProto(ctx context.Context, r *buildpb.QueryResult, workspaceroot string, hashConfig HashConfig) (Result, error) { // always calculate hash for individual files in the main repo. - fullHashRepos := map[string]struct{}{"": struct{}{}} - for _, repo := range hashConfig.FullHashRepos { - fullHashRepos[repo] = struct{}{} - } - excludedFiles := map[string]struct{}{} - for _, repo := range hashConfig.ExcludedFiles { - excludedFiles[repo] = struct{}{} - } - // Create a m + fullHashRepos := set.NewSet(append([]string{""}, hashConfig.FullHashRepos...)...) + excludedRegex := set.NewSet(hashConfig.ExcludedFiles...) return fromProto(ctx, r, &diskHashHelper{ workspaceroot: workspaceroot, knownFileHashes: hashConfig.KnownSourceHashes, - fullHashRepos: fullHashRepos, - excludedFiles: excludedFiles, - bzlmodEnabled: bzlmodEnabled, - }) + }, workspaceroot, fullHashRepos, excludedRegex) } // 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{}) + return fromProto(ctx, r, &noOpHasher{}, "", set.NewSet[string](), set.NewSet[string]()) } // for external targets, url and urls attributes could cause non-deterministic hash values, // depending on if port number is present or not // as a short term solution, remove these two attributes only if attribute sha256 exists on the external target var ( - urlAttrs = map[string]struct{}{"url": struct{}{}, "urls": struct{}{}} + urlAttrs = set.NewSet("url", "urls") sha256Attr = "sha256" integrityAttr = "integrity" ) @@ -137,14 +147,14 @@ func removeURLAttrs(t *buildpb.Target) { newAttrs := make([]*buildpb.Attribute, 0, len(oldAttrs)) for _, attr := range oldAttrs { - if _, ok := urlAttrs[attr.GetName()]; !ok { + if !urlAttrs.Contains(attr.GetName()) { newAttrs = append(newAttrs, attr) } } t.Rule.Attribute = newAttrs } -func toTarget(t *buildpb.Target) (Target, bool, error) { +func toTarget(t *buildpb.Target) (*Target, error) { switch *t.Type { case buildpb.Target_RULE: targetName := t.Rule.GetName() @@ -152,22 +162,25 @@ func toTarget(t *buildpb.Target) (Target, bool, error) { // sorting dependencies of rules, just like we do when calculating hashes for these rules. sort.Strings(deps) h := newHash() + // TODO: remove this and handle external targets in the same way as internal targets if strings.HasPrefix(targetName, externalWorkspaceRulePrefix) { // if this is an external target, remove unwanted attributes from the rule, e.g. url and urls removeURLAttrs(t) // Workspace rule usually representing external repository or an HTTP file. // It is only used to hash external source files, so store it in a separate map. HashRuleCommon(t.Rule, h) - return Target{ - Name: targetName, - Hash: h.Sum(nil), - RuleType: ExternalRuleType, - Deps: deps, - Rule: t.Rule, - }, true, nil + + return &Target{ + Name: targetName, + RuleType: ExternalRuleType, + Deps: deps, + Rule: t.Rule, + HashWithoutDeps: h.Sum(nil), + External: true, + }, nil } HashRuleCommon(t.Rule, h) - return Target{ + return &Target{ Name: targetName, HashWithoutDeps: h.Sum(nil), RuleType: t.Rule.GetRuleClass(), @@ -176,59 +189,97 @@ func toTarget(t *buildpb.Target) (Target, bool, error) { External: isExternalTarget(targetName), Rule: t.Rule, Attributes: t.Rule.GetAttribute(), - }, false, nil + }, nil case buildpb.Target_SOURCE_FILE: targetName := t.SourceFile.GetName() - return Target{ + return &Target{ Name: targetName, RuleType: SourceFileType, External: isExternalTarget(targetName), SourceFile: t.GetSourceFile(), - }, false, nil + }, nil case buildpb.Target_GENERATED_FILE: targetName := t.GeneratedFile.GetName() - return Target{ + return &Target{ Name: targetName, RuleType: GeneratedFileType, Deps: []string{t.GeneratedFile.GetGeneratingRule()}, External: isExternalTarget(targetName), - }, false, nil + }, nil case buildpb.Target_PACKAGE_GROUP: targetName := t.PackageGroup.GetName() - return Target{ + return &Target{ Name: targetName, RuleType: PackageGroup, External: isExternalTarget(targetName), - }, false, nil + }, nil default: - return Target{}, false, fmt.Errorf("cannot handle target type %q", buildpb.Target_Discriminator_name[int32(*t.Type)]) + return nil, fmt.Errorf("cannot handle target type %q", buildpb.Target_Discriminator_name[int32(*t.Type)]) } } -// GetTargetsWithoutHashAndRootInfo translate raw query result to a map of targets and a map of external targets without hash and root information -func GetTargetsWithoutHashAndRootInfo(ctx context.Context, r *buildpb.QueryResult) (targets map[string]Target, externalTargets map[string]Target, err error) { - targets = make(map[string]Target) - externalTargets = make(map[string]Target) +// GetInternalTargetsWithoutHashAndRootInfo this goes through *buildpb.QueryResult and return a list of internal targets without hash and root info. Repository rule targets (//external:*) are ignored in this function +func GetInternalTargetsWithoutHashAndRootInfo(ctx context.Context, r *buildpb.QueryResult) (map[string]*Target, error) { + targets := make(map[string]*Target) for _, t := range r.Target { if err := ctx.Err(); err != nil { - return nil, nil, err + return nil, err } - target, isExternalWorkspace, err := toTarget(t) + if t.GetRule() != nil && strings.HasPrefix(t.GetRule().GetName(), externalWorkspaceRulePrefix) { + continue + } + + target, err := toTarget(t) if err != nil { - return nil, nil, err + return nil, err } - if isExternalWorkspace { - externalTargets[target.Name] = target - } else { + targets[target.Name] = target + } + + return targets, nil +} + +// HashExternalTargets this goes through *buildpb.QueryResult again and hash repository rules recursively. +// It adds external rule targets (//external:*) directly into the provided targets map and hashes them. +func HashExternalTargets(ctx context.Context, r *buildpb.QueryResult, targets map[string]*Target, hasher SourceHasher, workspaceroot string, fullHashRepos set.Set[string], warns map[string]error) error { + for _, t := range r.Target { + if err := ctx.Err(); err != nil { + return err + } + + if t.GetRule() != nil && strings.HasPrefix(t.GetRule().GetName(), externalWorkspaceRulePrefix) { + target, err := toTarget(t) + if err != nil { + return err + } targets[target.Name] = target } } - return targets, externalTargets, nil + + hashParam := HashParam{ + Targets: targets, + Hasher: hasher, + Warns: warns, + WorkspaceRoot: workspaceroot, + FullHashRepos: fullHashRepos, + // we should not exclude anything for external repository rules + ExcludedRegex: set.NewSet[string](), + } + for name, target := range targets { + if target.RuleType == ExternalRuleType { + hashParam.TargetName = name + if _, err := HashRecursively(ctx, hashParam); err != nil { + return err + } + } + } + + return nil } // GetTopologicalRootsAndIdentifyBuildableRoots returns a list of topological roots and marks buildable roots in the target graph -func GetTopologicalRootsAndIdentifyBuildableRoots(targets map[string]Target) []string { +func GetTopologicalRootsAndIdentifyBuildableRoots(targets map[string]*Target) []string { // get targets that cannot be root, i.e. dependencies of some other targets notRoots := make(map[string]struct{}, len(targets)) for _, target := range targets { @@ -249,7 +300,6 @@ func GetTopologicalRootsAndIdentifyBuildableRoots(targets map[string]Target) []s // a separate `roots` variable for the purpose of this algorithm which will have all nodes not // referenced by other nodes. target.Root = true - targets[name] = target } } } @@ -261,21 +311,35 @@ func GetTopologicalRootsAndIdentifyBuildableRoots(targets map[string]Target) []s return roots } -func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher) (Result, error) { +func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher, workspaceroot string, fullHashRepos set.Set[string], excludedRegex set.Set[string]) (Result, error) { + warns := make(map[string]error) // Build target graph with dependencies, but without hash and root information. - targets, externalTargets, err := GetTargetsWithoutHashAndRootInfo(ctx, r) + targets, err := GetInternalTargetsWithoutHashAndRootInfo(ctx, r) if err != nil { return EmptyResult(), err } + + // add external rule targets (//external:*) to the same map and hash them + if err := HashExternalTargets(ctx, r, targets, hasher, workspaceroot, fullHashRepos, warns); err != nil { + return EmptyResult(), err + } // get topological roots and update buildable roots info roots := GetTopologicalRootsAndIdentifyBuildableRoots(targets) // Target graph is constructed with all the dependencies. Traverse it now and build Merkle DAG by hashing file // contents and rule's metainfo and hashes of dependencies. // This is potentially parallelizable, see https://t3.uberinternal.com/browse/GM-1523 - warns := make(map[string]error) + hashParam := HashParam{ + Targets: targets, + Hasher: hasher, + Warns: warns, + WorkspaceRoot: workspaceroot, + FullHashRepos: fullHashRepos, + ExcludedRegex: excludedRegex, + } for _, name := range roots { - if _, err := HashRecursively(ctx, targets, externalTargets, name, hasher, warns); err != nil { + hashParam.TargetName = name + if _, err := HashRecursively(ctx, hashParam); err != nil { return Result{}, err } } @@ -312,21 +376,19 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher) // Consider all root-uncreachable cyclic targets to be pseudo-roots. // An alternative would be to designate a deterministic cycle-breaker as // the root, but this is likely not worh the extra complexity. - target := targets[name] - target.Root = true - targets[name] = target + targets[name].Root = true - if _, err := HashRecursively(ctx, targets, externalTargets, name, hasher, warns); err != nil { + hashParam.TargetName = name + if _, err := HashRecursively(ctx, hashParam); err != nil { return Result{}, err } } } return Result{ - TargetNames: targetNames, - Targets: targets, - ExternalTargets: externalTargets, - Warnings: warns, + TargetNames: targetNames, + Targets: targets, + Warnings: warns, }, nil } @@ -336,7 +398,7 @@ func fromProto(ctx context.Context, r *buildpb.QueryResult, hasher SourceHasher) // it as well, conforming to Golang slice modification pattern. // // Additionally, this function needs to honor context cancellation -func ToposortRecursively(ctx context.Context, targetHashes map[string]Target, name string, targets []string, visited map[string]struct{}) ([]string, error) { +func ToposortRecursively(ctx context.Context, targetHashes map[string]*Target, name string, targets []string, visited map[string]struct{}) ([]string, error) { if ctx.Err() != nil { return nil, ctx.Err() } @@ -369,6 +431,17 @@ func ToposortRecursively(ctx context.Context, targetHashes map[string]Target, na return append(targets, name), nil } +// HashParam contains the parameters for HashRecursively. +type HashParam struct { + Targets map[string]*Target + TargetName string + Hasher SourceHasher + Warns map[string]error + WorkspaceRoot string + FullHashRepos set.Set[string] + ExcludedRegex set.Set[string] +} + // HashRecursively calculates hash for all of the target nodes in the target graph and updates Hash property for each // entry in `targetHashes` map. // It populates `warns` map with warnings when hash cannot be calculated but does not stop the calculation. An example @@ -377,36 +450,102 @@ func ToposortRecursively(ctx context.Context, targetHashes map[string]Target, na // point we should modify the code to only allow well-known files or errors to exhibit this behavior. // // Additionally, this function needs to honor context cancellation -func HashRecursively(ctx context.Context, targets, externalTargets map[string]Target, targetName string, hasher SourceHasher, warns map[string]error) ([]byte, error) { +func HashRecursively(ctx context.Context, p HashParam) ([]byte, error) { if ctx.Err() != nil { return nil, ctx.Err() } - if external, ok := externalTargets[targetName]; ok { - return []byte(external.Hash), nil + if t, ok := p.Targets[p.TargetName]; ok && t.Hash != nil { + return t.Hash, nil } - target, ok := targets[targetName] - if !ok { - return nil, fmt.Errorf("Unable to find target %s in the target graph", targetName) + + var target *Target + l := labels.Parse(p.TargetName) + // For external targets like @bazel_tools//tools/genrule:genrule-setup.sh, + // map the hash to //external:bazel_tools's hash (unless it's in FullHashRepos) + if isExternalTarget(p.TargetName) && l.Repository != "" && !p.FullHashRepos.Contains(l.Repository) { + translatedExternalTargetName := externalTargetForRule(p.TargetName) + if t, hasExternalRule := p.Targets[translatedExternalTargetName]; hasExternalRule { + target = t + } else { + // allows @... targets to be excluded even if //external:... doesn't exist. + if excluded, err := isExcluded(p.TargetName, p.ExcludedRegex); err != nil { + return nil, err + } else if excluded { + p.Targets[p.TargetName] = &Target{ + Name: p.TargetName, + RuleType: ExternalRuleType, + Hash: []byte{}, + HashWithoutDeps: []byte{}, + } + return []byte{}, nil + } + return nil, fmt.Errorf("cannot find external repository %s from external target %s", translatedExternalTargetName, p.TargetName) + } + } else if existingTarget, ok := p.Targets[p.TargetName]; ok { + target = existingTarget + } else { + // everything else + // such as unexported files + // //third_party/github.com/docker/docker:com_github_docker_docker_invalid_host_fix.patch + + if l.Repository != "" { + return nil, fmt.Errorf("unexpected repository from target %s", p.TargetName) + } + + path := filepath.Join(p.WorkspaceRoot, l.Package, l.Target) + sf := &buildpb.SourceFile{ + Name: &p.TargetName, + Location: &path, + } + target = &Target{ + Name: p.TargetName, + RuleType: SourceFileType, + External: false, + SourceFile: sf, + } + p.Targets[p.TargetName] = target } + // this node was already visited if target.Hash != nil { - // this node was already visited - return []byte(target.Hash), nil + if t, ok := p.Targets[p.TargetName]; ok { + t.Hash = target.Hash + t.HashWithoutDeps = target.HashWithoutDeps + } else { + // when @ target is translated to external rule, @ target may not exist, + // we should update hash for original target as well. + p.Targets[p.TargetName] = &Target{ + Name: p.TargetName, + RuleType: UnknownRuleType, + Hash: target.Hash, + HashWithoutDeps: target.HashWithoutDeps, + External: target.External, + } + } + return target.Hash, nil + } + + // excluded, return empty hash. + if excluded, err := isExcluded(p.TargetName, p.ExcludedRegex); err != nil { + return nil, err + } else if excluded { + p.Targets[p.TargetName].Hash = []byte{} + p.Targets[p.TargetName].HashWithoutDeps = []byte{} + return []byte{}, nil } // Mark node as visited by setting it to an empty slice instead of nil slice which it got by default - to avoid // cycles. It would be reset to a real hash value once dependencies are traversed. target.Hash = []byte{} target.HashWithoutDeps = []byte{} - targets[targetName] = target var hash []byte var hashWithoutDeps []byte switch target.RuleType { case SourceFileType: - h, err := hasher.HashSourceFile(target.SourceFile, externalTargets) + h, err := p.Hasher.HashSourceFile(target.SourceFile) if err != nil { return nil, err } @@ -415,7 +554,9 @@ func HashRecursively(ctx context.Context, targets, externalTargets map[string]Ta // In case of a generated file, it is sufficient to use the hash of the generating rule. // Note that generated file targets aren't explicitly defined targets, // but `out`'s of other rules: they have no attributes to hash. - if dephash, err := HashRecursively(ctx, targets, externalTargets, target.Deps[0], hasher, warns); err == nil { + depParam := p + depParam.TargetName = target.Deps[0] + if dephash, err := HashRecursively(ctx, depParam); err == nil { hash = dephash } else { return nil, err @@ -424,7 +565,7 @@ func HashRecursively(ctx context.Context, targets, externalTargets map[string]Ta // Package groups do not execute any action nor can be used as inputs of any action // There is no need to actually compute the hash, we include only the name for consistency h := newHash() - h.Write([]byte(targetName)) + h.Write([]byte(p.TargetName)) hash = h.Sum(nil) default: // Regular rule @@ -434,7 +575,9 @@ func HashRecursively(ctx context.Context, targets, externalTargets map[string]Ta hashWithoutDeps = noDepsHasher.Sum(nil) h.Write(hashWithoutDeps) for _, dep := range target.Deps { - if dephash, err := HashRecursively(ctx, targets, externalTargets, dep, hasher, warns); err == nil { + depParam := p + depParam.TargetName = dep + if dephash, err := HashRecursively(ctx, depParam); err == nil { h.Write(dephash) } else { return nil, err @@ -444,7 +587,7 @@ func HashRecursively(ctx context.Context, targets, externalTargets map[string]Ta } if hash != nil { - _, isNoOpHasher := hasher.(*noOpHasher) + _, isNoOpHasher := p.Hasher.(*noOpHasher) if isNoOpHasher { target.Hash = []byte{} target.HashWithoutDeps = []byte{} @@ -452,7 +595,18 @@ func HashRecursively(ctx context.Context, targets, externalTargets map[string]Ta target.Hash = hash target.HashWithoutDeps = hashWithoutDeps } - targets[targetName] = target + + // same as before. For translated external targets, ensure the original target is also added to + // the targets map + if _, ok := p.Targets[p.TargetName]; !ok && p.TargetName != target.Name { + p.Targets[p.TargetName] = &Target{ + Name: p.TargetName, + RuleType: UnknownRuleType, + Hash: target.Hash, + HashWithoutDeps: target.HashWithoutDeps, + External: target.External, + } + } } return hash, nil @@ -505,3 +659,16 @@ func hasAnyPrefix(t *buildpb.Target, rootRulePrefixes []string) bool { } return false } + +func isExcluded(targetName string, excludedRegex set.Set[string]) (bool, error) { + for expr := range excludedRegex.Iter() { + match, err := regexp.MatchString(expr, targetName) + if err != nil { + return false, fmt.Errorf("failed to check if excluded: %q", targetName) + } + if match { + return true, nil + } + } + return false, nil +} diff --git a/core/targethasher/graph_test.go b/core/targethasher/graph_test.go index b73409c5..d3c7aee5 100644 --- a/core/targethasher/graph_test.go +++ b/core/targethasher/graph_test.go @@ -1,4 +1,4 @@ -package targethash +package targethasher import ( "context" @@ -6,19 +6,21 @@ import ( "io" "os" "path" - "path/filepath" "strings" "testing" - "github.com/uber/tango/core/bazel" buildpb "github.com/bazelbuild/buildtools/build_proto" - "github.com/bazelbuild/buildtools/labels" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + set "github.com/deckarep/golang-set/v2" ) +func StringPtr(s string) *string { + return &s +} + type testHasher struct { known map[string][]byte } @@ -50,14 +52,14 @@ func TestFromProtoWithCyclicDependenciesNoRoot(t *testing.T) { { Type: buildpb.Target_RULE.Enum(), Rule: &buildpb.Rule{ - Name: ptr.String("//:a"), + Name: StringPtr("//:a"), RuleInput: []string{"//:b"}, }, }, { Type: buildpb.Target_RULE.Enum(), Rule: &buildpb.Rule{ - Name: ptr.String("//:b"), + Name: StringPtr("//:b"), RuleInput: []string{"//:a"}, }, }, @@ -82,12 +84,12 @@ func TestContextCancellation(t *testing.T) { qr := &buildpb.QueryResult{ Target: []*buildpb.Target{&buildpb.Target{}}, } - result, err := fromProto(ctx, qr, nil) + result, err := fromProto(ctx, qr, nil, "", set.NewSet[string](), set.NewSet[string]()) assert.Equal(t, EmptyResult(), result) assert.ErrorIs(t, err, context.Canceled) // verify fromProto honors context cancellation - bytes, err := HashRecursively(ctx, nil, nil, "", nil, nil) + bytes, err := HashRecursively(ctx, HashParam{}) assert.Empty(t, bytes) assert.ErrorIs(t, err, context.Canceled) @@ -101,63 +103,63 @@ func Test_RemoveAttrs(t *testing.T) { regularTarget := &buildpb.Target{ Type: buildpb.Target_RULE.Enum(), Rule: &buildpb.Rule{ - Name: ptr.String("//pkg:go_default_library"), + Name: StringPtr("//pkg:go_default_library"), Attribute: []*buildpb.Attribute{ &buildpb.Attribute{ - Name: ptr.String("url"), - StringValue: ptr.String("some_url"), + Name: StringPtr("url"), + StringValue: StringPtr("some_url"), }, &buildpb.Attribute{ - Name: ptr.String("urls"), + Name: StringPtr("urls"), StringListValue: []string{"url1", "url2"}, }, &buildpb.Attribute{ - Name: ptr.String("to_keep"), - StringValue: ptr.String("target1"), + Name: StringPtr("to_keep"), + StringValue: StringPtr("target1"), }, }, }, } - target, isExternal, err := toTarget(regularTarget) + target, err := toTarget(regularTarget) assert.NoError(t, err) - assert.False(t, isExternal) + assert.False(t, target.External) assert.Equal(t, regularTarget.GetRule().GetAttribute(), target.Attributes) assert.Equal(t, []byte{0x69, 0x8b, 0x5, 0x75, 0x55, 0x80, 0x66, 0x5d, 0x7e, 0xbc, 0x75, 0x4, 0x8e, 0x62, 0x48, 0xb0, 0x82, 0x9c, 0x87, 0x82}, target.HashWithoutDeps) externalTarget := &buildpb.Target{ Type: buildpb.Target_RULE.Enum(), Rule: &buildpb.Rule{ - Name: ptr.String("//external:some_rule"), + Name: StringPtr("//external:some_rule"), Attribute: []*buildpb.Attribute{ &buildpb.Attribute{ - Name: ptr.String("url"), - StringValue: ptr.String("some_url"), + Name: StringPtr("url"), + StringValue: StringPtr("some_url"), }, &buildpb.Attribute{ - Name: ptr.String("urls"), + Name: StringPtr("urls"), StringListValue: []string{"url1", "url2"}, }, &buildpb.Attribute{ - Name: ptr.String("to_keep"), - StringValue: ptr.String("external"), + Name: StringPtr("to_keep"), + StringValue: StringPtr("external"), }, }, }, } - external, isExternal, err := toTarget(externalTarget) + external, err := toTarget(externalTarget) assert.NoError(t, err) - assert.True(t, isExternal) - assert.Equal(t, []byte{0xbb, 0xee, 0x72, 0xbb, 0xda, 0x44, 0xa0, 0xb5, 0x27, 0x9f, 0x9c, 0xde, 0xda, 0xb3, 0xc9, 0x46, 0xbe, 0x7e, 0x14, 0x92}, external.Hash) + assert.True(t, external.External) + assert.Equal(t, []byte{0xbb, 0xee, 0x72, 0xbb, 0xda, 0x44, 0xa0, 0xb5, 0x27, 0x9f, 0x9c, 0xde, 0xda, 0xb3, 0xc9, 0x46, 0xbe, 0x7e, 0x14, 0x92}, external.HashWithoutDeps) // add sha256 attribute, hash should change externalTarget.Rule.Attribute = append(externalTarget.Rule.Attribute, &buildpb.Attribute{ - Name: ptr.String("sha256"), - StringValue: ptr.String("some_hash"), + Name: StringPtr("sha256"), + StringValue: StringPtr("some_hash"), }) - external, isExternal, err = toTarget(externalTarget) + external, err = toTarget(externalTarget) assert.NoError(t, err) - assert.True(t, isExternal) - assert.Equal(t, []byte{0x7c, 0xde, 0x91, 0xc2, 0x94, 0x1a, 0x22, 0xf3, 0xb2, 0x18, 0x7c, 0x21, 0xbf, 0x32, 0x17, 0xc0, 0xa3, 0xf0, 0xc, 0x77}, external.Hash) + assert.True(t, external.External) + assert.Equal(t, []byte{0x7c, 0xde, 0x91, 0xc2, 0x94, 0x1a, 0x22, 0xf3, 0xb2, 0x18, 0x7c, 0x21, 0xbf, 0x32, 0x17, 0xc0, 0xa3, 0xf0, 0xc, 0x77}, external.HashWithoutDeps) } func validateResultIsStable(t *testing.T, baseResult, result Result) { @@ -192,16 +194,3 @@ func assertEqualTargetHash(t *testing.T, expected, actual Target) { ignore := cmpopts.IgnoreFields(Target{}, "Attributes", "SourceFile", "Rule") assert.True(t, cmp.Equal(expected, actual, opt, ignore), cmp.Diff(expected, actual, opt)) } - -func parseQueryResult(t *testing.T, protoFile, workspace string) *buildpb.QueryResult { - qr, err := query.FromFile(protoFile) - require.NoError(t, err) - for _, target := range qr.Target { - sourceFile := target.GetSourceFile() - if sourceFile != nil { - label := labels.Parse(sourceFile.GetName()) - *sourceFile.Location = filepath.Join(workspace, label.Package, label.Target) - } - } - return qr -} diff --git a/core/targethasher/sourcehasher.go b/core/targethasher/sourcehasher.go index 9f7e0f0d..cbf7d750 100644 --- a/core/targethasher/sourcehasher.go +++ b/core/targethasher/sourcehasher.go @@ -8,23 +8,16 @@ import ( "io/fs" "os" "path/filepath" - "regexp" "sort" "strconv" "strings" - tangopb "github.com/uber/tango/tangopb" buildpb "github.com/bazelbuild/buildtools/build_proto" - "github.com/bazelbuild/buildtools/labels" ) var newHash = sha1.New const ( - // With bazel query, repository rules defined in the WORKSPACE file - // are queryable with this prefix. See: - // https://docs.bazel.build/versions/master/query.html#external-repos - externalWorkspaceRulePrefix = "//external:" externalWorkspaceFilePrefix = "@" _defaultSourceFileVisibility = "//visibility:private" ) @@ -33,7 +26,7 @@ const ( // can be calculated based on disk contents or form other sources such as a // vcs system. type SourceHasher interface { - HashSourceFile(s *buildpb.SourceFile, externalHashes map[string]Target) ([]byte, error) + HashSourceFile(s *buildpb.SourceFile) ([]byte, error) } // diskHashHelper is a SourceHasher that provides hashes based on disk @@ -44,9 +37,6 @@ type SourceHasher interface { type diskHashHelper struct { workspaceroot string knownFileHashes map[string][]byte - fullHashRepos map[string]struct{} - excludedFiles map[string]struct{} - bzlmodEnabled bool } // noOpHasher @@ -57,69 +47,22 @@ type noOpHasher struct { type Params struct { WorkspaceRoot string HashConfig HashConfig - BzlmodEnabled bool -} - -// HashConfig fine tunes behaviors during target hashing. -type HashConfig struct { - // KnownSourceHashes can be used in-place of generating a hash from disk for a given source file. The values typically - // can be produced by a VCS (e.g. `git ls-tree`). - KnownSourceHashes map[string][]byte - // FullHashRepos is the list of repositories that requires hashing each individual files. By default, the hash - // for a repository rule is used as the hash for all files in the repository. Hashing individual files is more - // accurate, but slower. - FullHashRepos []string - ExcludedFiles []string } // NewSourceHasher creates a new SourceHasher. func NewSourceHasher(p Params) SourceHasher { - fullHashRepos := make(map[string]struct{}) - for _, repo := range p.HashConfig.FullHashRepos { - fullHashRepos[repo] = struct{}{} - } - excludedFiles := make(map[string]struct{}) - for _, file := range p.HashConfig.ExcludedFiles { - excludedFiles[file] = struct{}{} - } return &diskHashHelper{ workspaceroot: p.WorkspaceRoot, knownFileHashes: p.HashConfig.KnownSourceHashes, - fullHashRepos: fullHashRepos, - excludedFiles: excludedFiles, - bzlmodEnabled: p.BzlmodEnabled, } } // HashSourceFile does a no-op hash for the noOpHasher. -func (hh *noOpHasher) HashSourceFile(sourceFile *buildpb.SourceFile, externalHashes map[string]Target) ([]byte, error) { +func (hh *noOpHasher) HashSourceFile(sourceFile *buildpb.SourceFile) ([]byte, error) { return nil, nil } -func (hh *diskHashHelper) HashSourceFile(sourceFile *buildpb.SourceFile, externalHashes map[string]Target) ([]byte, error) { - targetName := sourceFile.GetName() - - // If file is excluded, return empty hash - for expr := range hh.excludedFiles { - match, err := regexp.MatchString(expr, targetName) - if err != nil { - return nil, fmt.Errorf("Failed to check if excluded: %q", targetName) - } - if match { - return []byte{}, nil - } - } - - label := labels.Parse(targetName) - if _, ok := hh.fullHashRepos[label.Repository]; !ok { - // Use the hash for repository rules instead of hashing files one by one. - if h, ok := externalHashes[externalTargetForRule(targetName)]; ok { - return []byte(h.Hash), nil - } - if !hh.bzlmodEnabled { - return nil, fmt.Errorf("expected '//external:...' rule for: %q", targetName) - } - } +func (hh *diskHashHelper) HashSourceFile(sourceFile *buildpb.SourceFile) ([]byte, error) { nonDefaultVisibilities := filterVisibilityLabels(sourceFile.GetVisibilityLabel()) // The location may look like /foo/decl.go:1:1 location, _, _ := strings.Cut(sourceFile.GetLocation(), ":") @@ -130,12 +73,7 @@ func (hh *diskHashHelper) HashSourceFile(sourceFile *buildpb.SourceFile, externa } fi, err := os.Stat(location) - if os.IsNotExist(err) { - // Not considered a fatal error, e.g. a file is added to a dependency graph which is not - // present on the file system in certain scenarios, like flaky test filter. This is - // potentially dangerous as it can silently render an empty hash target graph. - return []byte{}, nil - } else if err != nil { + if err != nil { return nil, err } @@ -219,12 +157,12 @@ func pathForTarget(root, target string) string { return filepath.Join(root, parts[0], parts[1]) } -func hashRule(f *buildpb.Rule, targetHashes map[string]tangopb.OptimizedTarget) ([]byte, error) { +func hashRule(f *buildpb.Rule, targetHashes map[string]*Target) ([]byte, error) { h := newHash() HashRuleCommon(f, h) for _, dep := range f.GetRuleInput() { if dephash, ok := targetHashes[dep]; ok { - h.Write([]byte(dephash.Hash)) + h.Write(dephash.Hash) } else { return nil, fmt.Errorf("%q missing hash for dependency %q", f.GetName(), dep) } diff --git a/core/targethasher/sourcehasher_test.go b/core/targethasher/sourcehasher_test.go index 151cda99..cf2394b7 100644 --- a/core/targethasher/sourcehasher_test.go +++ b/core/targethasher/sourcehasher_test.go @@ -16,54 +16,22 @@ func TestNewSourceHasher_buildsMaps(t *testing.T) { WorkspaceRoot: "/ws", HashConfig: HashConfig{ KnownSourceHashes: map[string][]byte{"a/b.txt": []byte("abc")}, - FullHashRepos: []string{"", "extA"}, - ExcludedFiles: []string{`.*\.gen\.go`}, }, }) dh, ok := h.(*diskHashHelper) assert.True(t, ok, "expected *diskHashHelper, got %T", h) - _, ok = dh.fullHashRepos[""] - assert.True(t, ok, "fullHashRepos should contain main repo key \"\"") - _, ok = dh.fullHashRepos["extA"] - assert.True(t, ok, "fullHashRepos should contain \"extA\"") - _, ok = dh.excludedFiles[`.*\.gen\.go`] - assert.True(t, ok, "excludedFiles should contain the provided regex") + assert.Equal(t, "/ws", dh.workspaceroot, "workspaceroot mismatch") assert.Equal(t, "abc", string(dh.knownFileHashes["a/b.txt"]), "knownFileHashes mismatch") } func TestNoOpHasher_returnsNil(t *testing.T) { h := &noOpHasher{} sf := &buildpb.SourceFile{Name: strPtr("//:dummy")} - got, err := h.HashSourceFile(sf, nil) + got, err := h.HashSourceFile(sf) assert.NoError(t, err, "unexpected error: %v", err) assert.Nil(t, got, "expected nil hash, got %v", got) } -func TestDiskHashHelper_ExcludedFile(t *testing.T) { - tmp := t.TempDir() - wsRoot := tmp - - // File that matches the exclude pattern. - path := filepath.Join(wsRoot, "foo.gen.go") - require.NoError(t, os.WriteFile(path, []byte("hello"), 0o644)) - - h := &diskHashHelper{ - workspaceroot: wsRoot, - knownFileHashes: map[string][]byte{}, - fullHashRepos: map[string]struct{}{"": {}}, // main repo uses full hashing - excludedFiles: map[string]struct{}{`.*\.gen\.go`: {}}, - } - - sf := &buildpb.SourceFile{ - Name: strPtr("//:foo.gen.go"), - Location: strPtr(path + ":1:1"), - VisibilityLabel: []string{"//visibility:private"}, - } - hash, err := h.HashSourceFile(sf, nil) - assert.NoError(t, err, "unexpected error: %v", err) - assert.Equal(t, []byte{}, hash, "expected empty hash for excluded file, got %x", hash) -} - func TestDiskHashHelper_KnownFileHashUsed(t *testing.T) { tmp := t.TempDir() wsRoot := tmp @@ -80,15 +48,13 @@ func TestDiskHashHelper_KnownFileHashUsed(t *testing.T) { h := &diskHashHelper{ workspaceroot: wsRoot, knownFileHashes: map[string][]byte{filepath.ToSlash(rel): known}, - fullHashRepos: map[string]struct{}{"": {}}, // main repo full hashing enabled - excludedFiles: map[string]struct{}{}, } sf := &buildpb.SourceFile{ Name: strPtr("//" + filepath.ToSlash(filepath.Dir(rel)) + ":" + filepath.Base(rel)), Location: strPtr(abs + ":1:1"), VisibilityLabel: []string{"//visibility:private"}, } - got, err := h.HashSourceFile(sf, nil) + got, err := h.HashSourceFile(sf) assert.NoError(t, err, "unexpected err: %v", err) assert.Equal(t, string(known), string(got), "expected known hash %q, got %q", known, got) } @@ -108,57 +74,71 @@ func TestDiskHashHelper_NonDefaultVisibilityForcesDiskHash(t *testing.T) { h := &diskHashHelper{ workspaceroot: wsRoot, knownFileHashes: map[string][]byte{filepath.ToSlash(rel): []byte("KNOWN")}, - fullHashRepos: map[string]struct{}{"": {}}, - excludedFiles: map[string]struct{}{}, } sf := &buildpb.SourceFile{ Name: strPtr("//" + filepath.ToSlash(filepath.Dir(rel)) + ":" + filepath.Base(rel)), Location: strPtr(abs + ":1:1"), VisibilityLabel: []string{"//visibility:public"}, // non-default } - got, err := h.HashSourceFile(sf, nil) + got, err := h.HashSourceFile(sf) assert.NoError(t, err, "unexpected err: %v", err) assert.NotEqual(t, string(got), "KNOWN", "expected disk hash, but got known hash") assert.NotEqual(t, []byte{}, got, "expected non-empty disk hash") } -func TestDiskHashHelper_ExternalRepo_UsesExternalRuleHash(t *testing.T) { - // Label in an external repo; not in fullHashRepos → use external rule hash +func TestDiskHashHelper_HashesFileFromDisk(t *testing.T) { + tmp := t.TempDir() + wsRoot := tmp + rel := filepath.Join("pkg", "file.txt") + abs := filepath.Join(wsRoot, rel) + if err := os.MkdirAll(filepath.Dir(abs), 0o755); err != nil { + require.NoError(t, err, "mkdir: %v", err) + } + if err := os.WriteFile(abs, []byte("hello world"), 0o644); err != nil { + require.NoError(t, err, "write: %v", err) + } + h := &diskHashHelper{ - workspaceroot: "/ws", + workspaceroot: wsRoot, knownFileHashes: map[string][]byte{}, - fullHashRepos: map[string]struct{}{}, // empty: not using full hashing for any repo - excludedFiles: map[string]struct{}{}, } sf := &buildpb.SourceFile{ - Name: strPtr("@ext//pkg:lib.go"), - Location: strPtr("/ws/pkg/lib.go:1:1"), + Name: strPtr("//" + filepath.ToSlash(filepath.Dir(rel)) + ":" + filepath.Base(rel)), + Location: strPtr(abs), VisibilityLabel: []string{"//visibility:private"}, } - extKey := externalTargetForRule(sf.GetName()) - external := map[string]Target{ - extKey: {Hash: []byte("EXT-HASH")}, - } - got, err := h.HashSourceFile(sf, external) + got, err := h.HashSourceFile(sf) assert.NoError(t, err, "unexpected err: %v", err) - assert.Equal(t, "EXT-HASH", string(got), "expected EXT-HASH, got %q", string(got)) + assert.NotEmpty(t, got, "expected non-empty hash from disk") } -func TestDiskHashHelper_ExternalRepo_MissingRuleErrors(t *testing.T) { +func TestDiskHashHelper_HashesDirectory(t *testing.T) { + tmp := t.TempDir() + wsRoot := tmp + dirRel := "mydir" + dirAbs := filepath.Join(wsRoot, dirRel) + if err := os.MkdirAll(dirAbs, 0o755); err != nil { + require.NoError(t, err, "mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(dirAbs, "a.txt"), []byte("aaa"), 0o644); err != nil { + require.NoError(t, err, "write a.txt: %v", err) + } + if err := os.WriteFile(filepath.Join(dirAbs, "b.txt"), []byte("bbb"), 0o644); err != nil { + require.NoError(t, err, "write b.txt: %v", err) + } + h := &diskHashHelper{ - workspaceroot: "/ws", + workspaceroot: wsRoot, knownFileHashes: map[string][]byte{}, - fullHashRepos: map[string]struct{}{}, // empty: not using full hashing for any repo - excludedFiles: map[string]struct{}{}, } sf := &buildpb.SourceFile{ - Name: strPtr("@ext//pkg:lib.go"), - Location: strPtr("/ws/pkg/lib.go:1:1"), + Name: strPtr("//:" + dirRel), + Location: strPtr(dirAbs), VisibilityLabel: []string{"//visibility:private"}, } - _, err := h.HashSourceFile(sf, map[string]Target{}) - assert.Error(t, err, "expected error about missing //external rule, got %v", err) - assert.ErrorContains(t, err, "expected '//external:...' rule") + got, err := h.HashSourceFile(sf) + assert.NoError(t, err, "unexpected err: %v", err) + assert.NotEmpty(t, got, "expected non-empty hash for directory") } func strPtr(s string) *string { return &s } diff --git a/go.mod b/go.mod index 405f2a00..6d06f552 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.24.5 require ( github.com/bazelbuild/buildtools v0.0.0-20250930140053-2eb4fccefb52 + github.com/deckarep/golang-set/v2 v2.8.0 github.com/goccy/go-yaml v1.19.1 github.com/gofrs/flock v0.12.1 github.com/gogo/protobuf v1.3.2 diff --git a/go.sum b/go.sum index 3d278fcb..9c8bfa82 100644 --- a/go.sum +++ b/go.sum @@ -23,6 +23,8 @@ github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XL github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/deckarep/golang-set/v2 v2.8.0 h1:swm0rlPCmdWn9mESxKOjWk8hXSqoxOp+ZlfuyaAdFlQ= +github.com/deckarep/golang-set/v2 v2.8.0/go.mod h1:VAky9rY/yGXJOLEDv3OMci+7wtDpOF4IN+y82NBOac4= github.com/fatih/structtag v1.2.0 h1:/OdNE99OxoI/PqaW/SuSK9uxxT3f/tcSZgon/ssNSx4= github.com/fatih/structtag v1.2.0/go.mod h1:mBJUNpUnHmRKrKlQQlmCrh5PuhftFbNv8Ys4/aAZl94= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= diff --git a/orchestrator/native_orchestrator_test.go b/orchestrator/native_orchestrator_test.go index 3fd45b66..9921d9cd 100644 --- a/orchestrator/native_orchestrator_test.go +++ b/orchestrator/native_orchestrator_test.go @@ -99,8 +99,8 @@ func TestNative_GetTargetGraph_TreehashNotFound_NoError(t *testing.T) { rm := repomanagermock.NewMockRepoManager(ctrl) rm.EXPECT().Lease(gomock.Any(), gomock.Any()).Return(ws, nil) graphRunner := graphmock.NewMockGraphRunner(ctrl) - graphRunner.EXPECT().Compute(gomock.Any(), gomock.Any()).Return(targethasher.Result{Targets: map[string]targethasher.Target{ - "//:a": { + graphRunner.EXPECT().Compute(gomock.Any(), gomock.Any()).Return(targethasher.Result{Targets: map[string]*targethasher.Target{ + "//:a": &targethasher.Target{ Name: "//:a", RuleType: "go_library", },