diff --git a/config/config.go b/config/config.go index 6033ce09..fa752057 100644 --- a/config/config.go +++ b/config/config.go @@ -18,6 +18,7 @@ import ( "bytes" "fmt" "os" + "strings" yaml "github.com/goccy/go-yaml" ) @@ -105,6 +106,12 @@ func ParseBytes(yamlBytes []byte) (*Config, error) { if config.Repository[i].BzlmodEnabled == nil { config.Repository[i].BzlmodEnabled = &_bzlmodEnabledDefault } + if err := validateBazelDependencyModeArgs(i, "bazel_extra_args", config.Repository[i].BazelExtraArgs); err != nil { + return nil, err + } + if err := validateBazelDependencyModeArgs(i, "bazel_startup_options", config.Repository[i].BazelStartupOptions); err != nil { + return nil, err + } // Default query_timeout_seconds: 600 seconds (10 minutes). if config.Repository[i].QueryTimeoutSeconds <= 0 { config.Repository[i].QueryTimeoutSeconds = DefaultQueryTimeoutSeconds @@ -113,3 +120,19 @@ func ParseBytes(yamlBytes []byte) (*Config, error) { } return &config, nil } + +func validateBazelDependencyModeArgs(repositoryIndex int, field string, args []string) error { + for _, arg := range args { + flag, _, _ := strings.Cut(arg, "=") + switch flag { + case "--enable_bzlmod", "--noenable_bzlmod", "--enable_workspace", "--noenable_workspace": + return fmt.Errorf( + "repository[%d].%s must not set Bazel dependency mode flag %q; use bzlmod_enabled", + repositoryIndex, + field, + arg, + ) + } + } + return nil +} diff --git a/config/config_test.go b/config/config_test.go index 580d6a17..3553ea66 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -75,6 +75,33 @@ service: assert.Equal(t, 1000000, cfg.Service.MaxMessageBytes) } +func TestParseBytes_RejectsBazelDependencyModeArgs(t *testing.T) { + tests := []struct { + name string + field string + flag string + }{ + {name: "Bzlmod enabled in query args", field: "bazel_extra_args", flag: "--enable_bzlmod=true"}, + {name: "Bzlmod disabled in query args", field: "bazel_extra_args", flag: "--noenable_bzlmod"}, + {name: "WORKSPACE enabled in query args", field: "bazel_extra_args", flag: "--enable_workspace"}, + {name: "WORKSPACE disabled in startup options", field: "bazel_startup_options", flag: "--noenable_workspace"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + yamlStr := ` +repository: + - remote: "https://example.com/repo.git" + ` + tt.field + `: ["` + tt.flag + `"] +service: + max_worker_pool_size: 1 + workspaces_root_path: "/tmp/tango-repo-manager" +` + _, err := ParseBytes([]byte(yamlStr)) + require.Error(t, err) + }) + } +} + func TestParseBytes_StorageValidation(t *testing.T) { tests := []struct { name string diff --git a/config/repository_config.go b/config/repository_config.go index e84e274e..6b824691 100644 --- a/config/repository_config.go +++ b/config/repository_config.go @@ -35,14 +35,16 @@ type RepositoryConfig struct { FullHashRepos []string `yaml:"full_hash_repos"` ExcludedFiles []string `yaml:"excluded_files"` // BzlmodEnabled indicates whether this repository uses Bzlmod for external - // dependency management. Defaults to true if unset. Set to false only for - // repositories still using WORKSPACE. + // dependency management. Tango explicitly disables WORKSPACE when true and + // enables WORKSPACE while disabling Bzlmod when false. Defaults to true if + // unset. BzlmodEnabled *bool `yaml:"bzlmod_enabled"` // BazelCommandPath overrides the Bazel binary path. When empty, Tango // automatically downloads and caches Bazelisk from GitHub. BazelCommandPath string `yaml:"bazel_command_path"` // BazelExtraArgs are extra arguments passed to `bazel query` invocations, // inserted between the `query` subcommand and the query expression. + // Dependency-mode flags are reserved; use BzlmodEnabled instead. BazelExtraArgs []string `yaml:"bazel_extra_args"` // BazelStartupOptions are Bazel startup flags placed before the `query` subcommand (e.g. "--batch"); empty by default. BazelStartupOptions []string `yaml:"bazel_startup_options"` diff --git a/core/bazel/query_test.go b/core/bazel/query_test.go index c5bd8a8c..a14b11ce 100644 --- a/core/bazel/query_test.go +++ b/core/bazel/query_test.go @@ -130,6 +130,45 @@ func TestExecuteQuery_WithStartupOptions(t *testing.T) { }, capturedArgs) } +func TestSetupCommand_ConstructsBazelDependencyModeArguments(t *testing.T) { + tests := []struct { + name string + modeArgs []string + }{ + { + name: "Bzlmod", + modeArgs: []string{"--enable_bzlmod=true", "--enable_workspace=false"}, + }, + { + name: "WORKSPACE", + modeArgs: []string{"--enable_bzlmod=false", "--enable_workspace=true"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var capturedArgs []string + client, err := NewBazelClient(context.Background(), Params{ + BazelCommand: "bazel", + WorkspacePath: "/tmp/test", + EnvVarsMap: map[string]string{}, + Logger: zap.NewNop(), + ExecCommandContext: func(_ context.Context, _ string, arg ...string) commander { + capturedArgs = arg + return nil + }, + }) + require.NoError(t, err) + + client.setupCommand(context.Background(), "//...", []string{"--batch"}, tt.modeArgs...) + + expected := []string{"--batch", "query"} + expected = append(expected, tt.modeArgs...) + expected = append(expected, "--output=streamed_proto", "//...") + assert.Equal(t, expected, capturedArgs) + }) + } +} + func TestExecuteQueryInternal_DrainsStreamsBeforeWait(t *testing.T) { defer goleak.VerifyNone(t) ctrl := gomock.NewController(t) 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.go b/graphrunner/native.go index bb2cde9b..6fe9681c 100644 --- a/graphrunner/native.go +++ b/graphrunner/native.go @@ -43,6 +43,26 @@ type NativeGraphRunnerParams struct { Scope tally.Scope } +type bazelDependencyMode struct { + useBzlmod bool + query string + queryArgs [2]string +} + +func resolveBazelDependencyMode(enabled *bool) bazelDependencyMode { + if enabled == nil || *enabled { + return bazelDependencyMode{ + useBzlmod: true, + query: "deps(//...:all-targets)", + queryArgs: [2]string{"--enable_bzlmod=true", "--enable_workspace=false"}, + } + } + return bazelDependencyMode{ + query: "//external:all-targets + deps(//...:all-targets)", + queryArgs: [2]string{"--enable_bzlmod=false", "--enable_workspace=true"}, + } +} + // graph runner takes in a bazel query request and computes the graph func NewNativeGraphRunner(p NativeGraphRunnerParams) GraphRunner { return &nativeGraphRunner{ @@ -58,21 +78,17 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) op := metrics.Begin(g.emitter, _opCompute, metrics.SlowDurationBuckets) defer func() { op.Complete(retErr) }() - bzlmodEnabled := g.config.BzlmodEnabled == nil || *g.config.BzlmodEnabled - query := "deps(//...:all-targets)" - if !bzlmodEnabled { - // //external is only queryable under legacy WORKSPACE resolution; - // Bzlmod repos resolve external deps under @@ instead. - query = "//external:all-targets + " + query - } - additionalArgs := append( - []string{"--order_output=no", "--proto:locations", "--noproto:default_values"}, - g.config.BazelExtraArgs..., - ) + mode := resolveBazelDependencyMode(g.config.BzlmodEnabled) + additionalArgs := make([]string, 0, 3+len(g.config.BazelExtraArgs)+len(mode.queryArgs)) + additionalArgs = append(additionalArgs, "--order_output=no", "--proto:locations", "--noproto:default_values") + additionalArgs = append(additionalArgs, g.config.BazelExtraArgs...) + // Keep Tango's selected dependency mode last so programmatically-created + // configs cannot override it with BazelExtraArgs. + additionalArgs = append(additionalArgs, mode.queryArgs[:]...) bazelStart := time.Now() queryResult, err := g.bazel.ExecuteQuery(ctx, &bazel.QueryRequest{ - Query: query, + Query: mode.query, StartupOptions: g.config.BazelStartupOptions, // --order_output=no will make Bazel execute query faster // --proto:locations: we need to get external file location to make CTC more accurate @@ -100,7 +116,7 @@ func (g *nativeGraphRunner) Compute(ctx context.Context, ws workspace.Workspace) KnownSourceHashes: knownSourceHashes, FullHashRepos: g.config.FullHashRepos, ExcludedRegex: excludedRegex, - UseBzlmod: bzlmodEnabled, + UseBzlmod: mode.useBzlmod, AllTargetsFiles: g.config.AllTargetsFiles, } diff --git a/graphrunner/native_test.go b/graphrunner/native_test.go index 363c19c1..234e430b 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" @@ -62,6 +63,105 @@ func TestCompute_CallsBazelAndReturnsResult(t *testing.T) { assert.Equal(t, ruleClass, res.Targets[ruleName].Rule.GetRuleClass()) } +func TestCompute_ConfiguresBazelDependencyMode(t *testing.T) { + enabled := true + disabled := false + tests := []struct { + name string + bzlmodEnabled *bool + expectedMode bazelDependencyMode + expectedQuery string + expectedArgs []string + }{ + { + name: "defaults to Bzlmod", + expectedMode: bazelDependencyMode{ + useBzlmod: true, + query: "deps(//...:all-targets)", + queryArgs: [2]string{"--enable_bzlmod=true", "--enable_workspace=false"}, + }, + expectedQuery: "deps(//...:all-targets)", + expectedArgs: []string{ + "--order_output=no", + "--proto:locations", + "--noproto:default_values", + "--keep_going", + "--enable_bzlmod=true", + "--enable_workspace=false", + }, + }, + { + name: "explicitly selects Bzlmod", + bzlmodEnabled: &enabled, + expectedMode: bazelDependencyMode{ + useBzlmod: true, + query: "deps(//...:all-targets)", + queryArgs: [2]string{"--enable_bzlmod=true", "--enable_workspace=false"}, + }, + expectedQuery: "deps(//...:all-targets)", + expectedArgs: []string{ + "--order_output=no", + "--proto:locations", + "--noproto:default_values", + "--keep_going", + "--enable_bzlmod=true", + "--enable_workspace=false", + }, + }, + { + name: "explicitly selects WORKSPACE", + bzlmodEnabled: &disabled, + expectedMode: bazelDependencyMode{ + query: "//external:all-targets + deps(//...:all-targets)", + queryArgs: [2]string{"--enable_bzlmod=false", "--enable_workspace=true"}, + }, + expectedQuery: "//external:all-targets + deps(//...:all-targets)", + expectedArgs: []string{ + "--order_output=no", + "--proto:locations", + "--noproto:default_values", + "--keep_going", + "--enable_bzlmod=false", + "--enable_workspace=true", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expectedMode, resolveBazelDependencyMode(tt.bzlmodEnabled)) + + ctrl := gomock.NewController(t) + bazelMock := bazelmock.NewMockBazel(ctrl) + gitMock := gitmock.NewMockInterface(ctrl) + gitMock.EXPECT().FileHashes(gomock.Any(), "HEAD").Return(map[string][]byte{}, nil) + bazelMock.EXPECT().ExecuteQuery(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, req *bazel.QueryRequest) (*bazel.QueryResponse, error) { + assert.Equal(t, tt.expectedQuery, req.Query) + assert.Equal(t, []string{"--batch"}, req.StartupOptions) + assert.Equal(t, tt.expectedArgs, req.AdditionalArgs) + return &bazel.QueryResponse{Result: &buildpb.QueryResult{}}, nil + }, + ) + + gr := NewNativeGraphRunner(NativeGraphRunnerParams{ + BazelClient: bazelMock, + GitClient: gitMock, + Config: config.RepositoryConfig{ + BzlmodEnabled: tt.bzlmodEnabled, + BazelExtraArgs: []string{"--keep_going"}, + BazelStartupOptions: []string{"--batch"}, + }, + }) + ws := workspace.NewWorkspace(workspace.WorkspaceParams{Path: t.TempDir()}) + + res, err := gr.Compute(context.Background(), ws) + require.NoError(t, err) + assert.Empty(t, res.Targets) + }) + } +} + func TestCompute_PropagatesError(t *testing.T) { ctrl := gomock.NewController(t) bazelMock := bazelmock.NewMockBazel(ctrl)