Skip to content

Commit

Permalink
Work around bazel7 incompatible targets issue (#82)
Browse files Browse the repository at this point in the history
* Add version range checker

* Use version range checker in existing code

* Filter incompatible targets from query spec

This works around bazelbuild/bazel#21010

We only do this extra filtering when we detect we're running with a
Bazel which suffers from this bug.
  • Loading branch information
illicitonion authored Jan 25, 2024
1 parent f255eef commit 4bb8af0
Show file tree
Hide file tree
Showing 12 changed files with 279 additions and 15 deletions.
19 changes: 19 additions & 0 deletions common/versions/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "versions",
srcs = ["versions.go"],
importpath = "github.com/bazel-contrib/target-determinator/common/versions",
visibility = ["//visibility:public"],
deps = ["@com_github_hashicorp_go_version//:go-version"],
)

go_test(
name = "versions_test",
srcs = ["versions_test.go"],
embed = [":versions"],
deps = [
"@com_github_hashicorp_go_version//:go-version",
"@com_github_stretchr_testify//require",
],
)
31 changes: 31 additions & 0 deletions common/versions/versions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package versions

import (
"fmt"
"strings"

"github.com/hashicorp/go-version"
)

func ReleaseIsInRange(releaseString string, min *version.Version, max *version.Version) (*bool, string) {
releasePrefix := "release "
if !strings.HasPrefix(releaseString, releasePrefix) {
return nil, "Bazel wasn't a released version"
}

bazelVersion, err := version.NewVersion(releaseString[len(releasePrefix):])
if err != nil {
return nil, fmt.Sprintf("Failed to parse Bazel version %q", releaseString)
}
if min != nil && !bazelVersion.GreaterThanOrEqual(min) {
return ptr(false), fmt.Sprintf("Bazel version %s was less than minimum %s", bazelVersion, min.String())
}
if max != nil && !max.GreaterThan(bazelVersion) {
return ptr(false), fmt.Sprintf("Bazel version %s was not less than maximum %s", bazelVersion, max.String())
}
return ptr(true), ""
}

func ptr[V any](v V) *V {
return &v
}
136 changes: 136 additions & 0 deletions common/versions/versions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
package versions

import (
"testing"

"github.com/hashicorp/go-version"
"github.com/stretchr/testify/require"
)

func TestReleaseIsInRange(t *testing.T) {
for name, tc := range map[string]struct {
bazelReleaseString string
min string
max string
wantResult *bool
wantExplanation string
}{
"in_range": {
bazelReleaseString: "release 7.0.0",
min: "6.4.0",
max: "8.0.0",
wantResult: ptr(true),
wantExplanation: "",
},
"at_max": {
bazelReleaseString: "release 7.0.0",
min: "6.4.0",
max: "7.0.0",
wantResult: ptr(false),
wantExplanation: "Bazel version 7.0.0 was not less than maximum 7.0.0",
},
"at_min": {
bazelReleaseString: "release 7.0.0",
min: "7.0.0",
max: "8.0.0",
wantResult: ptr(true),
wantExplanation: "",
},
"above_max": {
bazelReleaseString: "release 7.0.0",
min: "6.4.0",
max: "6.5.0",
wantResult: ptr(false),
wantExplanation: "Bazel version 7.0.0 was not less than maximum 6.5.0",
},
"below_min": {
bazelReleaseString: "release 6.4.0",
min: "7.0.0",
max: "7.1.0",
wantResult: ptr(false),
wantExplanation: "Bazel version 6.4.0 was less than minimum 7.0.0",
},
"no_release_prefix": {
bazelReleaseString: "7.0.0",
min: "6.4.0",
max: "8.0.0",
wantResult: nil,
wantExplanation: "Bazel wasn't a released version",
},
"no_version": {
bazelReleaseString: "release beep",
min: "6.4.0",
max: "8.0.0",
wantResult: nil,
wantExplanation: "Failed to parse Bazel version \"release beep\"",
},
"prerelease_in_range": {
bazelReleaseString: "release 8.0.0-pre.20240101.1",
min: "7.0.0",
max: "8.0.0",
wantResult: ptr(true),
wantExplanation: "",
},
"prerelease_below_range": {
bazelReleaseString: "release 8.0.0-pre.20240101.1",
min: "8.0.0",
max: "8.1.0",
wantResult: ptr(false),
wantExplanation: "Bazel version 8.0.0-pre.20240101.1 was less than minimum 8.0.0",
},
"prerelease_above_range": {
bazelReleaseString: "release 8.0.0-pre.20240101.1",
min: "7.0.0",
max: "7.1.0",
wantResult: ptr(false),
wantExplanation: "Bazel version 8.0.0-pre.20240101.1 was not less than maximum 7.1.0",
},
"above_only_min": {
bazelReleaseString: "release 7.0.0",
min: "6.4.0",
max: "",
wantResult: ptr(true),
wantExplanation: "",
},
"at_only_min": {
bazelReleaseString: "release 6.4.0",
min: "6.4.0",
max: "",
wantResult: ptr(true),
wantExplanation: "",
},
"below_only_max": {
bazelReleaseString: "release 6.4.0",
min: "",
max: "7.0.0",
wantResult: ptr(true),
wantExplanation: "",
},
"at_only_max": {
bazelReleaseString: "release 7.0.0",
min: "",
max: "7.0.0",
wantResult: ptr(false),
wantExplanation: "Bazel version 7.0.0 was not less than maximum 7.0.0",
},
} {
t.Run(name, func(t *testing.T) {
var min *version.Version
if tc.min != "" {
min = version.Must(version.NewVersion(tc.min))
}
var max *version.Version
if tc.max != "" {
max = version.Must(version.NewVersion(tc.max))
}
result, explanation := ReleaseIsInRange(tc.bazelReleaseString, min, max)
if tc.wantResult == nil {
require.Nil(t, result)
} else {
require.NotNil(t, result)
require.Equal(t, *tc.wantResult, *result)
}
require.Equal(t, tc.wantExplanation, explanation)
})
}
}
4 changes: 4 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,18 @@ require (
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/stretchr/testify v1.8.4
github.com/wI2L/jsondiff v0.2.0
google.golang.org/protobuf v1.27.1
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/tidwall/gjson v1.14.0 // indirect
github.com/tidwall/match v1.1.1 // indirect
github.com/tidwall/pretty v1.2.0 // indirect
golang.org/x/sys v0.12.0 // indirect
golang.org/x/tools v0.9.1 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
10 changes: 10 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ github.com/aristanetworks/goarista v0.0.0-20220211174905-526022c8b178 h1:U7Y+d65
github.com/aristanetworks/goarista v0.0.0-20220211174905-526022c8b178/go.mod h1:9zxrD1FatJPUgxIsMwWVrALau7/v1sI1OJETI63r670=
github.com/bazelbuild/bazel-gazelle v0.33.1-0.20231019232305-a957b8358c44 h1:UqVCJ6m6bvESvpIHwynGS2DFZY2S/WtqJFRKl3tRo9Y=
github.com/bazelbuild/bazel-gazelle v0.33.1-0.20231019232305-a957b8358c44/go.mod h1:6BWjSqjc2gr7YfzMRCbkHiJZy5YRxIKj7iLButu58Jk=
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/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk=
github.com/golang/protobuf v1.5.2 h1:ROPKBNFfQgOUMifHyP+KYbvpjbdoFNs+aK7DXlji0Tw=
github.com/google/btree v1.1.2 h1:xf4v41cLI2Z6FxbKm+8Bu+m8ifhj15JuZ9sa0jZCMUU=
Expand All @@ -19,6 +21,10 @@ github.com/otiai10/curr v1.0.0/go.mod h1:LskTG5wDwr8Rs+nNQ+1LlxRjAtTZZjtJW4rMXl6
github.com/otiai10/mint v1.3.0/go.mod h1:F5AjcsTsWUqX+Na9fpHb52P8pcRX2CI6A3ctIT91xUo=
github.com/otiai10/mint v1.3.3 h1:7JgpsBaN0uMkyju4tbYHu0mnM55hNKVYLsXmwr15NQI=
github.com/otiai10/mint v1.3.3/go.mod h1:/yxELlJQ0ufhjUwhshSj+wFjZ78CnZ48/1wtmBH1OTc=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
github.com/tidwall/gjson v1.14.0 h1:6aeJ0bzojgWLa82gDQHcx3S0Lr/O51I9bJ5nv6JFx5w=
github.com/tidwall/gjson v1.14.0/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
github.com/tidwall/match v1.1.1 h1:+Ho715JplO36QYgwN9PGYNhgZvoUSc9X2c80KVTi+GA=
Expand All @@ -35,3 +41,7 @@ golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8T
google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
google.golang.org/protobuf v1.27.1 h1:SnqbnDw1V7RiZcXPx5MEeqPv2s79L9i7BJUlG/+RurQ=
google.golang.org/protobuf v1.27.1/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
1 change: 1 addition & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ go_library(
deps = [
"//common",
"//common/sorted_set",
"//common/versions",
"//third_party/protobuf/bazel/analysis",
"//third_party/protobuf/bazel/build",
"@com_github_aristanetworks_goarista//path",
Expand Down
17 changes: 6 additions & 11 deletions pkg/hash_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"sync"

ss "github.com/bazel-contrib/target-determinator/common/sorted_set"
"github.com/bazel-contrib/target-determinator/common/versions"
"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"
Expand All @@ -40,18 +41,12 @@ func NewTargetHashCache(context map[gazelle_label.Label]map[Configuration]*analy
}

func isConfiguredRuleInputsSupported(releaseString string) bool {
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
isSupportedVersion, explanation := versions.ReleaseIsInRange(releaseString, version.Must(version.NewVersion("7.0.0-pre.20230628.2")), nil)
if isSupportedVersion != nil {
return *isSupportedVersion
}
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")))
log.Printf("%s - assuming cquery does not support configured rule inputs (which is supported from bazel 7), which may lead to over-estimates of affected targets", explanation)
return false
}

// TargetHashCache caches hash computations for targets and files, so that transitive hashes can be
Expand Down
40 changes: 37 additions & 3 deletions pkg/target_determinator.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,19 @@ import (
path2 "path"
"reflect"
"runtime"
"sort"
"strconv"
"strings"
"sync"

"github.com/aristanetworks/goarista/path"
"github.com/bazel-contrib/target-determinator/common"
ss "github.com/bazel-contrib/target-determinator/common/sorted_set"
"github.com/bazel-contrib/target-determinator/common/versions"
"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"
"github.com/hashicorp/go-version"
"google.golang.org/protobuf/proto"
)

Expand Down Expand Up @@ -648,7 +651,25 @@ func doQueryDeps(context *Context, targets TargetsList) (*QueryResults, error) {
return nil, fmt.Errorf("failed to resolve the bazel release: %w", err)
}

// Work around https://github.com/bazelbuild/bazel/issues/21010
var incompatibleTargetsToFilter map[label.Label]bool
hasIncompatibleTargetsBug, explanation := versions.ReleaseIsInRange(bazelRelease, version.Must(version.NewVersion("7.0.0-pre.20230628.2")), nil)
if hasIncompatibleTargetsBug != nil && *hasIncompatibleTargetsBug {
if !context.FilterIncompatibleTargets {
return nil, fmt.Errorf("requested not to filter incompatible targets, but bazel version %s has a bug requiring filtering incompatible targets - see https://github.com/bazelbuild/bazel/issues/21010", bazelRelease)
}
incompatibleTargetsToFilter, err = findCompatibleTargets(context, targets.String(), false)
if err != nil {
return nil, fmt.Errorf("failed to find incompatible targets: %w", err)
}
} else if hasIncompatibleTargetsBug == nil {
log.Printf("Couldn't detect whether current bazel version (%s) suffers from https://github.com/bazelbuild/bazel/issues/21010: %s - assuming it does not", bazelRelease, explanation)
}

depsPattern := fmt.Sprintf("deps(%s)", targets.String())
if len(incompatibleTargetsToFilter) > 0 {
depsPattern += " - " + strings.Join(sortedStringKeys(incompatibleTargetsToFilter), " - ")
}
transitiveResult, err := runToCqueryResult(context, depsPattern, true)
if err != nil {
retErr := fmt.Errorf("failed to cquery %v: %w", depsPattern, err)
Expand Down Expand Up @@ -676,7 +697,7 @@ func doQueryDeps(context *Context, targets TargetsList) (*QueryResults, error) {

var compatibleTargets map[label.Label]bool
if context.FilterIncompatibleTargets {
if compatibleTargets, err = findCompatibleTargets(context, targets.String()); err != nil {
if compatibleTargets, err = findCompatibleTargets(context, targets.String(), true); err != nil {
return nil, fmt.Errorf("failed to find compatible targets: %w", err)
}
}
Expand Down Expand Up @@ -724,6 +745,15 @@ func doQueryDeps(context *Context, targets TargetsList) (*QueryResults, error) {
return queryResults, nil
}

func sortedStringKeys[V any](m map[label.Label]V) []string {
keys := make([]string, 0, len(m))
for key := range m {
keys = append(keys, key.String())
}
sort.Strings(keys)
return keys
}

func runToCqueryResult(context *Context, pattern string, includeTransitions bool) (*analysis.CqueryResult, error) {
log.Printf("Running cquery on %s", pattern)
var stdout bytes.Buffer
Expand Down Expand Up @@ -752,12 +782,16 @@ func runToCqueryResult(context *Context, pattern string, includeTransitions bool
return &result, nil
}

func findCompatibleTargets(context *Context, pattern string) (map[label.Label]bool, error) {
func findCompatibleTargets(context *Context, pattern string, compatibility bool) (map[label.Label]bool, error) {
log.Printf("Finding compatible targets under %s", pattern)
compatibleTargets := make(map[label.Label]bool)

// Add the `or []` to work around https://github.com/bazelbuild/bazel/issues/17749 which was fixed in 6.2.0.
queryFilter := ` if "IncompatiblePlatformProvider" not in (providers(target) or []) else ""`
negation := ""
if compatibility {
negation = "not "
}
queryFilter := fmt.Sprintf(` if "IncompatiblePlatformProvider" %sin (providers(target) or []) else ""`, negation)

// Separate alias and non-alias targets to work around https://github.com/bazelbuild/bazel/issues/18421
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,4 +162,8 @@ public void testChmodFile() {}
@Override
@Ignore("bazel-diff does not filter incompatible targets")
public void incompatibleTargetsAreFiltered() throws Exception {}

@Override
@Ignore("bazel-diff does not filter incompatible targets")
public void incompatibleTargetsAreFiltered_bazelIssue21010() throws Exception {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -199,4 +199,8 @@ public void zeroToOneTarget_native() {}
@Override
@Ignore("bazel-differ does not filter incompatible targets")
public void incompatibleTargetsAreFiltered() throws Exception {}

@Override
@Ignore("bazel-differ does not filter incompatible targets")
public void incompatibleTargetsAreFiltered_bazelIssue21010() throws Exception {}
}
Loading

0 comments on commit 4bb8af0

Please sign in to comment.