Handle SymlinkTree action, ignore PythonZipper action.

Introduce bazelBuildRunfiles to build runfiles symlink tree, allowing to
ignore a bogus PythonZipper action.

Bug: 232085015
Test: treehugger
Change-Id: I81267f523d8237fddbc7d65955cdd08ea6369046

android/bazel_handler.go

@@ -40,6 +40,13 @@ var (
 		Rspfile:        "${out}.rsp",
 		RspfileContent: "${content}",
 	}, "content")
+	_                 = pctx.HostBinToolVariable("bazelBuildRunfilesTool", "build-runfiles")
+	buildRunfilesRule = pctx.AndroidStaticRule("bazelBuildRunfiles", blueprint.RuleParams{
+		Command:     "${bazelBuildRunfilesTool} ${in} ${outDir}",
+		Depfile:     "",
+		Description: "",
+		CommandDeps: []string{"${bazelBuildRunfilesTool}"},
+	}, "outDir")
 )
 
 func init() {
@@ -910,6 +917,25 @@ func (c *bazelSingleton) GenerateBuildActions(ctx SingletonContext) {
 					"content": escaped,
 				},
 			})
+		} else if buildStatement.Mnemonic == "SymlinkTree" {
+			// build-runfiles arguments are the manifest file and the target directory
+			// where it creates the symlink tree according to this manifest (and then
+			// writes the MANIFEST file to it).
+			outManifest := PathForBazelOut(ctx, buildStatement.OutputPaths[0])
+			outManifestPath := outManifest.String()
+			if !strings.HasSuffix(outManifestPath, "MANIFEST") {
+				panic("the base name of the symlink tree action should be MANIFEST, got " + outManifestPath)
+			}
+			outDir := filepath.Dir(outManifestPath)
+			ctx.Build(pctx, BuildParams{
+				Rule:        buildRunfilesRule,
+				Output:      outManifest,
+				Inputs:      []Path{PathForBazelOut(ctx, buildStatement.InputPaths[0])},
+				Description: "symlink tree for " + outDir,
+				Args: map[string]string{
+					"outDir": outDir,
+				},
+			})
 		} else {
 			panic(fmt.Sprintf("unhandled build statement: %v", buildStatement))
 		}

bazel/aquery.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"path/filepath"
 	"reflect"
-	"regexp"
 	"sort"
 	"strings"
 
@@ -141,9 +140,6 @@ var templateActionOverriddenTokens = map[string]string{
 	"%python_binary%": "python3",
 }
 
-// This pattern matches the MANIFEST file created for a py_binary target.
-var manifestFilePattern = regexp.MustCompile(".*/.+\\.runfiles/MANIFEST$")
-
 // The file name of py3wrapper.sh, which is used by py_binary targets.
 const py3wrapperFileName = "/py3wrapper.sh"
 
@@ -227,20 +223,12 @@ func (a *aqueryArtifactHandler) populateDepsetMaps(depset depSetOfFiles, middlem
 			// Swap middleman artifacts with their corresponding depsets and drop the middleman artifacts.
 			transitiveDepsetIds = append(transitiveDepsetIds, depsetsToUse...)
 		} else if strings.HasSuffix(path, py3wrapperFileName) ||
-			manifestFilePattern.MatchString(path) ||
 			strings.HasPrefix(path, "../bazel_tools") {
 			// Drop these artifacts.
 			// See go/python-binary-host-mixed-build for more details.
-			// 1) For py3wrapper.sh, there is no action for creating py3wrapper.sh in the aquery output of
+			// 1) Drop py3wrapper.sh, just use python binary, the launcher script generated by the
-			// Bazel py_binary targets, so there is no Ninja build statements generated for creating it.
+			// TemplateExpandAction handles everything necessary to launch a Pythin application.
-			// 2) For MANIFEST file, SourceSymlinkManifest action is in aquery output of Bazel py_binary targets,
+			// 2) ../bazel_tools: they have MODIFY timestamp 10years in the future and would cause the
-			// but it doesn't contain sufficient information so no Ninja build statements are generated
-			// for creating it.
-			// So in mixed build mode, when these two are used as input of some Ninja build statement,
-			// since there is no build statement to create them, they should be removed from input paths.
-			// TODO(b/197135294): Clean up this custom runfiles handling logic when
-			// SourceSymlinkManifest and SymlinkTree actions are supported.
-			// 3) ../bazel_tools: they have MODIFY timestamp 10years in the future and would cause the
 			// containing depset to always be considered newer than their outputs.
 		} else {
 			directArtifactPaths = append(directArtifactPaths, path)
@@ -352,10 +340,10 @@ func AqueryBuildStatements(aqueryJsonProto []byte) ([]BuildStatement, []AqueryDe
 			buildStatement, err = aqueryHandler.symlinkActionBuildStatement(actionEntry)
 		} else if actionEntry.isTemplateExpandAction() && len(actionEntry.Arguments) < 1 {
 			buildStatement, err = aqueryHandler.templateExpandActionBuildStatement(actionEntry)
-		} else if actionEntry.isPythonZipperAction() {
-			buildStatement, err = aqueryHandler.pythonZipperActionBuildStatement(actionEntry, buildStatements)
 		} else if actionEntry.isFileWriteAction() {
 			buildStatement, err = aqueryHandler.fileWriteActionBuildStatement(actionEntry)
+		} else if actionEntry.isSymlinkTreeAction() {
+			buildStatement, err = aqueryHandler.symlinkTreeActionBuildStatement(actionEntry)
 		} else if len(actionEntry.Arguments) < 1 {
 			return nil, nil, fmt.Errorf("received action with no command: [%s]", actionEntry.Mnemonic)
 		} else {
@@ -456,54 +444,6 @@ func (a *aqueryArtifactHandler) normalActionBuildStatement(actionEntry action) (
 	return buildStatement, nil
 }
 
-func (a *aqueryArtifactHandler) pythonZipperActionBuildStatement(actionEntry action, prevBuildStatements []BuildStatement) (BuildStatement, error) {
-	inputPaths, err := a.getInputPaths(actionEntry.InputDepSetIds)
-	if err != nil {
-		return BuildStatement{}, err
-	}
-	outputPaths, depfile, err := a.getOutputPaths(actionEntry)
-	if err != nil {
-		return BuildStatement{}, err
-	}
-
-	if len(inputPaths) < 1 || len(outputPaths) != 1 {
-		return BuildStatement{}, fmt.Errorf("Expect 1+ input and 1 output to python zipper action, got: input %q, output %q", inputPaths, outputPaths)
-	}
-	command := strings.Join(proptools.ShellEscapeListIncludingSpaces(actionEntry.Arguments), " ")
-	inputPaths, command = removePy3wrapperScript(inputPaths, command)
-	command = addCommandForPyBinaryRunfilesDir(command, outputPaths[0])
-	// Add the python zip file as input of the corresponding python binary stub script in Ninja build statements.
-	// In Ninja build statements, the outputs of dependents of a python binary have python binary stub script as input,
-	// which is not sufficient without the python zip file from which runfiles directory is created for py_binary.
-	//
-	// The following logic relies on that Bazel aquery output returns actions in the order that
-	// PythonZipper is after TemplateAction of creating Python binary stub script. If later Bazel doesn't return actions
-	// in that order, the following logic might not find the build statement generated for Python binary
-	// stub script and the build might fail. So the check of pyBinaryFound is added to help debug in case later Bazel might change aquery output.
-	// See go/python-binary-host-mixed-build for more details.
-	pythonZipFilePath := outputPaths[0]
-	pyBinaryFound := false
-	for i := range prevBuildStatements {
-		if len(prevBuildStatements[i].OutputPaths) == 1 && prevBuildStatements[i].OutputPaths[0]+".zip" == pythonZipFilePath {
-			prevBuildStatements[i].InputPaths = append(prevBuildStatements[i].InputPaths, pythonZipFilePath)
-			pyBinaryFound = true
-		}
-	}
-	if !pyBinaryFound {
-		return BuildStatement{}, fmt.Errorf("Could not find the correspondinging Python binary stub script of PythonZipper: %q", outputPaths)
-	}
-
-	buildStatement := BuildStatement{
-		Command:     command,
-		Depfile:     depfile,
-		OutputPaths: outputPaths,
-		InputPaths:  inputPaths,
-		Env:         actionEntry.EnvironmentVariables,
-		Mnemonic:    actionEntry.Mnemonic,
-	}
-	return buildStatement, nil
-}
-
 func (a *aqueryArtifactHandler) templateExpandActionBuildStatement(actionEntry action) (BuildStatement, error) {
 	outputPaths, depfile, err := a.getOutputPaths(actionEntry)
 	if err != nil {
@@ -555,6 +495,28 @@ func (a *aqueryArtifactHandler) fileWriteActionBuildStatement(actionEntry action
 	}, nil
 }
 
+func (a *aqueryArtifactHandler) symlinkTreeActionBuildStatement(actionEntry action) (BuildStatement, error) {
+	outputPaths, _, err := a.getOutputPaths(actionEntry)
+	if err != nil {
+		return BuildStatement{}, err
+	}
+	inputPaths, err := a.getInputPaths(actionEntry.InputDepSetIds)
+	if err != nil {
+		return BuildStatement{}, err
+	}
+	if len(inputPaths) != 1 || len(outputPaths) != 1 {
+		return BuildStatement{}, fmt.Errorf("Expect 1 input and 1 output to symlink action, got: input %q, output %q", inputPaths, outputPaths)
+	}
+	// The actual command is generated in bazelSingleton.GenerateBuildActions
+	return BuildStatement{
+		Depfile:     nil,
+		OutputPaths: outputPaths,
+		Env:         actionEntry.EnvironmentVariables,
+		Mnemonic:    actionEntry.Mnemonic,
+		InputPaths:  inputPaths,
+	}, nil
+}
+
 func (a *aqueryArtifactHandler) symlinkActionBuildStatement(actionEntry action) (BuildStatement, error) {
 	outputPaths, depfile, err := a.getOutputPaths(actionEntry)
 	if err != nil {
@@ -637,46 +599,6 @@ func escapeCommandlineArgument(str string) string {
 	return replacer.Replace(str)
 }
 
-// removePy3wrapperScript removes py3wrapper.sh from the input paths and command of the action of
-// creating python zip file in mixed build mode. py3wrapper.sh is returned as input by aquery but
-// there is no action returned by aquery for creating it. So in mixed build "python3" is used
-// as the PYTHON_BINARY in python binary stub script, and py3wrapper.sh is not needed and should be
-// removed from input paths and command of creating python zip file.
-// See go/python-binary-host-mixed-build for more details.
-// TODO(b/205879240) remove this after py3wrapper.sh could be created in the mixed build mode.
-func removePy3wrapperScript(inputPaths []string, command string) (newInputPaths []string, newCommand string) {
-	// Remove from inputs
-	var filteredInputPaths []string
-	for _, path := range inputPaths {
-		if !strings.HasSuffix(path, py3wrapperFileName) {
-			filteredInputPaths = append(filteredInputPaths, path)
-		}
-	}
-	newInputPaths = filteredInputPaths
-
-	// Remove from command line
-	var re = regexp.MustCompile(`\S*` + py3wrapperFileName)
-	newCommand = re.ReplaceAllString(command, "")
-	return
-}
-
-// addCommandForPyBinaryRunfilesDir adds commands creating python binary runfiles directory.
-// runfiles directory is created by using MANIFEST file and MANIFEST file is the output of
-// SourceSymlinkManifest action is in aquery output of Bazel py_binary targets,
-// but since SourceSymlinkManifest doesn't contain sufficient information
-// so MANIFEST file could not be created, which also blocks the creation of runfiles directory.
-// See go/python-binary-host-mixed-build for more details.
-// TODO(b/197135294) create runfiles directory from MANIFEST file once it can be created from SourceSymlinkManifest action.
-func addCommandForPyBinaryRunfilesDir(oldCommand string, zipFilePath string) string {
-	// Unzip the zip file, zipFilePath looks like <python_binary>.zip
-	runfilesDirName := zipFilePath[0:len(zipFilePath)-4] + ".runfiles"
-	command := fmt.Sprintf("%s x %s -d %s", "../bazel_tools/tools/zip/zipper/zipper", zipFilePath, runfilesDirName)
-	// Create a symbolic link in <python_binary>.runfiles/, which is the expected structure
-	// when running the python binary stub script.
-	command += fmt.Sprintf(" && ln -sf runfiles/__main__ %s", runfilesDirName)
-	return oldCommand + " && " + command
-}
-
 func (a action) isSymlinkAction() bool {
 	return a.Mnemonic == "Symlink" || a.Mnemonic == "SolibSymlink" || a.Mnemonic == "ExecutableSymlink"
 }
@@ -685,25 +607,25 @@ func (a action) isTemplateExpandAction() bool {
 	return a.Mnemonic == "TemplateExpand"
 }
 
-func (a action) isPythonZipperAction() bool {
-	return a.Mnemonic == "PythonZipper"
-}
-
 func (a action) isFileWriteAction() bool {
 	return a.Mnemonic == "FileWrite" || a.Mnemonic == "SourceSymlinkManifest"
 }
 
+func (a action) isSymlinkTreeAction() bool {
+	return a.Mnemonic == "SymlinkTree"
+}
+
 func shouldSkipAction(a action) bool {
-	// TODO(b/180945121): Handle complex symlink actions.
-	if a.Mnemonic == "SymlinkTree" {
-		return true
-	}
 	// Middleman actions are not handled like other actions; they are handled separately as a
 	// preparatory step so that their inputs may be relayed to actions depending on middleman
 	// artifacts.
 	if a.Mnemonic == "Middleman" {
 		return true
 	}
+	// PythonZipper is bogus action returned by aquery, ignore it (b/236198693)
+	if a.Mnemonic == "PythonZipper" {
+		return true
+	}
 	// Skip "Fail" actions, which are placeholder actions designed to always fail.
 	if a.Mnemonic == "Fail" {
 		return true

bazel/aquery_test.go

@@ -460,6 +460,43 @@ func TestTransitiveInputDepsets(t *testing.T) {
 	}
 }
 
+func TestSymlinkTree(t *testing.T) {
+	const inputString = `
+{
+  "artifacts": [
+    { "id": 1, "pathFragmentId": 1 },
+    { "id": 2, "pathFragmentId": 2 }],
+  "actions": [{
+    "targetId": 1,
+    "actionKey": "x",
+    "mnemonic": "SymlinkTree",
+    "configurationId": 1,
+    "inputDepSetIds": [1],
+    "outputIds": [2],
+    "primaryOutputId": 2,
+    "executionPlatform": "//build/bazel/platforms:linux_x86_64"
+  }],
+  "pathFragments": [
+    { "id": 1, "label": "foo.manifest" },
+    { "id": 2, "label": "foo.runfiles/MANIFEST" }],
+  "depSetOfFiles": [
+    { "id": 1, "directArtifactIds": [1] }]
+}
+`
+	actual, _, err := AqueryBuildStatements([]byte(inputString))
+	if err != nil {
+		t.Errorf("Unexpected error %q", err)
+	}
+	assertBuildStatements(t, []BuildStatement{
+		{
+			Command:     "",
+			OutputPaths: []string{"foo.runfiles/MANIFEST"},
+			Mnemonic:    "SymlinkTree",
+			InputPaths:  []string{"foo.manifest"},
+		},
+	}, actual)
+}
+
 func TestBazelOutRemovalFromInputDepsets(t *testing.T) {
 	const inputString = `{
   "artifacts": [{
@@ -861,153 +898,6 @@ func TestTemplateExpandActionNoOutput(t *testing.T) {
 	assertError(t, err, `Expect 1 output to template expand action, got: output []`)
 }
 
-func TestPythonZipperActionSuccess(t *testing.T) {
-	const inputString = `
-{
-  "artifacts": [
-    { "id": 1, "pathFragmentId": 1 },
-    { "id": 2, "pathFragmentId": 2 },
-    { "id": 3, "pathFragmentId": 3 },
-    { "id": 4, "pathFragmentId": 4 },
-    { "id": 5, "pathFragmentId": 10 },
-    { "id": 10, "pathFragmentId": 20 }],
-  "actions": [{
-    "targetId": 1,
-    "actionKey": "x",
-    "mnemonic": "TemplateExpand",
-    "configurationId": 1,
-    "outputIds": [1],
-    "primaryOutputId": 1,
-    "executionPlatform": "//build/bazel/platforms:linux_x86_64",
-    "templateContent": "Test template substitutions: %token1%, %python_binary%",
-    "substitutions": [{
-      "key": "%token1%",
-      "value": "abcd"
-    },{
-      "key": "%python_binary%",
-      "value": "python3"
-    }]
-  },{
-    "targetId": 1,
-    "actionKey": "x",
-    "mnemonic": "PythonZipper",
-    "configurationId": 1,
-    "arguments": ["../bazel_tools/tools/zip/zipper/zipper", "cC", "python_binary.zip", "__main__.py\u003dbazel-out/k8-fastbuild/bin/python_binary.temp", "__init__.py\u003d", "runfiles/__main__/__init__.py\u003d", "runfiles/__main__/python_binary.py\u003dpython_binary.py", "runfiles/bazel_tools/tools/python/py3wrapper.sh\u003dbazel-out/bazel_tools/k8-fastbuild/bin/tools/python/py3wrapper.sh"],
-    "outputIds": [2],
-    "inputDepSetIds": [1],
-    "primaryOutputId": 2
-  }],
-  "depSetOfFiles": [
-    { "id": 1, "directArtifactIds": [4, 3, 5] }],
-  "pathFragments": [
-    { "id": 1, "label": "python_binary" },
-    { "id": 2, "label": "python_binary.zip" },
-    { "id": 3, "label": "python_binary.py" },
-    { "id": 9, "label": ".." },
-    { "id": 8, "label": "bazel_tools", "parentId": 9 },
-    { "id": 7, "label": "tools", "parentId": 8 },
-    { "id": 6, "label": "zip", "parentId": 7  },
-    { "id": 5, "label": "zipper", "parentId": 6 },
-    { "id": 4, "label": "zipper", "parentId": 5 },
-    { "id": 16, "label": "bazel-out" },
-    { "id": 15, "label": "bazel_tools", "parentId": 16 },
-    { "id": 14, "label": "k8-fastbuild", "parentId": 15 },
-    { "id": 13, "label": "bin", "parentId": 14 },
-    { "id": 12, "label": "tools", "parentId": 13 },
-    { "id": 11, "label": "python", "parentId": 12 },
-    { "id": 10, "label": "py3wrapper.sh", "parentId": 11 },
-    { "id": 20, "label": "python_binary" }]
-}`
-	actual, _, err := AqueryBuildStatements([]byte(inputString))
-
-	if err != nil {
-		t.Errorf("Unexpected error %q", err)
-	}
-
-	expectedBuildStatements := []BuildStatement{
-		{
-			Command: "/bin/bash -c 'echo \"Test template substitutions: abcd, python3\" | sed \"s/\\\\\\\\n/\\\\n/g\" > python_binary && " +
-				"chmod a+x python_binary'",
-			InputPaths:  []string{"python_binary.zip"},
-			OutputPaths: []string{"python_binary"},
-			Mnemonic:    "TemplateExpand",
-		},
-		{
-			Command: "../bazel_tools/tools/zip/zipper/zipper cC python_binary.zip __main__.py=bazel-out/k8-fastbuild/bin/python_binary.temp " +
-				"__init__.py= runfiles/__main__/__init__.py= runfiles/__main__/python_binary.py=python_binary.py  && " +
-				"../bazel_tools/tools/zip/zipper/zipper x python_binary.zip -d python_binary.runfiles && ln -sf runfiles/__main__ python_binary.runfiles",
-			InputPaths:  []string{"python_binary.py"},
-			OutputPaths: []string{"python_binary.zip"},
-			Mnemonic:    "PythonZipper",
-		},
-	}
-	assertBuildStatements(t, expectedBuildStatements, actual)
-}
-
-func TestPythonZipperActionNoInput(t *testing.T) {
-	const inputString = `
-{
-  "artifacts": [
-    { "id": 1, "pathFragmentId": 1 },
-    { "id": 2, "pathFragmentId": 2 }],
-  "actions": [{
-    "targetId": 1,
-    "actionKey": "x",
-    "mnemonic": "PythonZipper",
-    "configurationId": 1,
-    "arguments": ["../bazel_tools/tools/zip/zipper/zipper", "cC", "python_binary.zip", "__main__.py\u003dbazel-out/k8-fastbuild/bin/python_binary.temp", "__init__.py\u003d", "runfiles/__main__/__init__.py\u003d", "runfiles/__main__/python_binary.py\u003dpython_binary.py", "runfiles/bazel_tools/tools/python/py3wrapper.sh\u003dbazel-out/bazel_tools/k8-fastbuild/bin/tools/python/py3wrapper.sh"],
-    "outputIds": [2],
-    "primaryOutputId": 2
-  }],
-  "pathFragments": [
-    { "id": 1, "label": "python_binary" },
-    { "id": 2, "label": "python_binary.zip" }]
-}`
-	_, _, err := AqueryBuildStatements([]byte(inputString))
-	assertError(t, err, `Expect 1+ input and 1 output to python zipper action, got: input [], output ["python_binary.zip"]`)
-}
-
-func TestPythonZipperActionNoOutput(t *testing.T) {
-	const inputString = `
-{
-  "artifacts": [
-    { "id": 1, "pathFragmentId": 1 },
-    { "id": 2, "pathFragmentId": 2 },
-    { "id": 3, "pathFragmentId": 3 },
-    { "id": 4, "pathFragmentId": 4 },
-    { "id": 5, "pathFragmentId": 10 }],
-  "actions": [{
-    "targetId": 1,
-    "actionKey": "x",
-    "mnemonic": "PythonZipper",
-    "configurationId": 1,
-    "arguments": ["../bazel_tools/tools/zip/zipper/zipper", "cC", "python_binary.zip", "__main__.py\u003dbazel-out/k8-fastbuild/bin/python_binary.temp", "__init__.py\u003d", "runfiles/__main__/__init__.py\u003d", "runfiles/__main__/python_binary.py\u003dpython_binary.py", "runfiles/bazel_tools/tools/python/py3wrapper.sh\u003dbazel-out/bazel_tools/k8-fastbuild/bin/tools/python/py3wrapper.sh"],
-    "inputDepSetIds": [1]
-  }],
-  "depSetOfFiles": [
-    { "id": 1, "directArtifactIds": [4, 3, 5]}],
-  "pathFragments": [
-    { "id": 1, "label": "python_binary" },
-    { "id": 2, "label": "python_binary.zip" },
-    { "id": 3, "label": "python_binary.py" },
-    { "id": 9, "label": ".." },
-    { "id": 8, "label": "bazel_tools", "parentId": 9 },
-    { "id": 7, "label": "tools", "parentId": 8 },
-    { "id": 6, "label": "zip", "parentId": 7 },
-    { "id": 5, "label": "zipper", "parentId": 6 },
-    { "id": 4, "label": "zipper", "parentId": 5 },
-    { "id": 16, "label": "bazel-out" },
-    { "id": 15, "label": "bazel_tools", "parentId": 16 },
-    { "id": 14, "label": "k8-fastbuild", "parentId": 15 },
-    { "id": 13, "label": "bin", "parentId": 14 },
-    { "id": 12, "label": "tools", "parentId": 13 },
-    { "id": 11, "label": "python", "parentId": 12 },
-    { "id": 10, "label": "py3wrapper.sh", "parentId": 11 }]
-}`
-	_, _, err := AqueryBuildStatements([]byte(inputString))
-	assertError(t, err, `Expect 1+ input and 1 output to python zipper action, got: input ["python_binary.py"], output []`)
-}
-
 func TestFileWrite(t *testing.T) {
 	const inputString = `
 {