From c180dbd4c7f0d1cf089c80941757ccf8be651e0d Mon Sep 17 00:00:00 2001 From: Sasha Smundak Date: Sun, 3 Jul 2022 14:55:58 -0700 Subject: [PATCH] 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 | 26 ++++++ bazel/aquery.go | 148 ++++++++----------------------- bazel/aquery_test.go | 184 ++++++++------------------------------- 3 files changed, 98 insertions(+), 260 deletions(-) diff --git a/android/bazel_handler.go b/android/bazel_handler.go index eff03176d..d1df47988 100644 --- a/android/bazel_handler.go +++ b/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)) } diff --git a/bazel/aquery.go b/bazel/aquery.go index 2853a70bc..ae2b107e1 100644 --- a/bazel/aquery.go +++ b/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 - // Bazel py_binary targets, so there is no Ninja build statements generated for creating it. - // 2) For MANIFEST file, SourceSymlinkManifest action is in aquery output of Bazel py_binary targets, - // 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 + // 1) Drop py3wrapper.sh, just use python binary, the launcher script generated by the + // TemplateExpandAction handles everything necessary to launch a Pythin application. + // 2) ../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 .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 .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 diff --git a/bazel/aquery_test.go b/bazel/aquery_test.go index ba22f37a1..3a2bf0f38 100644 --- a/bazel/aquery_test.go +++ b/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 = ` {