Refactor and cleanup of bazel handler

- Creation of a bazel command is independent of test/real implementation
- Use a custom struct instead of cmd.Exec
- Move mock bazel runner to the test

Bug: 270989498
Test: m nothing
Test: USE_PERSISTENT_BAZEL=0 m nothing
Test: Treehugger
Change-Id: Ieec35dad5e21aac644d5b8dc79a92397d42db861

android/bazel_handler.go

@@ -18,7 +18,6 @@ import (
 	"bytes"
 	"fmt"
 	"os"
-	"os/exec"
 	"path"
 	"path/filepath"
 	"runtime"
@@ -30,7 +29,6 @@ import (
 	"android/soong/bazel/cquery"
 	"android/soong/shared"
 	"android/soong/starlark_fmt"
-
 	"github.com/google/blueprint"
 	"github.com/google/blueprint/metrics"
 
@@ -220,8 +218,7 @@ type BazelContext interface {
 }
 
 type bazelRunner interface {
-	createBazelCommand(config Config, paths *bazelPaths, runName bazel.RunName, command bazelCommand, extraFlags ...string) *exec.Cmd
+	issueBazelCommand(cmdRequest bazel.CmdRequest, paths *bazelPaths, eventHandler *metrics.EventHandler) (output string, errorMessage string, error error)
-	issueBazelCommand(bazelCmd *exec.Cmd, eventHandler *metrics.EventHandler) (output string, errorMessage string, error error)
 }
 
 type bazelPaths struct {
@@ -660,36 +657,6 @@ type bazelCommand struct {
 	expression string
 }
 
-type mockBazelRunner struct {
-	bazelCommandResults map[bazelCommand]string
-	// use *exec.Cmd as a key to get the bazelCommand, the map will be used in issueBazelCommand()
-	// Register createBazelCommand() invocations. Later, an
-	// issueBazelCommand() invocation can be mapped to the *exec.Cmd instance
-	// and then to the expected result via bazelCommandResults
-	tokens     map[*exec.Cmd]bazelCommand
-	commands   []bazelCommand
-	extraFlags []string
-}
-
-func (r *mockBazelRunner) createBazelCommand(_ Config, _ *bazelPaths, _ bazel.RunName,
-	command bazelCommand, extraFlags ...string) *exec.Cmd {
-	r.commands = append(r.commands, command)
-	r.extraFlags = append(r.extraFlags, strings.Join(extraFlags, " "))
-	cmd := &exec.Cmd{}
-	if r.tokens == nil {
-		r.tokens = make(map[*exec.Cmd]bazelCommand)
-	}
-	r.tokens[cmd] = command
-	return cmd
-}
-
-func (r *mockBazelRunner) issueBazelCommand(bazelCmd *exec.Cmd, _ *metrics.EventHandler) (string, string, error) {
-	if command, ok := r.tokens[bazelCmd]; ok {
-		return r.bazelCommandResults[command], "", nil
-	}
-	return "", "", nil
-}
-
 type builtinBazelRunner struct {
 	useBazelProxy bool
 	outDir        string
@@ -699,17 +666,12 @@ type builtinBazelRunner struct {
 // Returns (stdout, stderr, error). The first and second return values are strings
 // containing the stdout and stderr of the run command, and an error is returned if
 // the invocation returned an error code.
-func (r *builtinBazelRunner) issueBazelCommand(bazelCmd *exec.Cmd, eventHandler *metrics.EventHandler) (string, string, error) {
+func (r *builtinBazelRunner) issueBazelCommand(cmdRequest bazel.CmdRequest, paths *bazelPaths, eventHandler *metrics.EventHandler) (string, string, error) {
 	if r.useBazelProxy {
 		eventHandler.Begin("client_proxy")
 		defer eventHandler.End("client_proxy")
 		proxyClient := bazel.NewProxyClient(r.outDir)
-		// Omit the arg containing the Bazel binary, as that is handled by the proxy
+		resp, err := proxyClient.IssueCommand(cmdRequest)
-		// server.
-		bazelFlags := bazelCmd.Args[1:]
-		// TODO(b/270989498): Refactor these functions to not take exec.Cmd, as its
-		// not actually executed for client proxying.
-		resp, err := proxyClient.IssueCommand(bazel.CmdRequest{bazelFlags, bazelCmd.Env})
 
 		if err != nil {
 			return "", "", err
@@ -721,26 +683,19 @@ func (r *builtinBazelRunner) issueBazelCommand(bazelCmd *exec.Cmd, eventHandler
 	} else {
 		eventHandler.Begin("bazel command")
 		defer eventHandler.End("bazel command")
-		stderr := &bytes.Buffer{}
+
-		bazelCmd.Stderr = stderr
+		stdout, stderr, err := bazel.ExecBazel(paths.bazelPath, absolutePath(paths.syntheticWorkspaceDir()), cmdRequest)
-		if output, err := bazelCmd.Output(); err != nil {
+		return string(stdout), string(stderr), err
-			return "", string(stderr.Bytes()),
-				fmt.Errorf("bazel command failed: %s\n---command---\n%s\n---env---\n%s\n---stderr---\n%s---",
-					err, bazelCmd, strings.Join(bazelCmd.Env, "\n"), stderr)
-		} else {
-			return string(output), string(stderr.Bytes()), nil
-		}
 	}
 }
 
-func (r *builtinBazelRunner) createBazelCommand(config Config, paths *bazelPaths, runName bazel.RunName, command bazelCommand,
+func (context *mixedBuildBazelContext) createBazelCommand(config Config, runName bazel.RunName, command bazelCommand,
-	extraFlags ...string) *exec.Cmd {
+	extraFlags ...string) bazel.CmdRequest {
 	cmdFlags := []string{
-		"--output_base=" + absolutePath(paths.outputBase),
+		"--output_base=" + absolutePath(context.paths.outputBase),
 		command.command,
 		command.expression,
-		// TODO(asmundak): is it needed in every build?
+		"--profile=" + shared.BazelMetricsFilename(context.paths, runName),
-		"--profile=" + shared.BazelMetricsFilename(paths, runName),
 
 		// We don't need to set --host_platforms because it's set in bazelrc files
 		// that the bazel shell script wrapper passes
@@ -755,15 +710,13 @@ func (r *builtinBazelRunner) createBazelCommand(config Config, paths *bazelPaths
 	}
 	cmdFlags = append(cmdFlags, extraFlags...)
 
-	bazelCmd := exec.Command(paths.bazelPath, cmdFlags...)
-	bazelCmd.Dir = absolutePath(paths.syntheticWorkspaceDir())
 	extraEnv := []string{
-		"HOME=" + paths.homeDir,
+		"HOME=" + context.paths.homeDir,
 		pwdPrefix(),
-		"BUILD_DIR=" + absolutePath(paths.soongOutDir),
+		"BUILD_DIR=" + absolutePath(context.paths.soongOutDir),
 		// Make OUT_DIR absolute here so build/bazel/bin/bazel uses the correct
 		// OUT_DIR at <root>/out, instead of <root>/out/soong/workspace/out.
-		"OUT_DIR=" + absolutePath(paths.outDir()),
+		"OUT_DIR=" + absolutePath(context.paths.outDir()),
 		// Disables local host detection of gcc; toolchain information is defined
 		// explicitly in BUILD files.
 		"BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1",
@@ -775,15 +728,15 @@ func (r *builtinBazelRunner) createBazelCommand(config Config, paths *bazelPaths
 		}
 		extraEnv = append(extraEnv, fmt.Sprintf("%s=%s", envvar, val))
 	}
-	bazelCmd.Env = append(os.Environ(), extraEnv...)
+	envVars := append(os.Environ(), extraEnv...)
 
-	return bazelCmd
+	return bazel.CmdRequest{cmdFlags, envVars}
 }
 
-func printableCqueryCommand(bazelCmd *exec.Cmd) string {
+func (context *mixedBuildBazelContext) printableCqueryCommand(bazelCmd bazel.CmdRequest) string {
-	outputString := strings.Join(bazelCmd.Env, " ") + " \"" + strings.Join(bazelCmd.Args, "\" \"") + "\""
+	args := append([]string{context.paths.bazelPath}, bazelCmd.Argv...)
+	outputString := strings.Join(bazelCmd.Env, " ") + " \"" + strings.Join(args, "\" \"") + "\""
 	return outputString
-
 }
 
 func (context *mixedBuildBazelContext) mainBzlFileContents() []byte {
@@ -1109,6 +1062,7 @@ var (
 	cqueryCmd        = bazelCommand{"cquery", fmt.Sprintf("deps(%s, 2)", buildrootLabel)}
 	aqueryCmd        = bazelCommand{"aquery", fmt.Sprintf("deps(%s)", buildrootLabel)}
 	buildCmd         = bazelCommand{"build", "@soong_injection//mixed_builds:phonyroot"}
+	allBazelCommands = []bazelCommand{aqueryCmd, cqueryCmd, buildCmd}
 )
 
 // Issues commands to Bazel to receive results for all cquery requests
@@ -1170,12 +1124,12 @@ func (context *mixedBuildBazelContext) runCquery(config Config, ctx invokeBazelC
 		extraFlags = append(extraFlags, "--collect_code_coverage")
 	}
 
-	cqueryCommandWithFlag := context.createBazelCommand(config, context.paths, bazel.CqueryBuildRootRunName, cqueryCmd, extraFlags...)
+	cqueryCmdRequest := context.createBazelCommand(config, bazel.CqueryBuildRootRunName, cqueryCmd, extraFlags...)
-	cqueryOutput, cqueryErrorMessage, cqueryErr := context.issueBazelCommand(cqueryCommandWithFlag, eventHandler)
+	cqueryOutput, cqueryErrorMessage, cqueryErr := context.issueBazelCommand(cqueryCmdRequest, context.paths, eventHandler)
 	if cqueryErr != nil {
 		return cqueryErr
 	}
-	cqueryCommandPrint := fmt.Sprintf("cquery command line:\n  %s \n\n\n", printableCqueryCommand(cqueryCommandWithFlag))
+	cqueryCommandPrint := fmt.Sprintf("cquery command line:\n  %s \n\n\n", context.printableCqueryCommand(cqueryCmdRequest))
 	if err := os.WriteFile(filepath.Join(soongInjectionPath, "cquery.out"), []byte(cqueryCommandPrint+cqueryOutput), 0666); err != nil {
 		return err
 	}
@@ -1233,8 +1187,8 @@ func (context *mixedBuildBazelContext) runAquery(config Config, ctx invokeBazelC
 			extraFlags = append(extraFlags, "--instrumentation_filter="+strings.Join(paths, ","))
 		}
 	}
-	aqueryOutput, _, err := context.issueBazelCommand(context.createBazelCommand(config, context.paths, bazel.AqueryBuildRootRunName, aqueryCmd,
+	aqueryOutput, _, err := context.issueBazelCommand(context.createBazelCommand(config, bazel.AqueryBuildRootRunName, aqueryCmd,
-		extraFlags...), eventHandler)
+		extraFlags...), context.paths, eventHandler)
 	if err != nil {
 		return err
 	}
@@ -1249,7 +1203,7 @@ func (context *mixedBuildBazelContext) generateBazelSymlinks(config Config, ctx
 	// Issue a build command of the phony root to generate symlink forests for dependencies of the
 	// Bazel build. This is necessary because aquery invocations do not generate this symlink forest,
 	// but some of symlinks may be required to resolve source dependencies of the build.
-	_, _, err := context.issueBazelCommand(context.createBazelCommand(config, context.paths, bazel.BazelBuildPhonyRootRunName, buildCmd), eventHandler)
+	_, _, err := context.issueBazelCommand(context.createBazelCommand(config, bazel.BazelBuildPhonyRootRunName, buildCmd), context.paths, eventHandler)
 	return err
 }
 

android/bazel_handler_test.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	"android/soong/bazel"
 	"android/soong/bazel/cquery"
 	analysis_v2_proto "prebuilts/bazel/common/proto/analysis_v2"
 
@@ -19,6 +20,34 @@ var testConfig = TestConfig("out", nil, "", nil)
 
 type testInvokeBazelContext struct{}
 
+type mockBazelRunner struct {
+	testHelper *testing.T
+	// Stores mock behavior. If an issueBazelCommand request is made for command
+	// k, and {k:v} is present in this map, then the mock will return v.
+	bazelCommandResults map[bazelCommand]string
+	// Requests actually made of the mockBazelRunner with issueBazelCommand,
+	// keyed by the command they represent.
+	bazelCommandRequests map[bazelCommand]bazel.CmdRequest
+}
+
+func (r *mockBazelRunner) bazelCommandForRequest(cmdRequest bazel.CmdRequest) bazelCommand {
+	for _, arg := range cmdRequest.Argv {
+		for _, cmdType := range allBazelCommands {
+			if arg == cmdType.command {
+				return cmdType
+			}
+		}
+	}
+	r.testHelper.Fatalf("Unrecognized bazel request: %s", cmdRequest)
+	return cqueryCmd
+}
+
+func (r *mockBazelRunner) issueBazelCommand(cmdRequest bazel.CmdRequest, paths *bazelPaths, eventHandler *metrics.EventHandler) (string, string, error) {
+	command := r.bazelCommandForRequest(cmdRequest)
+	r.bazelCommandRequests[command] = cmdRequest
+	return r.bazelCommandResults[command], "", nil
+}
+
 func (t *testInvokeBazelContext) GetEventHandler() *metrics.EventHandler {
 	return &metrics.EventHandler{}
 }
@@ -36,9 +65,7 @@ func TestRequestResultsAfterInvokeBazel(t *testing.T) {
 		`@//foo:foo|arm64_armv8-a|android|within_apex|29>>out/foo/foo.txt`,
 		`@//foo:bar|arm64_armv8-a|android>>out/foo/bar.txt`,
 	}
-	bazelContext, _ := testBazelContext(t, map[bazelCommand]string{
+	bazelContext, _ := testBazelContext(t, map[bazelCommand]string{cqueryCmd: strings.Join(cmd_results, "\n")})
-		bazelCommand{command: "cquery", expression: "deps(@soong_injection//mixed_builds:buildroot, 2)"}: strings.Join(cmd_results, "\n"),
-	})
 
 	bazelContext.QueueBazelRequest(label_foo, cquery.GetOutputFiles, cfg_foo)
 	bazelContext.QueueBazelRequest(label_bar, cquery.GetOutputFiles, cfg_bar)
@@ -139,8 +166,7 @@ func TestInvokeBazelPopulatesBuildStatements(t *testing.T) {
 		if err != nil {
 			t.Error(err)
 		}
-		bazelContext, _ := testBazelContext(t, map[bazelCommand]string{
+		bazelContext, _ := testBazelContext(t, map[bazelCommand]string{aqueryCmd: string(data)})
-			bazelCommand{command: "aquery", expression: "deps(@soong_injection//mixed_builds:buildroot)"}: string(data)})
 
 		err = bazelContext.InvokeBazel(testConfig, &testInvokeBazelContext{})
 		if err != nil {
@@ -166,30 +192,26 @@ func TestCoverageFlagsAfterInvokeBazel(t *testing.T) {
 
 	testConfig.productVariables.NativeCoveragePaths = []string{"foo1", "foo2"}
 	testConfig.productVariables.NativeCoverageExcludePaths = []string{"bar1", "bar2"}
-	verifyExtraFlags(t, testConfig, `--collect_code_coverage --instrumentation_filter=+foo1,+foo2,-bar1,-bar2`)
+	verifyAqueryContainsFlags(t, testConfig, "--collect_code_coverage", "--instrumentation_filter=+foo1,+foo2,-bar1,-bar2")
 
 	testConfig.productVariables.NativeCoveragePaths = []string{"foo1"}
 	testConfig.productVariables.NativeCoverageExcludePaths = []string{"bar1"}
-	verifyExtraFlags(t, testConfig, `--collect_code_coverage --instrumentation_filter=+foo1,-bar1`)
+	verifyAqueryContainsFlags(t, testConfig, "--collect_code_coverage", "--instrumentation_filter=+foo1,-bar1")
 
 	testConfig.productVariables.NativeCoveragePaths = []string{"foo1"}
 	testConfig.productVariables.NativeCoverageExcludePaths = nil
-	verifyExtraFlags(t, testConfig, `--collect_code_coverage --instrumentation_filter=+foo1`)
+	verifyAqueryContainsFlags(t, testConfig, "--collect_code_coverage", "--instrumentation_filter=+foo1")
 
 	testConfig.productVariables.NativeCoveragePaths = nil
 	testConfig.productVariables.NativeCoverageExcludePaths = []string{"bar1"}
-	verifyExtraFlags(t, testConfig, `--collect_code_coverage --instrumentation_filter=-bar1`)
+	verifyAqueryContainsFlags(t, testConfig, "--collect_code_coverage", "--instrumentation_filter=-bar1")
 
 	testConfig.productVariables.NativeCoveragePaths = []string{"*"}
 	testConfig.productVariables.NativeCoverageExcludePaths = nil
-	verifyExtraFlags(t, testConfig, `--collect_code_coverage --instrumentation_filter=+.*`)
+	verifyAqueryContainsFlags(t, testConfig, "--collect_code_coverage", "--instrumentation_filter=+.*")
 
 	testConfig.productVariables.ClangCoverage = boolPtr(false)
-	actual := verifyExtraFlags(t, testConfig, ``)
+	verifyAqueryDoesNotContainSubstrings(t, testConfig, "collect_code_coverage", "instrumentation_filter")
-	if strings.Contains(actual, "--collect_code_coverage") ||
-		strings.Contains(actual, "--instrumentation_filter=") {
-		t.Errorf("Expected code coverage disabled, but got %#v", actual)
-	}
 }
 
 func TestBazelRequestsSorted(t *testing.T) {
@@ -268,7 +290,8 @@ func TestIsModuleNameAllowed(t *testing.T) {
 	}
 }
 
-func verifyExtraFlags(t *testing.T, config Config, expected string) string {
+func verifyAqueryContainsFlags(t *testing.T, config Config, expected ...string) {
+	t.Helper()
 	bazelContext, _ := testBazelContext(t, map[bazelCommand]string{})
 
 	err := bazelContext.InvokeBazel(config, &testInvokeBazelContext{})
@@ -276,17 +299,49 @@ func verifyExtraFlags(t *testing.T, config Config, expected string) string {
 		t.Fatalf("Did not expect error invoking Bazel, but got %s", err)
 	}
 
-	flags := bazelContext.bazelRunner.(*mockBazelRunner).extraFlags
+	sliceContains := func(slice []string, x string) bool {
-	if expected := 3; len(flags) != expected {
+		for _, s := range slice {
-		t.Errorf("Expected %d extra flags got %#v", expected, flags)
+			if s == x {
+				return true
+			}
+		}
+		return false
 	}
 
-	actual := flags[1]
+	aqueryArgv := bazelContext.bazelRunner.(*mockBazelRunner).bazelCommandRequests[aqueryCmd].Argv
-	if !strings.Contains(actual, expected) {
+
-		t.Errorf("Expected %#v got %#v", expected, actual)
+	for _, expectedFlag := range expected {
+		if !sliceContains(aqueryArgv, expectedFlag) {
+			t.Errorf("aquery does not contain expected flag %#v. Argv was: %#v", expectedFlag, aqueryArgv)
+		}
+	}
+}
+
+func verifyAqueryDoesNotContainSubstrings(t *testing.T, config Config, substrings ...string) {
+	t.Helper()
+	bazelContext, _ := testBazelContext(t, map[bazelCommand]string{})
+
+	err := bazelContext.InvokeBazel(config, &testInvokeBazelContext{})
+	if err != nil {
+		t.Fatalf("Did not expect error invoking Bazel, but got %s", err)
 	}
 
-	return actual
+	sliceContainsSubstring := func(slice []string, substring string) bool {
+		for _, s := range slice {
+			if strings.Contains(s, substring) {
+				return true
+			}
+		}
+		return false
+	}
+
+	aqueryArgv := bazelContext.bazelRunner.(*mockBazelRunner).bazelCommandRequests[aqueryCmd].Argv
+
+	for _, substring := range substrings {
+		if sliceContainsSubstring(aqueryArgv, substring) {
+			t.Errorf("aquery contains unexpected substring %#v. Argv was: %#v", substring, aqueryArgv)
+		}
+	}
 }
 
 func testBazelContext(t *testing.T, bazelCommandResults map[bazelCommand]string) (*mixedBuildBazelContext, string) {
@@ -296,11 +351,14 @@ func testBazelContext(t *testing.T, bazelCommandResults map[bazelCommand]string)
 		outputBase:   "outputbase",
 		workspaceDir: "workspace_dir",
 	}
-	aqueryCommand := bazelCommand{command: "aquery", expression: "deps(@soong_injection//mixed_builds:buildroot)"}
+	if _, exists := bazelCommandResults[aqueryCmd]; !exists {
-	if _, exists := bazelCommandResults[aqueryCommand]; !exists {
+		bazelCommandResults[aqueryCmd] = ""
-		bazelCommandResults[aqueryCommand] = ""
+	}
+	runner := &mockBazelRunner{
+		testHelper:           t,
+		bazelCommandResults:  bazelCommandResults,
+		bazelCommandRequests: map[bazelCommand]bazel.CmdRequest{},
 	}
-	runner := &mockBazelRunner{bazelCommandResults: bazelCommandResults}
 	return &mixedBuildBazelContext{
 		bazelRunner: runner,
 		paths:       &p,

bazel/bazel_proxy.go

@@ -128,6 +128,24 @@ func NewProxyServer(logger ServerLogger, outDir string, workspaceDir string) *Pr
 	}
 }
 
+func ExecBazel(bazelPath string, workspaceDir string, request CmdRequest) (stdout []byte, stderr []byte, cmdErr error) {
+	bazelCmd := exec.Command(bazelPath, request.Argv...)
+	bazelCmd.Dir = workspaceDir
+	bazelCmd.Env = request.Env
+
+	stderrBuffer := &bytes.Buffer{}
+	bazelCmd.Stderr = stderrBuffer
+
+	if output, err := bazelCmd.Output(); err != nil {
+		cmdErr = fmt.Errorf("bazel command failed: %s\n---command---\n%s\n---env---\n%s\n---stderr---\n%s---",
+			err, bazelCmd, strings.Join(bazelCmd.Env, "\n"), stderrBuffer)
+	} else {
+		stdout = output
+	}
+	stderr = stderrBuffer.Bytes()
+	return
+}
+
 func (b *ProxyServer) handleRequest(conn net.Conn) error {
 	defer conn.Close()
 
@@ -137,23 +155,13 @@ func (b *ProxyServer) handleRequest(conn net.Conn) error {
 		return fmt.Errorf("Error decoding request: %s", err)
 	}
 
-	bazelCmd := exec.Command("./build/bazel/bin/bazel", req.Argv...)
+	stdout, stderr, cmdErr := ExecBazel("./build/bazel/bin/bazel", b.workspaceDir, req)
-	bazelCmd.Dir = b.workspaceDir
+	errorString := ""
-	bazelCmd.Env = req.Env
+	if cmdErr != nil {
-
+		errorString = cmdErr.Error()
-	stderr := &bytes.Buffer{}
-	bazelCmd.Stderr = stderr
-	var stdout string
-	var bazelErrString string
-
-	if output, err := bazelCmd.Output(); err != nil {
-		bazelErrString = fmt.Sprintf("bazel command failed: %s\n---command---\n%s\n---env---\n%s\n---stderr---\n%s---",
-			err, bazelCmd, strings.Join(bazelCmd.Env, "\n"), stderr)
-	} else {
-		stdout = string(output)
 	}
 
-	resp := CmdResponse{stdout, string(stderr.Bytes()), bazelErrString}
+	resp := CmdResponse{string(stdout), string(stderr), errorString}
 	enc := gob.NewEncoder(conn)
 	if err := enc.Encode(&resp); err != nil {
 		return fmt.Errorf("Error encoding response: %s", err)