From cfae1f39cebf17e1a2946c09b1db903beef94406 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 30 Aug 2023 18:40:42 +0100 Subject: [PATCH] Support using configured rule inputs from 7.0.0 prereleases (#74) This allows for more precise dependency analysis as we no longer need to guess configurations (where we over-estimate), and avoids potential deadlocks if we hit dependency cycles. Fixes #72 --- go.mod | 1 + go.sum | 2 + pkg/BUILD.bazel | 1 + pkg/hash_cache.go | 28 ++++++++++- pkg/hash_cache_test.go | 48 ++++++++++++------- .../TargetDeterminatorIntegrationTest.java | 2 + third_party/go/deps.bzl | 6 +++ 7 files changed, 70 insertions(+), 18 deletions(-) diff --git a/go.mod b/go.mod index ab305198..bd8b258f 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/bazelbuild/bazel-gazelle v0.32.0 github.com/google/btree v1.1.2 github.com/google/uuid v1.3.0 + github.com/hashicorp/go-version v1.6.0 github.com/otiai10/copy v1.7.1-0.20211223015809-9aae5f77261f github.com/wI2L/jsondiff v0.2.0 google.golang.org/protobuf v1.27.1 diff --git a/go.sum b/go.sum index 23d13dc3..23c61bed 100644 --- a/go.sum +++ b/go.sum @@ -10,6 +10,8 @@ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/hashicorp/go-version v1.6.0 h1:feTTfFNnjP967rlCxM/I9g701jU+RN74YKx2mOkIeek= +github.com/hashicorp/go-version v1.6.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/otiai10/copy v1.7.1-0.20211223015809-9aae5f77261f h1:P7Ab27T4In6ExIHmjOe88b1BHpuHlr4Vr75hX2QKAXw= github.com/otiai10/copy v1.7.1-0.20211223015809-9aae5f77261f/go.mod h1:rmRl6QPdJj6EiUqXQ/4Nn2lLXoNQjFCQbbNrxgc/t3U= github.com/otiai10/curr v0.0.0-20150429015615-9b4961190c95/go.mod h1:9qAhocn7zKJG+0mI8eUu6xqkFDYS2kb2saOteoSB3cE= diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 34011236..0f6cb1a8 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -19,6 +19,7 @@ go_library( "//third_party/protobuf/bazel/build", "@com_github_aristanetworks_goarista//path", "@com_github_bazelbuild_bazel_gazelle//label", + "@com_github_hashicorp_go_version//:go-version", "@com_github_wi2l_jsondiff//:jsondiff", "@org_golang_google_protobuf//encoding/protojson", "@org_golang_google_protobuf//proto", diff --git a/pkg/hash_cache.go b/pkg/hash_cache.go index e5756427..14a6f4cb 100644 --- a/pkg/hash_cache.go +++ b/pkg/hash_cache.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "log" "os" "path/filepath" "sort" @@ -17,6 +18,7 @@ import ( "github.com/bazel-contrib/target-determinator/third_party/protobuf/bazel/analysis" "github.com/bazel-contrib/target-determinator/third_party/protobuf/bazel/build" gazelle_label "github.com/bazelbuild/bazel-gazelle/label" + "github.com/hashicorp/go-version" "google.golang.org/protobuf/encoding/protojson" "google.golang.org/protobuf/proto" ) @@ -38,8 +40,18 @@ func NewTargetHashCache(context map[gazelle_label.Label]map[Configuration]*analy } func isConfiguredRuleInputsSupported(releaseString string) bool { - // Disabled until https://github.com/bazelbuild/bazel/issues/17916 is resolved - return false + releasePrefix := "release " + explanation := " - assuming cquery does not support configured rule inputs (which is supported from bazel 7), which may lead to over-estimates of affected targets" + if !strings.HasPrefix(releaseString, releasePrefix) { + log.Printf("Bazel wasn't a released version%s", explanation) + return false + } + v, err := version.NewVersion(releaseString[len(releasePrefix):]) + if err != nil { + log.Printf("Failed to parse Bazel version %q: %s", releaseString, explanation) + return false + } + return v.GreaterThanOrEqual(version.Must(version.NewVersion("7.0.0-pre.20230628.2"))) } // TargetHashCache caches hash computations for targets and files, so that transitive hashes can be @@ -492,6 +504,8 @@ func hashRule(thc *TargetHashCache, rule *build.Rule, configuration *analysis.Co hasher.Write(protoBytes) } + ownConfiguration := NormalizeConfiguration(configuration.GetChecksum()) + // Hash rule inputs if thc.bazelVersionSupportsConfiguredRuleInputs { for _, configuredRuleInput := range rule.ConfiguredRuleInput { @@ -500,6 +514,16 @@ func hashRule(thc *TargetHashCache, rule *build.Rule, configuration *analysis.Co return nil, fmt.Errorf("failed to parse configuredRuleInput label %s: %w", configuredRuleInput.GetLabel(), err) } ruleInputConfiguration := NormalizeConfiguration(configuredRuleInput.GetConfigurationChecksum()) + if ruleInputConfiguration.String() == "" { + // Configured Rule Inputs which aren't transitioned end up with an empty string as their configuration. + // This _either_ means there was no transition, _or_ means that the input was a source file (so didn't have a configuration at all). + // Fortunately, these are mutually exclusive - a target either has a configuration or doesn't, so we look up which one exists. + if _, ok := thc.context[ruleInputLabel][ownConfiguration]; ok { + ruleInputConfiguration = ownConfiguration + } else if _, ok := thc.context[ruleInputLabel][ruleInputConfiguration]; !ok { + return nil, fmt.Errorf("configuredRuleInputs for %s included %s in configuration %s but it couldn't be found either unconfigured or in the depending target's configuration %s. This probably indicates a bug in Bazel - please report it with a git repo that reproduces at https://github.com/bazel-contrib/target-determinator/issues so we can investigate", rule.GetName(), ruleInputLabel, ruleInputConfiguration, ownConfiguration) + } + } ruleInputHash, err := thc.Hash(LabelAndConfiguration{Label: ruleInputLabel, Configuration: ruleInputConfiguration}) if err != nil { return nil, fmt.Errorf("failed to hash configuredRuleInput %s %s which is a dependency of %s %s: %w", ruleInputLabel, ruleInputConfiguration, rule.GetName(), configuration.GetChecksum(), err) diff --git a/pkg/hash_cache_test.go b/pkg/hash_cache_test.go index c9ac321a..597bb877 100644 --- a/pkg/hash_cache_test.go +++ b/pkg/hash_cache_test.go @@ -1,4 +1,4 @@ -package pkg_test +package pkg import ( "encoding/hex" @@ -9,7 +9,6 @@ import ( "reflect" "testing" - "github.com/bazel-contrib/target-determinator/pkg" "github.com/bazel-contrib/target-determinator/third_party/protobuf/bazel/analysis" "github.com/bazel-contrib/target-determinator/third_party/protobuf/bazel/build" "github.com/bazelbuild/bazel-gazelle/label" @@ -29,7 +28,7 @@ func TestAbsolutifiesSourceFileInBuildDirBazel4(t *testing.T) { }, } const want = "/some/path/to/java/example/simple/Dep.java" - got := pkg.AbsolutePath(&target) + got := AbsolutePath(&target) if want != got { t.Fatalf("Wrong absolute path: want %v got %v", want, got) } @@ -45,7 +44,7 @@ func TestAbsolutifiesSourceFileInNestedDirBazel4(t *testing.T) { }, } const want = "/some/path/to/java/example/simple/just/a/File.java" - got := pkg.AbsolutePath(&target) + got := AbsolutePath(&target) if want != got { t.Fatalf("Wrong absolute path: want %v got %v", want, got) } @@ -61,7 +60,7 @@ func TestAbsolutifiesSourceFileInBuildDirBazel5(t *testing.T) { }, } const want = "/some/path/to/java/example/simple/Dep.java" - got := pkg.AbsolutePath(&target) + got := AbsolutePath(&target) if want != got { t.Fatalf("Wrong absolute path: want %v got %v", want, got) } @@ -77,7 +76,7 @@ func TestAbsolutifiesSourceFileInNestedDirBazel5(t *testing.T) { }, } const want = "/some/path/to/java/example/simple/just/a/File.java" - got := pkg.AbsolutePath(&target) + got := AbsolutePath(&target) if want != got { t.Fatalf("Wrong absolute path: want %v got %v", want, got) } @@ -95,7 +94,7 @@ func TestAbsolutifiesBuildFile(t *testing.T) { }, } const want = "/some/path/to/BUILD.bazel" - got := pkg.AbsolutePath(&target) + got := AbsolutePath(&target) if want != got { t.Fatalf("Wrong absolute path: want %v got %v", want, got) } @@ -105,7 +104,7 @@ func TestDigestsSingleSourceFile(t *testing.T) { _, cqueryResult := layoutProject(t) thc := parseResult(t, cqueryResult, "release 5.1.1") - hash, err := thc.Hash(pkg.LabelAndConfiguration{ + hash, err := thc.Hash(LabelAndConfiguration{ Label: mustParseLabel("//HelloWorld:HelloWorld.java"), }) if err != nil { @@ -125,7 +124,7 @@ func TestDigestingMissingSourceFileIsNotError(t *testing.T) { _, cqueryResult := layoutProject(t) thc := parseResult(t, cqueryResult, "release 5.1.1") - _, err := thc.Hash(pkg.LabelAndConfiguration{ + _, err := thc.Hash(LabelAndConfiguration{ Label: mustParseLabel("//HelloWorld:ThereIsNoWorld.java"), }) if err != nil { @@ -144,7 +143,7 @@ func TestDigestingDirectoryIsNotError(t *testing.T) { _, cqueryResult := layoutProject(t) thc := parseResult(t, cqueryResult, "release 5.1.1") - _, err := thc.Hash(pkg.LabelAndConfiguration{ + _, err := thc.Hash(LabelAndConfiguration{ Label: mustParseLabel("//HelloWorld:InhabitedPlanets"), }) if err != nil { @@ -158,9 +157,9 @@ func TestDigestTree(t *testing.T) { // v // HelloWorld.java - labelAndConfiguration := pkg.LabelAndConfiguration{ + labelAndConfiguration := LabelAndConfiguration{ Label: mustParseLabel("//HelloWorld:HelloWorld"), - Configuration: pkg.NormalizeConfiguration(configurationChecksum), + Configuration: NormalizeConfiguration(configurationChecksum), } const defaultBazelVersion = "release 5.1.1" @@ -380,12 +379,12 @@ func layoutProject(t *testing.T) (string, *analysis.CqueryResult) { return dir, &cqueryResult } -func parseResult(t *testing.T, result *analysis.CqueryResult, bazelRelease string) *pkg.TargetHashCache { - cqueryResult, err := pkg.ParseCqueryResult(result) +func parseResult(t *testing.T, result *analysis.CqueryResult, bazelRelease string) *TargetHashCache { + cqueryResult, err := ParseCqueryResult(result) if err != nil { t.Fatalf("Failed to parse cquery result: %v", err) } - return pkg.NewTargetHashCache(cqueryResult, bazelRelease) + return NewTargetHashCache(cqueryResult, bazelRelease) } func areHashesEqual(left, right []byte) bool { @@ -393,9 +392,26 @@ func areHashesEqual(left, right []byte) bool { } func mustParseLabel(s string) label.Label { - l, err := pkg.ParseCanonicalLabel(s) + l, err := ParseCanonicalLabel(s) if err != nil { panic(err) } return l } + +func Test_isConfiguredRuleInputsSupported(t *testing.T) { + for version, want := range map[string]bool{ + "release 6.3.1": false, + "release 7.0.0-pre.20230530.3": false, + "release 7.0.0-pre.20230628.2": true, + "release 7.0.0-pre.20230816.3": true, + "release 7.0.0": true, + } { + t.Run(version, func(t *testing.T) { + got := isConfiguredRuleInputsSupported(version) + if want != got { + t.Fatalf("Incorrect isConfiguredRuleInputsSupported: want %v got %v", want, got) + } + }) + } +} diff --git a/tests/integration/java/com/github/bazel_contrib/target_determinator/integration/TargetDeterminatorIntegrationTest.java b/tests/integration/java/com/github/bazel_contrib/target_determinator/integration/TargetDeterminatorIntegrationTest.java index 7e6d2a10..8966db78 100644 --- a/tests/integration/java/com/github/bazel_contrib/target_determinator/integration/TargetDeterminatorIntegrationTest.java +++ b/tests/integration/java/com/github/bazel_contrib/target_determinator/integration/TargetDeterminatorIntegrationTest.java @@ -65,6 +65,7 @@ public void tryImportInBazelrcAffectingJava() throws Exception { super.tryImportInBazelrcAffectingJava(); } + // TODO: Stop ignoring this test when our tests use Bazel 7.0.0 @Override public void addingTargetUsedInHostConfiguration() throws Exception { allowOverBuilds( @@ -108,6 +109,7 @@ public void reducingVisibilityOnDependencyAffectsTarget() throws Exception { } } + // TODO: Stop ignoring this test when our tests use Bazel 7.0.0 @Override public void ignoredPlatformSpecificDepChanged() throws Exception { allowOverBuilds("Platform-specific narrowing is disabled due to https://github.com/bazelbuild/bazel/issues/17916"); diff --git a/third_party/go/deps.bzl b/third_party/go/deps.bzl index 6d51cc75..cd662242 100644 --- a/third_party/go/deps.bzl +++ b/third_party/go/deps.bzl @@ -121,6 +121,12 @@ def go_dependencies(): sum = "h1:cfejS+Tpcp13yd5nYHWDI6qVCny6wyX2Mt5SGur2IGE=", version = "v1.0.2", ) + go_repository( + name = "com_github_hashicorp_go_version", + importpath = "github.com/hashicorp/go-version", + sum = "h1:feTTfFNnjP967rlCxM/I9g701jU+RN74YKx2mOkIeek=", + version = "v1.6.0", + ) go_repository( name = "com_github_influxdata_influxdb1_client", importpath = "github.com/influxdata/influxdb1-client",