From fd708b5651be777b444406aaae02dff3c7a4a143 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 23 Mar 2021 14:16:05 -0700 Subject: [PATCH 1/7] Move response file handling to a separate package sbox is going to need to read and write response files, move ReadRspFile to its own package. Test: response_test.go Change-Id: Iecb5486b4aaeb2531828743ad8ef784df675e18e --- cmd/merge_zips/Android.bp | 2 +- cmd/merge_zips/merge_zips.go | 19 ++++--- response/Android.bp | 16 ++++++ response/response.go | 70 ++++++++++++++++++++++++++ response/response_test.go | 96 ++++++++++++++++++++++++++++++++++++ zip/Android.bp | 1 + zip/cmd/main.go | 12 +++-- zip/zip.go | 52 ++----------------- zip/zip_test.go | 72 --------------------------- 9 files changed, 210 insertions(+), 130 deletions(-) create mode 100644 response/Android.bp create mode 100644 response/response.go create mode 100644 response/response_test.go diff --git a/cmd/merge_zips/Android.bp b/cmd/merge_zips/Android.bp index 930d040f4..c516f99c1 100644 --- a/cmd/merge_zips/Android.bp +++ b/cmd/merge_zips/Android.bp @@ -22,7 +22,7 @@ blueprint_go_binary { "android-archive-zip", "blueprint-pathtools", "soong-jar", - "soong-zip", + "soong-response", ], srcs: [ "merge_zips.go", diff --git a/cmd/merge_zips/merge_zips.go b/cmd/merge_zips/merge_zips.go index 274c8eee5..712c7fccd 100644 --- a/cmd/merge_zips/merge_zips.go +++ b/cmd/merge_zips/merge_zips.go @@ -25,12 +25,14 @@ import ( "os" "path/filepath" "sort" + "strings" + + "android/soong/response" "github.com/google/blueprint/pathtools" "android/soong/jar" "android/soong/third_party/zip" - soongZip "android/soong/zip" ) // Input zip: we can open it, close it, and obtain an array of entries @@ -690,15 +692,20 @@ func main() { inputs := make([]string, 0) for _, input := range args[1:] { if input[0] == '@' { - bytes, err := ioutil.ReadFile(input[1:]) + f, err := os.Open(strings.TrimPrefix(input[1:], "@")) if err != nil { log.Fatal(err) } - inputs = append(inputs, soongZip.ReadRespFile(bytes)...) - continue + + rspInputs, err := response.ReadRspFile(f) + f.Close() + if err != nil { + log.Fatal(err) + } + inputs = append(inputs, rspInputs...) + } else { + inputs = append(inputs, input) } - inputs = append(inputs, input) - continue } log.SetFlags(log.Lshortfile) diff --git a/response/Android.bp b/response/Android.bp new file mode 100644 index 000000000..e19981f8f --- /dev/null +++ b/response/Android.bp @@ -0,0 +1,16 @@ +package { + default_applicable_licenses: ["Android-Apache-2.0"], +} + +bootstrap_go_package { + name: "soong-response", + pkgPath: "android/soong/response", + deps: [ + ], + srcs: [ + "response.go", + ], + testSrcs: [ + "response_test.go", + ], +} diff --git a/response/response.go b/response/response.go new file mode 100644 index 000000000..e8ff4b2c9 --- /dev/null +++ b/response/response.go @@ -0,0 +1,70 @@ +// Copyright 2021 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package response + +import ( + "io" + "io/ioutil" + "unicode" +) + +const noQuote = '\x00' + +// ReadRspFile reads a file in Ninja's response file format and returns its contents. +func ReadRspFile(r io.Reader) ([]string, error) { + var files []string + var file []byte + + buf, err := ioutil.ReadAll(r) + if err != nil { + return nil, err + } + + isEscaping := false + quotingStart := byte(noQuote) + for _, c := range buf { + switch { + case isEscaping: + if quotingStart == '"' { + if !(c == '"' || c == '\\') { + // '\"' or '\\' will be escaped under double quoting. + file = append(file, '\\') + } + } + file = append(file, c) + isEscaping = false + case c == '\\' && quotingStart != '\'': + isEscaping = true + case quotingStart == noQuote && (c == '\'' || c == '"'): + quotingStart = c + case quotingStart != noQuote && c == quotingStart: + quotingStart = noQuote + case quotingStart == noQuote && unicode.IsSpace(rune(c)): + // Current character is a space outside quotes + if len(file) != 0 { + files = append(files, string(file)) + } + file = file[:0] + default: + file = append(file, c) + } + } + + if len(file) != 0 { + files = append(files, string(file)) + } + + return files, nil +} diff --git a/response/response_test.go b/response/response_test.go new file mode 100644 index 000000000..99ce62359 --- /dev/null +++ b/response/response_test.go @@ -0,0 +1,96 @@ +// Copyright 2021 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package response + +import ( + "bytes" + "reflect" + "testing" +) + +func TestReadRspFile(t *testing.T) { + testCases := []struct { + name, in string + out []string + }{ + { + name: "single quoting test case 1", + in: `./cmd '"'-C`, + out: []string{"./cmd", `"-C`}, + }, + { + name: "single quoting test case 2", + in: `./cmd '-C`, + out: []string{"./cmd", `-C`}, + }, + { + name: "single quoting test case 3", + in: `./cmd '\"'-C`, + out: []string{"./cmd", `\"-C`}, + }, + { + name: "single quoting test case 4", + in: `./cmd '\\'-C`, + out: []string{"./cmd", `\\-C`}, + }, + { + name: "none quoting test case 1", + in: `./cmd \'-C`, + out: []string{"./cmd", `'-C`}, + }, + { + name: "none quoting test case 2", + in: `./cmd \\-C`, + out: []string{"./cmd", `\-C`}, + }, + { + name: "none quoting test case 3", + in: `./cmd \"-C`, + out: []string{"./cmd", `"-C`}, + }, + { + name: "double quoting test case 1", + in: `./cmd "'"-C`, + out: []string{"./cmd", `'-C`}, + }, + { + name: "double quoting test case 2", + in: `./cmd "\\"-C`, + out: []string{"./cmd", `\-C`}, + }, + { + name: "double quoting test case 3", + in: `./cmd "\""-C`, + out: []string{"./cmd", `"-C`}, + }, + { + name: "ninja rsp file", + in: "'a'\nb\n'@'\n'foo'\\''bar'\n'foo\"bar'", + out: []string{"a", "b", "@", "foo'bar", `foo"bar`}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + got, err := ReadRspFile(bytes.NewBuffer([]byte(testCase.in))) + if err != nil { + t.Errorf("unexpected error: %q", err) + } + if !reflect.DeepEqual(got, testCase.out) { + t.Errorf("expected %q got %q", testCase.out, got) + } + }) + } +} diff --git a/zip/Android.bp b/zip/Android.bp index b28adbd51..14541eb1c 100644 --- a/zip/Android.bp +++ b/zip/Android.bp @@ -25,6 +25,7 @@ bootstrap_go_package { "android-archive-zip", "blueprint-pathtools", "soong-jar", + "soong-response", ], srcs: [ "zip.go", diff --git a/zip/cmd/main.go b/zip/cmd/main.go index fc976f689..cbc73eda6 100644 --- a/zip/cmd/main.go +++ b/zip/cmd/main.go @@ -24,7 +24,6 @@ package main import ( "flag" "fmt" - "io/ioutil" "os" "runtime" "runtime/pprof" @@ -32,6 +31,7 @@ import ( "strconv" "strings" + "android/soong/response" "android/soong/zip" ) @@ -125,12 +125,18 @@ func main() { var expandedArgs []string for _, arg := range os.Args { if strings.HasPrefix(arg, "@") { - bytes, err := ioutil.ReadFile(strings.TrimPrefix(arg, "@")) + f, err := os.Open(strings.TrimPrefix(arg, "@")) + if err != nil { + fmt.Fprintln(os.Stderr, err.Error()) + os.Exit(1) + } + + respArgs, err := response.ReadRspFile(f) + f.Close() if err != nil { fmt.Fprintln(os.Stderr, err.Error()) os.Exit(1) } - respArgs := zip.ReadRespFile(bytes) expandedArgs = append(expandedArgs, respArgs...) } else { expandedArgs = append(expandedArgs, arg) diff --git a/zip/zip.go b/zip/zip.go index 088ed0d04..a6490d452 100644 --- a/zip/zip.go +++ b/zip/zip.go @@ -29,7 +29,8 @@ import ( "sync" "syscall" "time" - "unicode" + + "android/soong/response" "github.com/google/blueprint/pathtools" @@ -164,14 +165,12 @@ func (b *FileArgsBuilder) RspFile(name string) *FileArgsBuilder { } defer f.Close() - list, err := ioutil.ReadAll(f) + arg := b.state + arg.SourceFiles, err = response.ReadRspFile(f) if err != nil { b.err = err return b } - - arg := b.state - arg.SourceFiles = ReadRespFile(list) for i := range arg.SourceFiles { arg.SourceFiles[i] = pathtools.MatchEscape(arg.SourceFiles[i]) } @@ -253,49 +252,6 @@ type ZipArgs struct { Filesystem pathtools.FileSystem } -const NOQUOTE = '\x00' - -func ReadRespFile(bytes []byte) []string { - var args []string - var arg []rune - - isEscaping := false - quotingStart := NOQUOTE - for _, c := range string(bytes) { - switch { - case isEscaping: - if quotingStart == '"' { - if !(c == '"' || c == '\\') { - // '\"' or '\\' will be escaped under double quoting. - arg = append(arg, '\\') - } - } - arg = append(arg, c) - isEscaping = false - case c == '\\' && quotingStart != '\'': - isEscaping = true - case quotingStart == NOQUOTE && (c == '\'' || c == '"'): - quotingStart = c - case quotingStart != NOQUOTE && c == quotingStart: - quotingStart = NOQUOTE - case quotingStart == NOQUOTE && unicode.IsSpace(c): - // Current character is a space outside quotes - if len(arg) != 0 { - args = append(args, string(arg)) - } - arg = arg[:0] - default: - arg = append(arg, c) - } - } - - if len(arg) != 0 { - args = append(args, string(arg)) - } - - return args -} - func zipTo(args ZipArgs, w io.Writer) error { if args.EmulateJar { args.AddDirectoryEntriesToZip = true diff --git a/zip/zip_test.go b/zip/zip_test.go index b456ef8f2..a37ae41e4 100644 --- a/zip/zip_test.go +++ b/zip/zip_test.go @@ -535,78 +535,6 @@ func TestZip(t *testing.T) { } } -func TestReadRespFile(t *testing.T) { - testCases := []struct { - name, in string - out []string - }{ - { - name: "single quoting test case 1", - in: `./cmd '"'-C`, - out: []string{"./cmd", `"-C`}, - }, - { - name: "single quoting test case 2", - in: `./cmd '-C`, - out: []string{"./cmd", `-C`}, - }, - { - name: "single quoting test case 3", - in: `./cmd '\"'-C`, - out: []string{"./cmd", `\"-C`}, - }, - { - name: "single quoting test case 4", - in: `./cmd '\\'-C`, - out: []string{"./cmd", `\\-C`}, - }, - { - name: "none quoting test case 1", - in: `./cmd \'-C`, - out: []string{"./cmd", `'-C`}, - }, - { - name: "none quoting test case 2", - in: `./cmd \\-C`, - out: []string{"./cmd", `\-C`}, - }, - { - name: "none quoting test case 3", - in: `./cmd \"-C`, - out: []string{"./cmd", `"-C`}, - }, - { - name: "double quoting test case 1", - in: `./cmd "'"-C`, - out: []string{"./cmd", `'-C`}, - }, - { - name: "double quoting test case 2", - in: `./cmd "\\"-C`, - out: []string{"./cmd", `\-C`}, - }, - { - name: "double quoting test case 3", - in: `./cmd "\""-C`, - out: []string{"./cmd", `"-C`}, - }, - { - name: "ninja rsp file", - in: "'a'\nb\n'@'\n'foo'\\''bar'\n'foo\"bar'", - out: []string{"a", "b", "@", "foo'bar", `foo"bar`}, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - got := ReadRespFile([]byte(testCase.in)) - if !reflect.DeepEqual(got, testCase.out) { - t.Errorf("expected %q got %q", testCase.out, got) - } - }) - } -} - func TestSrcJar(t *testing.T) { mockFs := pathtools.MockFs(map[string][]byte{ "wrong_package.java": []byte("package foo;"), From a4eafddc41d81d066650618eae24792cb66064c9 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 24 Mar 2021 14:09:28 -0700 Subject: [PATCH 2/7] Support multiple rsp files in REParams rewrapper supports a comma separate list of rsp files, replace REParams.RSPFile with REParmas.RSPFiles. Test: remoteexec_test.go Change-Id: I7850c071c23d368d6fad4480dd527d146c13c6d3 --- android/rule_builder.go | 2 +- cc/builder.go | 4 ++-- java/builder.go | 6 +++--- java/droiddoc.go | 2 +- remoteexec/remoteexec.go | 9 ++++----- remoteexec/remoteexec_test.go | 4 ++-- 6 files changed, 13 insertions(+), 14 deletions(-) diff --git a/android/rule_builder.go b/android/rule_builder.go index 0d8e2b7c9..4b356fa1f 100644 --- a/android/rule_builder.go +++ b/android/rule_builder.go @@ -654,7 +654,7 @@ func (r *RuleBuilder) Build(name string, desc string) { inputs = append(inputs, inputsListFile) r.rbeParams.OutputFiles = outputs.Strings() - r.rbeParams.RSPFile = inputsListFile.String() + r.rbeParams.RSPFiles = []string{inputsListFile.String()} rewrapperCommand := r.rbeParams.NoVarTemplate(r.ctx.Config().RBEWrapper()) commandString = rewrapperCommand + " bash -c '" + strings.ReplaceAll(commandString, `'`, `'\''`) + "'" } diff --git a/cc/builder.go b/cc/builder.go index 273cdd3f4..4771b8932 100644 --- a/cc/builder.go +++ b/cc/builder.go @@ -73,7 +73,7 @@ var ( Labels: map[string]string{"type": "link", "tool": "clang"}, ExecStrategy: "${config.RECXXLinksExecStrategy}", Inputs: []string{"${out}.rsp", "$implicitInputs"}, - RSPFile: "${out}.rsp", + RSPFiles: []string{"${out}.rsp"}, OutputFiles: []string{"${out}", "$implicitOutputs"}, ToolchainInputs: []string{"$ldCmd"}, Platform: map[string]string{remoteexec.PoolKey: "${config.RECXXLinksPool}"}, @@ -256,7 +256,7 @@ var ( Labels: map[string]string{"type": "tool", "name": "abi-linker"}, ExecStrategy: "${config.REAbiLinkerExecStrategy}", Inputs: []string{"$sAbiLinkerLibs", "${out}.rsp", "$implicitInputs"}, - RSPFile: "${out}.rsp", + RSPFiles: []string{"${out}.rsp"}, OutputFiles: []string{"$out"}, ToolchainInputs: []string{"$sAbiLinker"}, Platform: map[string]string{remoteexec.PoolKey: "${config.RECXXPool}"}, diff --git a/java/builder.go b/java/builder.go index fc740a831..cde87310f 100644 --- a/java/builder.go +++ b/java/builder.go @@ -150,7 +150,7 @@ var ( &remoteexec.REParams{Labels: map[string]string{"type": "tool", "name": "turbine"}, ExecStrategy: "${config.RETurbineExecStrategy}", Inputs: []string{"${config.TurbineJar}", "${out}.rsp", "$implicits"}, - RSPFile: "${out}.rsp", + RSPFiles: []string{"${out}.rsp"}, OutputFiles: []string{"$out.tmp"}, OutputDirectories: []string{"$outDir"}, ToolchainInputs: []string{"${config.JavaCmd}"}, @@ -167,7 +167,7 @@ var ( &remoteexec.REParams{ ExecStrategy: "${config.REJarExecStrategy}", Inputs: []string{"${config.SoongZipCmd}", "${out}.rsp"}, - RSPFile: "${out}.rsp", + RSPFiles: []string{"${out}.rsp"}, OutputFiles: []string{"$out"}, Platform: map[string]string{remoteexec.PoolKey: "${config.REJavaPool}"}, }, []string{"jarArgs"}, nil) @@ -182,7 +182,7 @@ var ( &remoteexec.REParams{ ExecStrategy: "${config.REZipExecStrategy}", Inputs: []string{"${config.SoongZipCmd}", "${out}.rsp", "$implicits"}, - RSPFile: "${out}.rsp", + RSPFiles: []string{"${out}.rsp"}, OutputFiles: []string{"$out"}, Platform: map[string]string{remoteexec.PoolKey: "${config.REJavaPool}"}, }, []string{"jarArgs"}, []string{"implicits"}) diff --git a/java/droiddoc.go b/java/droiddoc.go index a892b363e..34f017dc5 100644 --- a/java/droiddoc.go +++ b/java/droiddoc.go @@ -1231,7 +1231,7 @@ func metalavaCmd(ctx android.ModuleContext, rule *android.RuleBuilder, javaVersi Labels: labels, ExecStrategy: execStrategy, Inputs: inputs, - RSPFile: implicitsRsp.String(), + RSPFiles: []string{implicitsRsp.String()}, ToolchainInputs: []string{config.JavaCmd(ctx).String()}, Platform: map[string]string{remoteexec.PoolKey: pool}, EnvironmentVariables: []string{"ANDROID_SDK_HOME"}, diff --git a/remoteexec/remoteexec.go b/remoteexec/remoteexec.go index 166f68c54..ef4672a73 100644 --- a/remoteexec/remoteexec.go +++ b/remoteexec/remoteexec.go @@ -64,9 +64,8 @@ type REParams struct { ExecStrategy string // Inputs is a list of input paths or ninja variables. Inputs []string - // RSPFile is the name of the ninja variable used by the rule as a placeholder for an rsp - // input. - RSPFile string + // RSPFiles is the name of the files used by the rule as a placeholder for an rsp input. + RSPFiles []string // OutputFiles is a list of output file paths or ninja variables as placeholders for rule // outputs. OutputFiles []string @@ -134,8 +133,8 @@ func (r *REParams) wrapperArgs() string { args += " --inputs=" + strings.Join(r.Inputs, ",") } - if r.RSPFile != "" { - args += " --input_list_paths=" + r.RSPFile + if len(r.RSPFiles) > 0 { + args += " --input_list_paths=" + strings.Join(r.RSPFiles, ",") } if len(r.OutputFiles) > 0 { diff --git a/remoteexec/remoteexec_test.go b/remoteexec/remoteexec_test.go index 875aa6ae7..b117b8915 100644 --- a/remoteexec/remoteexec_test.go +++ b/remoteexec/remoteexec_test.go @@ -45,14 +45,14 @@ func TestTemplate(t *testing.T) { Inputs: []string{"$in"}, OutputFiles: []string{"$out"}, ExecStrategy: "remote", - RSPFile: "$out.rsp", + RSPFiles: []string{"$out.rsp", "out2.rsp"}, ToolchainInputs: []string{"clang++"}, Platform: map[string]string{ ContainerImageKey: DefaultImage, PoolKey: "default", }, }, - want: fmt.Sprintf("${android.RBEWrapper} --labels=compiler=clang,lang=cpp,type=compile --platform=\"Pool=default,container-image=%s\" --exec_strategy=remote --inputs=$in --input_list_paths=$out.rsp --output_files=$out --toolchain_inputs=clang++ -- ", DefaultImage), + want: fmt.Sprintf("${android.RBEWrapper} --labels=compiler=clang,lang=cpp,type=compile --platform=\"Pool=default,container-image=%s\" --exec_strategy=remote --inputs=$in --input_list_paths=$out.rsp,out2.rsp --output_files=$out --toolchain_inputs=clang++ -- ", DefaultImage), }, } for _, test := range tests { From 045bfd964099247157f7467850ad8cf9cf5c6b9e Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 24 Mar 2021 16:38:03 -0700 Subject: [PATCH 3/7] Add test for sbox input sandboxing Add a test that was dropped in Ic0db961961b186e4ed9b76246881e3f04971825c. Test: rule_builder_test.go Change-Id: Idf136919939ad28eb5260dd8d686abe6948f47c7 --- android/rule_builder_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/android/rule_builder_test.go b/android/rule_builder_test.go index 9cd60a2a9..00b0a5752 100644 --- a/android/rule_builder_test.go +++ b/android/rule_builder_test.go @@ -458,6 +458,35 @@ func TestRuleBuilder(t *testing.T) { AssertSame(t, "rule.composeRspFileContent()", wantRspFileContent, rule.composeRspFileContent(rule.RspFileInputs())) }) + + t.Run("sbox inputs", func(t *testing.T) { + rule := NewRuleBuilder(pctx, ctx).Sbox(PathForOutput(ctx, "module"), + PathForOutput(ctx, "sbox.textproto")).SandboxInputs() + addCommands(rule) + + wantCommands := []string{ + "__SBOX_SANDBOX_DIR__/out/DepFile Flag FlagWithArg=arg FlagWithDepFile=__SBOX_SANDBOX_DIR__/out/depfile " + + "FlagWithInput=input FlagWithOutput=__SBOX_SANDBOX_DIR__/out/output " + + "FlagWithRspFileInputList=__SBOX_SANDBOX_DIR__/out/rsp Input __SBOX_SANDBOX_DIR__/out/Output " + + "__SBOX_SANDBOX_DIR__/out/SymlinkOutput Text __SBOX_SANDBOX_DIR__/tools/src/Tool after command2 old cmd", + "command2 __SBOX_SANDBOX_DIR__/out/depfile2 input2 __SBOX_SANDBOX_DIR__/out/output2 __SBOX_SANDBOX_DIR__/tools/src/tool2", + "command3 input3 __SBOX_SANDBOX_DIR__/out/output2 __SBOX_SANDBOX_DIR__/out/output3 input3 __SBOX_SANDBOX_DIR__/out/output2", + } + + wantDepMergerCommand := "__SBOX_SANDBOX_DIR__/tools/out/bin/dep_fixer __SBOX_SANDBOX_DIR__/out/DepFile __SBOX_SANDBOX_DIR__/out/depfile __SBOX_SANDBOX_DIR__/out/ImplicitDepFile __SBOX_SANDBOX_DIR__/out/depfile2" + + AssertDeepEquals(t, "rule.Commands()", wantCommands, rule.Commands()) + + AssertDeepEquals(t, "rule.Inputs()", wantInputs, rule.Inputs()) + AssertDeepEquals(t, "rule.RspfileInputs()", wantRspFileInputs, rule.RspFileInputs()) + AssertDeepEquals(t, "rule.Outputs()", wantOutputs, rule.Outputs()) + AssertDeepEquals(t, "rule.SymlinkOutputs()", wantSymlinkOutputs, rule.SymlinkOutputs()) + AssertDeepEquals(t, "rule.DepFiles()", wantDepFiles, rule.DepFiles()) + AssertDeepEquals(t, "rule.Tools()", wantTools, rule.Tools()) + AssertDeepEquals(t, "rule.OrderOnlys()", wantOrderOnlys, rule.OrderOnlys()) + + AssertSame(t, "rule.depFileMergerCmd()", wantDepMergerCommand, rule.depFileMergerCmd(rule.DepFiles()).String()) + }) } func testRuleBuilderFactory() Module { From e55bd423dfd3acb1d4dda6c2f120abf00578d42e Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 23 Mar 2021 13:44:30 -0700 Subject: [PATCH 4/7] Pass rsp files into sbox and rewrapper The current implementation causes inputs listed in an rsp file used with sbox to be duplicated 3 times in the build.ninja file; once as a dependency of the rule, once in the rspfile_content field of the rule with the paths rewritten to be relative to the sandbox, and once in the rule to write the sbox manifest. When RBE is enabled it also gets a fourth copy in the list of files to be treated as inputs by rewrapper. Reduce this to a single copy by using "$in" for the rspfile_content so that the files only have to be listed in the input dependencies of the rule, and then add support to sbox to rewrite the rsp file while copying it into the sandbox, and pass it to rewrapper as well. Test: m lint-check Change-Id: I3f46f61119508d39a8bb231c99fc130153fb6f04 --- android/Android.bp | 1 + android/rule_builder.go | 87 +++++++++--------- android/rule_builder_test.go | 12 --- cmd/sbox/Android.bp | 1 + cmd/sbox/sbox.go | 90 ++++++++++++++++++- cmd/sbox/sbox_proto/sbox.pb.go | 159 ++++++++++++++++++++++++++++----- cmd/sbox/sbox_proto/sbox.proto | 21 ++++- response/response.go | 42 +++++++++ response/response_test.go | 27 ++++++ 9 files changed, 359 insertions(+), 81 deletions(-) diff --git a/android/Android.bp b/android/Android.bp index 4da0f4e74..773aa6ae1 100644 --- a/android/Android.bp +++ b/android/Android.bp @@ -14,6 +14,7 @@ bootstrap_go_package { "soong-bazel", "soong-cquery", "soong-remoteexec", + "soong-response", "soong-shared", "soong-ui-metrics_proto", ], diff --git a/android/rule_builder.go b/android/rule_builder.go index 4b356fa1f..4813d7a10 100644 --- a/android/rule_builder.go +++ b/android/rule_builder.go @@ -28,6 +28,7 @@ import ( "android/soong/cmd/sbox/sbox_proto" "android/soong/remoteexec" + "android/soong/response" "android/soong/shared" ) @@ -459,26 +460,6 @@ func (r *RuleBuilder) depFileMergerCmd(depFiles WritablePaths) *RuleBuilderComma Inputs(depFiles.Paths()) } -// composeRspFileContent returns a string that will serve as the contents of the rsp file to pass -// the listed input files to the command running in the sandbox. -func (r *RuleBuilder) composeRspFileContent(rspFileInputs Paths) string { - if r.sboxInputs { - if len(rspFileInputs) > 0 { - // When SandboxInputs is used the paths need to be rewritten to be relative to the sandbox - // directory so that they are valid after sbox chdirs into the sandbox directory. - return proptools.NinjaEscape(strings.Join(r.sboxPathsForInputsRel(rspFileInputs), " ")) - } else { - // If the list of inputs is empty fall back to "$in" so that the rspfilecontent Ninja - // variable is set to something non-empty, otherwise ninja will complain. The inputs - // will be empty (all the non-rspfile inputs are implicits), so $in will evaluate to - // an empty string. - return "$in" - } - } else { - return "$in" - } -} - // Build adds the built command line to the build graph, with dependencies on Inputs and Tools, and output files for // Outputs. func (r *RuleBuilder) Build(name string, desc string) { @@ -577,9 +558,19 @@ func (r *RuleBuilder) Build(name string, desc string) { // If using an rsp file copy it into the sbox directory. if rspFilePath != nil { - command.CopyBefore = append(command.CopyBefore, &sbox_proto.Copy{ - From: proto.String(rspFilePath.String()), - To: proto.String(r.sboxPathForInputRel(rspFilePath)), + command.RspFiles = append(command.RspFiles, &sbox_proto.RspFile{ + File: proto.String(rspFilePath.String()), + // These have to match the logic in sboxPathForInputRel + PathMappings: []*sbox_proto.PathMapping{ + { + From: proto.String(r.outDir.String()), + To: proto.String(sboxOutSubDir), + }, + { + From: proto.String(PathForOutput(r.ctx).String()), + To: proto.String(sboxOutSubDir), + }, + }, }) } @@ -641,20 +632,30 @@ func (r *RuleBuilder) Build(name string, desc string) { inputs = append(inputs, sboxCmd.inputs...) if r.rbeParams != nil { - var remoteInputs []string - remoteInputs = append(remoteInputs, inputs.Strings()...) - remoteInputs = append(remoteInputs, tools.Strings()...) - remoteInputs = append(remoteInputs, rspFileInputs.Strings()...) + // RBE needs a list of input files to copy to the remote builder. For inputs already + // listed in an rsp file, pass the rsp file directly to rewrapper. For the rest, + // create a new rsp file to pass to rewrapper. + var remoteRspFiles Paths + var remoteInputs Paths + + remoteInputs = append(remoteInputs, inputs...) + remoteInputs = append(remoteInputs, tools...) + if rspFilePath != nil { - remoteInputs = append(remoteInputs, rspFilePath.String()) + remoteInputs = append(remoteInputs, rspFilePath) + remoteRspFiles = append(remoteRspFiles, rspFilePath) + } + + if len(remoteInputs) > 0 { + inputsListFile := r.sboxManifestPath.ReplaceExtension(r.ctx, "rbe_inputs.list") + writeRspFileRule(r.ctx, inputsListFile, remoteInputs) + remoteRspFiles = append(remoteRspFiles, inputsListFile) + // Add the new rsp file as an extra input to the rule. + inputs = append(inputs, inputsListFile) } - inputsListFile := r.sboxManifestPath.ReplaceExtension(r.ctx, "rbe_inputs.list") - inputsListContents := rspFileForInputs(remoteInputs) - WriteFileRule(r.ctx, inputsListFile, inputsListContents) - inputs = append(inputs, inputsListFile) r.rbeParams.OutputFiles = outputs.Strings() - r.rbeParams.RSPFiles = []string{inputsListFile.String()} + r.rbeParams.RSPFiles = remoteRspFiles.Strings() rewrapperCommand := r.rbeParams.NoVarTemplate(r.ctx.Config().RBEWrapper()) commandString = rewrapperCommand + " bash -c '" + strings.ReplaceAll(commandString, `'`, `'\''`) + "'" } @@ -674,7 +675,10 @@ func (r *RuleBuilder) Build(name string, desc string) { var rspFile, rspFileContent string if rspFilePath != nil { rspFile = rspFilePath.String() - rspFileContent = r.composeRspFileContent(rspFileInputs) + // Use "$in" for rspFileContent to avoid duplicating the list of files in the dependency + // list and in the contents of the rsp file. Inputs to the rule that are not in the + // rsp file will be listed in Implicits instead of Inputs so they don't show up in "$in". + rspFileContent = "$in" } var pool blueprint.Pool @@ -1264,13 +1268,12 @@ func (builderContextForTests) Rule(PackageContext, string, blueprint.RuleParams, } func (builderContextForTests) Build(PackageContext, BuildParams) {} -func rspFileForInputs(paths []string) string { - s := strings.Builder{} - for i, path := range paths { - if i != 0 { - s.WriteByte(' ') - } - s.WriteString(proptools.ShellEscape(path)) +func writeRspFileRule(ctx BuilderContext, rspFile WritablePath, paths Paths) { + buf := &strings.Builder{} + err := response.WriteRspFile(buf, paths.Strings()) + if err != nil { + // There should never be I/O errors writing to a bytes.Buffer. + panic(err) } - return s.String() + WriteFileRule(ctx, rspFile, buf.String()) } diff --git a/android/rule_builder_test.go b/android/rule_builder_test.go index 00b0a5752..3df900f88 100644 --- a/android/rule_builder_test.go +++ b/android/rule_builder_test.go @@ -376,8 +376,6 @@ func TestRuleBuilder(t *testing.T) { wantDepMergerCommand := "out_local/host/" + ctx.Config().PrebuiltOS() + "/bin/dep_fixer " + "out_local/module/DepFile out_local/module/depfile out_local/module/ImplicitDepFile out_local/module/depfile2" - wantRspFileContent := "$in" - AssertDeepEquals(t, "rule.Commands()", wantCommands, rule.Commands()) AssertDeepEquals(t, "rule.Inputs()", wantInputs, rule.Inputs()) @@ -389,8 +387,6 @@ func TestRuleBuilder(t *testing.T) { AssertDeepEquals(t, "rule.OrderOnlys()", wantOrderOnlys, rule.OrderOnlys()) AssertSame(t, "rule.depFileMergerCmd()", wantDepMergerCommand, rule.depFileMergerCmd(rule.DepFiles()).String()) - - AssertSame(t, "rule.composeRspFileContent()", wantRspFileContent, rule.composeRspFileContent(rule.RspFileInputs())) }) t.Run("sbox", func(t *testing.T) { @@ -409,8 +405,6 @@ func TestRuleBuilder(t *testing.T) { wantDepMergerCommand := "out_local/host/" + ctx.Config().PrebuiltOS() + "/bin/dep_fixer __SBOX_SANDBOX_DIR__/out/DepFile __SBOX_SANDBOX_DIR__/out/depfile __SBOX_SANDBOX_DIR__/out/ImplicitDepFile __SBOX_SANDBOX_DIR__/out/depfile2" - wantRspFileContent := "$in" - AssertDeepEquals(t, "rule.Commands()", wantCommands, rule.Commands()) AssertDeepEquals(t, "rule.Inputs()", wantInputs, rule.Inputs()) @@ -422,8 +416,6 @@ func TestRuleBuilder(t *testing.T) { AssertDeepEquals(t, "rule.OrderOnlys()", wantOrderOnlys, rule.OrderOnlys()) AssertSame(t, "rule.depFileMergerCmd()", wantDepMergerCommand, rule.depFileMergerCmd(rule.DepFiles()).String()) - - AssertSame(t, "rule.composeRspFileContent()", wantRspFileContent, rule.composeRspFileContent(rule.RspFileInputs())) }) t.Run("sbox tools", func(t *testing.T) { @@ -442,8 +434,6 @@ func TestRuleBuilder(t *testing.T) { wantDepMergerCommand := "__SBOX_SANDBOX_DIR__/tools/out/bin/dep_fixer __SBOX_SANDBOX_DIR__/out/DepFile __SBOX_SANDBOX_DIR__/out/depfile __SBOX_SANDBOX_DIR__/out/ImplicitDepFile __SBOX_SANDBOX_DIR__/out/depfile2" - wantRspFileContent := "$in" - AssertDeepEquals(t, "rule.Commands()", wantCommands, rule.Commands()) AssertDeepEquals(t, "rule.Inputs()", wantInputs, rule.Inputs()) @@ -455,8 +445,6 @@ func TestRuleBuilder(t *testing.T) { AssertDeepEquals(t, "rule.OrderOnlys()", wantOrderOnlys, rule.OrderOnlys()) AssertSame(t, "rule.depFileMergerCmd()", wantDepMergerCommand, rule.depFileMergerCmd(rule.DepFiles()).String()) - - AssertSame(t, "rule.composeRspFileContent()", wantRspFileContent, rule.composeRspFileContent(rule.RspFileInputs())) }) t.Run("sbox inputs", func(t *testing.T) { diff --git a/cmd/sbox/Android.bp b/cmd/sbox/Android.bp index d88505fcc..b8d75ed7a 100644 --- a/cmd/sbox/Android.bp +++ b/cmd/sbox/Android.bp @@ -21,6 +21,7 @@ blueprint_go_binary { deps: [ "sbox_proto", "soong-makedeps", + "soong-response", ], srcs: [ "sbox.go", diff --git a/cmd/sbox/sbox.go b/cmd/sbox/sbox.go index f47c6012e..fcc80a94e 100644 --- a/cmd/sbox/sbox.go +++ b/cmd/sbox/sbox.go @@ -32,6 +32,7 @@ import ( "android/soong/cmd/sbox/sbox_proto" "android/soong/makedeps" + "android/soong/response" "github.com/golang/protobuf/proto" ) @@ -218,6 +219,11 @@ func runCommand(command *sbox_proto.Command, tempDir string) (depFile string, er return "", fmt.Errorf("command is required") } + pathToTempDirInSbox := tempDir + if command.GetChdir() { + pathToTempDirInSbox = "." + } + err = os.MkdirAll(tempDir, 0777) if err != nil { return "", fmt.Errorf("failed to create %q: %w", tempDir, err) @@ -228,10 +234,9 @@ func runCommand(command *sbox_proto.Command, tempDir string) (depFile string, er if err != nil { return "", err } - - pathToTempDirInSbox := tempDir - if command.GetChdir() { - pathToTempDirInSbox = "." + err = copyRspFiles(command.RspFiles, tempDir, pathToTempDirInSbox) + if err != nil { + return "", err } if strings.Contains(rawCommand, depFilePlaceholder) { @@ -409,6 +414,83 @@ func copyOneFile(from string, to string, executable bool) error { return nil } +// copyRspFiles copies rsp files into the sandbox with path mappings, and also copies the files +// listed into the sandbox. +func copyRspFiles(rspFiles []*sbox_proto.RspFile, toDir, toDirInSandbox string) error { + for _, rspFile := range rspFiles { + err := copyOneRspFile(rspFile, toDir, toDirInSandbox) + if err != nil { + return err + } + } + return nil +} + +// copyOneRspFiles copies an rsp file into the sandbox with path mappings, and also copies the files +// listed into the sandbox. +func copyOneRspFile(rspFile *sbox_proto.RspFile, toDir, toDirInSandbox string) error { + in, err := os.Open(rspFile.GetFile()) + if err != nil { + return err + } + defer in.Close() + + files, err := response.ReadRspFile(in) + if err != nil { + return err + } + + for i, from := range files { + // Convert the real path of the input file into the path inside the sandbox using the + // path mappings. + to := applyPathMappings(rspFile.PathMappings, from) + + // Copy the file into the sandbox. + err := copyOneFile(from, joinPath(toDir, to), false) + if err != nil { + return err + } + + // Rewrite the name in the list of files to be relative to the sandbox directory. + files[i] = joinPath(toDirInSandbox, to) + } + + // Convert the real path of the rsp file into the path inside the sandbox using the path + // mappings. + outRspFile := joinPath(toDir, applyPathMappings(rspFile.PathMappings, rspFile.GetFile())) + + err = os.MkdirAll(filepath.Dir(outRspFile), 0777) + if err != nil { + return err + } + + out, err := os.Create(outRspFile) + if err != nil { + return err + } + defer out.Close() + + // Write the rsp file with converted paths into the sandbox. + err = response.WriteRspFile(out, files) + if err != nil { + return err + } + + return nil +} + +// applyPathMappings takes a list of path mappings and a path, and returns the path with the first +// matching path mapping applied. If the path does not match any of the path mappings then it is +// returned unmodified. +func applyPathMappings(pathMappings []*sbox_proto.PathMapping, path string) string { + for _, mapping := range pathMappings { + if strings.HasPrefix(path, mapping.GetFrom()+"/") { + return joinPath(mapping.GetTo()+"/", strings.TrimPrefix(path, mapping.GetFrom()+"/")) + } + } + return path +} + // moveFiles moves files specified by a set of copy rules. It uses os.Rename, so it is restricted // to moving files where the source and destination are in the same filesystem. This is OK for // sbox because the temporary directory is inside the out directory. It updates the timestamp diff --git a/cmd/sbox/sbox_proto/sbox.pb.go b/cmd/sbox/sbox_proto/sbox.pb.go index 79bb90c4c..b996481c3 100644 --- a/cmd/sbox/sbox_proto/sbox.pb.go +++ b/cmd/sbox/sbox_proto/sbox.pb.go @@ -86,10 +86,13 @@ type Command struct { CopyAfter []*Copy `protobuf:"bytes,4,rep,name=copy_after,json=copyAfter" json:"copy_after,omitempty"` // An optional hash of the input files to ensure the textproto files and the sbox rule reruns // when the lists of inputs changes, even if the inputs are not on the command line. - InputHash *string `protobuf:"bytes,5,opt,name=input_hash,json=inputHash" json:"input_hash,omitempty"` - XXX_NoUnkeyedLiteral struct{} `json:"-"` - XXX_unrecognized []byte `json:"-"` - XXX_sizecache int32 `json:"-"` + InputHash *string `protobuf:"bytes,5,opt,name=input_hash,json=inputHash" json:"input_hash,omitempty"` + // A list of files that will be copied before the sandboxed command, and whose contents should be + // copied as if they were listed in copy_before. + RspFiles []*RspFile `protobuf:"bytes,6,rep,name=rsp_files,json=rspFiles" json:"rsp_files,omitempty"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` } func (m *Command) Reset() { *m = Command{} } @@ -152,6 +155,13 @@ func (m *Command) GetInputHash() string { return "" } +func (m *Command) GetRspFiles() []*RspFile { + if m != nil { + return m.RspFiles + } + return nil +} + // Copy describes a from-to pair of files to copy. The paths may be relative, the root that they // are relative to is specific to the context the Copy is used in and will be different for // from and to. @@ -211,10 +221,110 @@ func (m *Copy) GetExecutable() bool { return false } +// RspFile describes an rspfile that should be copied into the sandbox directory. +type RspFile struct { + // The path to the rsp file. + File *string `protobuf:"bytes,1,req,name=file" json:"file,omitempty"` + // A list of path mappings that should be applied to each file listed in the rsp file. + PathMappings []*PathMapping `protobuf:"bytes,2,rep,name=path_mappings,json=pathMappings" json:"path_mappings,omitempty"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` +} + +func (m *RspFile) Reset() { *m = RspFile{} } +func (m *RspFile) String() string { return proto.CompactTextString(m) } +func (*RspFile) ProtoMessage() {} +func (*RspFile) Descriptor() ([]byte, []int) { + return fileDescriptor_9d0425bf0de86ed1, []int{3} +} + +func (m *RspFile) XXX_Unmarshal(b []byte) error { + return xxx_messageInfo_RspFile.Unmarshal(m, b) +} +func (m *RspFile) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) { + return xxx_messageInfo_RspFile.Marshal(b, m, deterministic) +} +func (m *RspFile) XXX_Merge(src proto.Message) { + xxx_messageInfo_RspFile.Merge(m, src) +} +func (m *RspFile) XXX_Size() int { + return xxx_messageInfo_RspFile.Size(m) +} +func (m *RspFile) XXX_DiscardUnknown() { + xxx_messageInfo_RspFile.DiscardUnknown(m) +} + +var xxx_messageInfo_RspFile proto.InternalMessageInfo + +func (m *RspFile) GetFile() string { + if m != nil && m.File != nil { + return *m.File + } + return "" +} + +func (m *RspFile) GetPathMappings() []*PathMapping { + if m != nil { + return m.PathMappings + } + return nil +} + +// PathMapping describes a mapping from a path outside the sandbox to the path inside the sandbox. +type PathMapping struct { + From *string `protobuf:"bytes,1,req,name=from" json:"from,omitempty"` + To *string `protobuf:"bytes,2,req,name=to" json:"to,omitempty"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` +} + +func (m *PathMapping) Reset() { *m = PathMapping{} } +func (m *PathMapping) String() string { return proto.CompactTextString(m) } +func (*PathMapping) ProtoMessage() {} +func (*PathMapping) Descriptor() ([]byte, []int) { + return fileDescriptor_9d0425bf0de86ed1, []int{4} +} + +func (m *PathMapping) XXX_Unmarshal(b []byte) error { + return xxx_messageInfo_PathMapping.Unmarshal(m, b) +} +func (m *PathMapping) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) { + return xxx_messageInfo_PathMapping.Marshal(b, m, deterministic) +} +func (m *PathMapping) XXX_Merge(src proto.Message) { + xxx_messageInfo_PathMapping.Merge(m, src) +} +func (m *PathMapping) XXX_Size() int { + return xxx_messageInfo_PathMapping.Size(m) +} +func (m *PathMapping) XXX_DiscardUnknown() { + xxx_messageInfo_PathMapping.DiscardUnknown(m) +} + +var xxx_messageInfo_PathMapping proto.InternalMessageInfo + +func (m *PathMapping) GetFrom() string { + if m != nil && m.From != nil { + return *m.From + } + return "" +} + +func (m *PathMapping) GetTo() string { + if m != nil && m.To != nil { + return *m.To + } + return "" +} + func init() { proto.RegisterType((*Manifest)(nil), "sbox.Manifest") proto.RegisterType((*Command)(nil), "sbox.Command") proto.RegisterType((*Copy)(nil), "sbox.Copy") + proto.RegisterType((*RspFile)(nil), "sbox.RspFile") + proto.RegisterType((*PathMapping)(nil), "sbox.PathMapping") } func init() { @@ -222,22 +332,27 @@ func init() { } var fileDescriptor_9d0425bf0de86ed1 = []byte{ - // 268 bytes of a gzipped FileDescriptorProto - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x64, 0x90, 0x4f, 0x4b, 0xc3, 0x40, - 0x10, 0xc5, 0xc9, 0x9f, 0xd2, 0x64, 0x6a, 0x7b, 0x18, 0x3c, 0xec, 0x45, 0x09, 0x01, 0x21, 0x45, - 0xe8, 0xc1, 0x6f, 0x60, 0xf5, 0x20, 0x82, 0x97, 0x1c, 0x45, 0x08, 0x9b, 0x64, 0x43, 0x02, 0x4d, - 0x26, 0xec, 0x6e, 0xa0, 0xfd, 0x56, 0x7e, 0x44, 0xd9, 0x49, 0x2a, 0x82, 0xb7, 0x99, 0xdf, 0xe3, - 0xcd, 0x7b, 0x0c, 0x80, 0x29, 0xe9, 0x7c, 0x18, 0x35, 0x59, 0xc2, 0xd0, 0xcd, 0xe9, 0x17, 0x44, - 0x1f, 0x72, 0xe8, 0x1a, 0x65, 0x2c, 0xee, 0x21, 0xaa, 0xa8, 0xef, 0xe5, 0x50, 0x1b, 0xe1, 0x25, - 0x41, 0xb6, 0x79, 0xda, 0x1e, 0xd8, 0xf0, 0x32, 0xd3, 0xfc, 0x57, 0xc6, 0x07, 0xd8, 0xd1, 0x64, - 0xc7, 0xc9, 0x16, 0xb5, 0x1a, 0x9b, 0xee, 0xa4, 0x84, 0x9f, 0x78, 0x59, 0x9c, 0x6f, 0x67, 0xfa, - 0x3a, 0xc3, 0xf4, 0xdb, 0x83, 0xf5, 0x62, 0xc6, 0x47, 0xd8, 0x54, 0x34, 0x5e, 0x8a, 0x52, 0x35, - 0xa4, 0xd5, 0x12, 0x00, 0xd7, 0x80, 0xf1, 0x92, 0x83, 0x93, 0x8f, 0xac, 0xe2, 0x2d, 0xac, 0xaa, - 0xb6, 0xee, 0x34, 0x9f, 0x8d, 0xf2, 0x79, 0x41, 0x01, 0xeb, 0xa5, 0x81, 0x08, 0x12, 0x3f, 0x8b, - 0xf3, 0xeb, 0x8a, 0x7b, 0x60, 0x77, 0x21, 0x1b, 0xab, 0xb4, 0x08, 0xff, 0xdd, 0x8e, 0x9d, 0xfa, - 0xec, 0x44, 0xbc, 0x03, 0xe8, 0x06, 0xd7, 0xbc, 0x95, 0xa6, 0x15, 0x2b, 0xae, 0x1d, 0x33, 0x79, - 0x93, 0xa6, 0x4d, 0xdf, 0x21, 0x74, 0x0e, 0x44, 0x08, 0x1b, 0x4d, 0xbd, 0xf0, 0x38, 0x88, 0x67, - 0xdc, 0x81, 0x6f, 0x49, 0xf8, 0x4c, 0x7c, 0x4b, 0x78, 0x0f, 0xa0, 0xce, 0xaa, 0x9a, 0xac, 0x2c, - 0x4f, 0x4a, 0x04, 0x5c, 0xf5, 0x0f, 0x39, 0xde, 0x7c, 0xf2, 0xc3, 0x0b, 0x7e, 0xf8, 0x4f, 0x00, - 0x00, 0x00, 0xff, 0xff, 0x78, 0x37, 0x3e, 0x6a, 0x7d, 0x01, 0x00, 0x00, + // 342 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x8c, 0x92, 0x5f, 0x4b, 0xc3, 0x30, + 0x14, 0xc5, 0x69, 0xd7, 0xb9, 0xf6, 0xee, 0x0f, 0x18, 0x7c, 0xc8, 0x8b, 0x32, 0x0a, 0xc2, 0xa6, + 0x30, 0xd0, 0x07, 0xdf, 0x9d, 0x22, 0x22, 0x0c, 0x24, 0xe0, 0x8b, 0x08, 0x25, 0xeb, 0x52, 0x5b, + 0x58, 0x9b, 0x90, 0x64, 0xb0, 0x7d, 0x57, 0x3f, 0x8c, 0xe4, 0xa6, 0xd3, 0x82, 0x2f, 0xbe, 0xdd, + 0x7b, 0x0e, 0xf7, 0xdc, 0x5f, 0xc2, 0x05, 0x30, 0x6b, 0xb9, 0x5f, 0x28, 0x2d, 0xad, 0x24, 0x91, + 0xab, 0xd3, 0x0f, 0x88, 0x57, 0xbc, 0xa9, 0x0a, 0x61, 0x2c, 0x99, 0x43, 0x9c, 0xcb, 0xba, 0xe6, + 0xcd, 0xc6, 0xd0, 0x60, 0xda, 0x9b, 0x0d, 0x6f, 0xc7, 0x0b, 0x1c, 0x78, 0xf0, 0x2a, 0xfb, 0xb1, + 0xc9, 0x25, 0x4c, 0xe4, 0xce, 0xaa, 0x9d, 0xcd, 0x36, 0x42, 0x15, 0xd5, 0x56, 0xd0, 0x70, 0x1a, + 0xcc, 0x12, 0x36, 0xf6, 0xea, 0xa3, 0x17, 0xd3, 0xaf, 0x00, 0x06, 0xed, 0x30, 0xb9, 0x86, 0x61, + 0x2e, 0xd5, 0x21, 0x5b, 0x8b, 0x42, 0x6a, 0xd1, 0x2e, 0x80, 0xe3, 0x02, 0x75, 0x60, 0xe0, 0xec, + 0x25, 0xba, 0xe4, 0x0c, 0xfa, 0x79, 0xb9, 0xa9, 0x34, 0xc6, 0xc6, 0xcc, 0x37, 0x84, 0xc2, 0xa0, + 0x25, 0xa0, 0xbd, 0x69, 0x38, 0x4b, 0xd8, 0xb1, 0x25, 0x73, 0xc0, 0xe9, 0x8c, 0x17, 0x56, 0x68, + 0x1a, 0xfd, 0xc9, 0x4e, 0x9c, 0x7b, 0xef, 0x4c, 0x72, 0x0e, 0x50, 0x35, 0x8e, 0xbc, 0xe4, 0xa6, + 0xa4, 0x7d, 0xc4, 0x4e, 0x50, 0x79, 0xe6, 0xa6, 0x24, 0x57, 0x90, 0x68, 0xa3, 0x32, 0x87, 0x6f, + 0xe8, 0x49, 0xf7, 0x17, 0x98, 0x51, 0x4f, 0xd5, 0x56, 0xb0, 0x58, 0xfb, 0xc2, 0xa4, 0x2f, 0x10, + 0xb9, 0x74, 0x42, 0x20, 0x2a, 0xb4, 0xac, 0x69, 0x80, 0x50, 0x58, 0x93, 0x09, 0x84, 0x56, 0xd2, + 0x10, 0x95, 0xd0, 0x4a, 0x72, 0x01, 0x20, 0xf6, 0x22, 0xdf, 0x59, 0xbe, 0xde, 0x0a, 0xda, 0xc3, + 0x67, 0x75, 0x94, 0xf4, 0x0d, 0x06, 0xed, 0x02, 0x8c, 0x73, 0x5f, 0x7a, 0x8c, 0x73, 0xda, 0x1d, + 0x8c, 0x15, 0xb7, 0x65, 0x56, 0x73, 0xa5, 0xaa, 0xe6, 0xd3, 0xd0, 0x10, 0xd1, 0x4e, 0x3d, 0xda, + 0x2b, 0xb7, 0xe5, 0xca, 0x3b, 0x6c, 0xa4, 0x7e, 0x1b, 0x93, 0xde, 0xc0, 0xb0, 0x63, 0xfe, 0x87, + 0x74, 0x39, 0x7a, 0xc7, 0x33, 0xc9, 0xf0, 0x4c, 0xbe, 0x03, 0x00, 0x00, 0xff, 0xff, 0x83, 0x82, + 0xb0, 0xc3, 0x33, 0x02, 0x00, 0x00, } diff --git a/cmd/sbox/sbox_proto/sbox.proto b/cmd/sbox/sbox_proto/sbox.proto index 695b0e86e..bdf92c669 100644 --- a/cmd/sbox/sbox_proto/sbox.proto +++ b/cmd/sbox/sbox_proto/sbox.proto @@ -47,6 +47,10 @@ message Command { // An optional hash of the input files to ensure the textproto files and the sbox rule reruns // when the lists of inputs changes, even if the inputs are not on the command line. optional string input_hash = 5; + + // A list of files that will be copied before the sandboxed command, and whose contents should be + // copied as if they were listed in copy_before. + repeated RspFile rsp_files = 6; } // Copy describes a from-to pair of files to copy. The paths may be relative, the root that they @@ -58,4 +62,19 @@ message Copy { // If true, make the file executable after copying it. optional bool executable = 3; -} \ No newline at end of file +} + +// RspFile describes an rspfile that should be copied into the sandbox directory. +message RspFile { + // The path to the rsp file. + required string file = 1; + + // A list of path mappings that should be applied to each file listed in the rsp file. + repeated PathMapping path_mappings = 2; +} + +// PathMapping describes a mapping from a path outside the sandbox to the path inside the sandbox. +message PathMapping { + required string from = 1; + required string to = 2; +} diff --git a/response/response.go b/response/response.go index e8ff4b2c9..b65503eb4 100644 --- a/response/response.go +++ b/response/response.go @@ -17,6 +17,7 @@ package response import ( "io" "io/ioutil" + "strings" "unicode" ) @@ -68,3 +69,44 @@ func ReadRspFile(r io.Reader) ([]string, error) { return files, nil } + +func rspUnsafeChar(r rune) bool { + switch { + case 'A' <= r && r <= 'Z', + 'a' <= r && r <= 'z', + '0' <= r && r <= '9', + r == '_', + r == '+', + r == '-', + r == '.', + r == '/': + return false + default: + return true + } +} + +var rspEscaper = strings.NewReplacer(`'`, `'\''`) + +// WriteRspFile writes a list of files to a file in Ninja's response file format. +func WriteRspFile(w io.Writer, files []string) error { + for i, f := range files { + if i != 0 { + _, err := io.WriteString(w, " ") + if err != nil { + return err + } + } + + if strings.IndexFunc(f, rspUnsafeChar) != -1 { + f = `'` + rspEscaper.Replace(f) + `'` + } + + _, err := io.WriteString(w, f) + if err != nil { + return err + } + } + + return nil +} diff --git a/response/response_test.go b/response/response_test.go index 99ce62359..4d1fb417a 100644 --- a/response/response_test.go +++ b/response/response_test.go @@ -94,3 +94,30 @@ func TestReadRspFile(t *testing.T) { }) } } + +func TestWriteRspFile(t *testing.T) { + testCases := []struct { + name string + in []string + out string + }{ + { + name: "ninja rsp file", + in: []string{"a", "b", "@", "foo'bar", `foo"bar`}, + out: "a b '@' 'foo'\\''bar' 'foo\"bar'", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + buf := &bytes.Buffer{} + err := WriteRspFile(buf, testCase.in) + if err != nil { + t.Errorf("unexpected error: %q", err) + } + if buf.String() != testCase.out { + t.Errorf("expected %q got %q", testCase.out, buf.String()) + } + }) + } +} From ce3a51dc96fe82fabc3877fe927214c72cff0119 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 19 Mar 2021 16:22:12 -0700 Subject: [PATCH 5/7] Support multiple rsp files in RuleBuilder The lint rule is manually creating a second rsp file because Ninja only supports on per rule. Move the support into RuleBuilder so that it can apply the same rewrites that it does to the primary one. Test: TestRuleBuilder_Build Change-Id: Iec250a2d60e74ccf1b4ad085a960fec6867ea559 --- android/rule_builder.go | 76 +++++++++++++------------- android/rule_builder_test.go | 100 ++++++++++++++++++++++++++++++----- java/java_test.go | 2 +- 3 files changed, 127 insertions(+), 51 deletions(-) diff --git a/android/rule_builder.go b/android/rule_builder.go index 4813d7a10..72c0d10cd 100644 --- a/android/rule_builder.go +++ b/android/rule_builder.go @@ -409,30 +409,21 @@ func (r *RuleBuilder) Tools() Paths { func (r *RuleBuilder) RspFileInputs() Paths { var rspFileInputs Paths for _, c := range r.commands { - if c.rspFileInputs != nil { - if rspFileInputs != nil { - panic("Multiple commands in a rule may not have rsp file inputs") - } - rspFileInputs = c.rspFileInputs + for _, rspFile := range c.rspFiles { + rspFileInputs = append(rspFileInputs, rspFile.paths...) } } return rspFileInputs } -// RspFile returns the path to the rspfile that was passed to the RuleBuilderCommand.FlagWithRspFileInputList method. -func (r *RuleBuilder) RspFile() WritablePath { - var rspFile WritablePath +func (r *RuleBuilder) rspFiles() []rspFileAndPaths { + var rspFiles []rspFileAndPaths for _, c := range r.commands { - if c.rspFile != nil { - if rspFile != nil { - panic("Multiple commands in a rule may not have rsp file inputs") - } - rspFile = c.rspFile - } + rspFiles = append(rspFiles, c.rspFiles...) } - return rspFile + return rspFiles } // Commands returns a slice containing the built command line for each call to RuleBuilder.Command. @@ -501,8 +492,7 @@ func (r *RuleBuilder) Build(name string, desc string) { commands := r.Commands() outputs := r.Outputs() inputs := r.Inputs() - rspFileInputs := r.RspFileInputs() - rspFilePath := r.RspFile() + rspFiles := r.rspFiles() if len(commands) == 0 { return @@ -556,10 +546,11 @@ func (r *RuleBuilder) Build(name string, desc string) { }) } - // If using an rsp file copy it into the sbox directory. - if rspFilePath != nil { + // If using rsp files copy them and their contents into the sbox directory with + // the appropriate path mappings. + for _, rspFile := range rspFiles { command.RspFiles = append(command.RspFiles, &sbox_proto.RspFile{ - File: proto.String(rspFilePath.String()), + File: proto.String(rspFile.file.String()), // These have to match the logic in sboxPathForInputRel PathMappings: []*sbox_proto.PathMapping{ { @@ -641,9 +632,9 @@ func (r *RuleBuilder) Build(name string, desc string) { remoteInputs = append(remoteInputs, inputs...) remoteInputs = append(remoteInputs, tools...) - if rspFilePath != nil { - remoteInputs = append(remoteInputs, rspFilePath) - remoteRspFiles = append(remoteRspFiles, rspFilePath) + for _, rspFile := range rspFiles { + remoteInputs = append(remoteInputs, rspFile.file) + remoteRspFiles = append(remoteRspFiles, rspFile.file) } if len(remoteInputs) > 0 { @@ -673,12 +664,24 @@ func (r *RuleBuilder) Build(name string, desc string) { implicitOutputs := outputs[1:] var rspFile, rspFileContent string - if rspFilePath != nil { - rspFile = rspFilePath.String() + var rspFileInputs Paths + if len(rspFiles) > 0 { + // The first rsp files uses Ninja's rsp file support for the rule + rspFile = rspFiles[0].file.String() // Use "$in" for rspFileContent to avoid duplicating the list of files in the dependency // list and in the contents of the rsp file. Inputs to the rule that are not in the // rsp file will be listed in Implicits instead of Inputs so they don't show up in "$in". rspFileContent = "$in" + rspFileInputs = append(rspFileInputs, rspFiles[0].paths...) + + for _, rspFile := range rspFiles[1:] { + // Any additional rsp files need an extra rule to write the file. + writeRspFileRule(r.ctx, rspFile.file, rspFile.paths) + // The main rule needs to depend on the inputs listed in the extra rsp file. + inputs = append(inputs, rspFile.paths...) + // The main rule needs to depend on the extra rsp file. + inputs = append(inputs, rspFile.file) + } } var pool blueprint.Pool @@ -729,8 +732,12 @@ type RuleBuilderCommand struct { depFiles WritablePaths tools Paths packagedTools []PackagingSpec - rspFileInputs Paths - rspFile WritablePath + rspFiles []rspFileAndPaths +} + +type rspFileAndPaths struct { + file WritablePath + paths Paths } func (c *RuleBuilderCommand) addInput(path Path) string { @@ -1174,22 +1181,19 @@ func (c *RuleBuilderCommand) FlagWithDepFile(flag string, path WritablePath) *Ru return c.Text(flag + c.PathForOutput(path)) } -// FlagWithRspFileInputList adds the specified flag and path to an rspfile to the command line, with no separator -// between them. The paths will be written to the rspfile. If sbox is enabled, the rspfile must -// be outside the sbox directory. +// FlagWithRspFileInputList adds the specified flag and path to an rspfile to the command line, with +// no separator between them. The paths will be written to the rspfile. If sbox is enabled, the +// rspfile must be outside the sbox directory. The first use of FlagWithRspFileInputList in any +// RuleBuilderCommand of a RuleBuilder will use Ninja's rsp file support for the rule, additional +// uses will result in an auxiliary rules to write the rspFile contents. func (c *RuleBuilderCommand) FlagWithRspFileInputList(flag string, rspFile WritablePath, paths Paths) *RuleBuilderCommand { - if c.rspFileInputs != nil { - panic("FlagWithRspFileInputList cannot be called if rsp file inputs have already been provided") - } - // Use an empty slice if paths is nil, the non-nil slice is used as an indicator that the rsp file must be // generated. if paths == nil { paths = Paths{} } - c.rspFileInputs = paths - c.rspFile = rspFile + c.rspFiles = append(c.rspFiles, rspFileAndPaths{rspFile, paths}) if c.rule.sbox { if _, isRel, _ := maybeRelErr(c.rule.outDir.String(), rspFile.String()); isRel { diff --git a/android/rule_builder_test.go b/android/rule_builder_test.go index 3df900f88..9c5ca4148 100644 --- a/android/rule_builder_test.go +++ b/android/rule_builder_test.go @@ -489,8 +489,9 @@ type testRuleBuilderModule struct { properties struct { Srcs []string - Restat bool - Sbox bool + Restat bool + Sbox bool + Sbox_inputs bool } } @@ -499,9 +500,15 @@ func (t *testRuleBuilderModule) GenerateAndroidBuildActions(ctx ModuleContext) { out := PathForModuleOut(ctx, "gen", ctx.ModuleName()) outDep := PathForModuleOut(ctx, "gen", ctx.ModuleName()+".d") outDir := PathForModuleOut(ctx, "gen") + rspFile := PathForModuleOut(ctx, "rsp") + rspFile2 := PathForModuleOut(ctx, "rsp2") + rspFileContents := PathsForSource(ctx, []string{"rsp_in"}) + rspFileContents2 := PathsForSource(ctx, []string{"rsp_in2"}) manifestPath := PathForModuleOut(ctx, "sbox.textproto") - testRuleBuilder_Build(ctx, in, out, outDep, outDir, manifestPath, t.properties.Restat, t.properties.Sbox) + testRuleBuilder_Build(ctx, in, out, outDep, outDir, manifestPath, t.properties.Restat, + t.properties.Sbox, t.properties.Sbox_inputs, rspFile, rspFileContents, + rspFile2, rspFileContents2) } type testRuleBuilderSingleton struct{} @@ -515,18 +522,35 @@ func (t *testRuleBuilderSingleton) GenerateBuildActions(ctx SingletonContext) { out := PathForOutput(ctx, "singleton/gen/baz") outDep := PathForOutput(ctx, "singleton/gen/baz.d") outDir := PathForOutput(ctx, "singleton/gen") + rspFile := PathForOutput(ctx, "singleton/rsp") + rspFile2 := PathForOutput(ctx, "singleton/rsp2") + rspFileContents := PathsForSource(ctx, []string{"rsp_in"}) + rspFileContents2 := PathsForSource(ctx, []string{"rsp_in2"}) manifestPath := PathForOutput(ctx, "singleton/sbox.textproto") - testRuleBuilder_Build(ctx, Paths{in}, out, outDep, outDir, manifestPath, true, false) + testRuleBuilder_Build(ctx, Paths{in}, out, outDep, outDir, manifestPath, true, false, false, + rspFile, rspFileContents, rspFile2, rspFileContents2) } -func testRuleBuilder_Build(ctx BuilderContext, in Paths, out, outDep, outDir, manifestPath WritablePath, restat, sbox bool) { +func testRuleBuilder_Build(ctx BuilderContext, in Paths, out, outDep, outDir, manifestPath WritablePath, + restat, sbox, sboxInputs bool, + rspFile WritablePath, rspFileContents Paths, rspFile2 WritablePath, rspFileContents2 Paths) { + rule := NewRuleBuilder(pctx, ctx) if sbox { rule.Sbox(outDir, manifestPath) + if sboxInputs { + rule.SandboxInputs() + } } - rule.Command().Tool(PathForSource(ctx, "cp")).Inputs(in).Output(out).ImplicitDepFile(outDep) + rule.Command(). + Tool(PathForSource(ctx, "cp")). + Inputs(in). + Output(out). + ImplicitDepFile(outDep). + FlagWithRspFileInputList("@", rspFile, rspFileContents). + FlagWithRspFileInputList("@", rspFile2, rspFileContents2) if restat { rule.Restat() @@ -557,6 +581,12 @@ func TestRuleBuilder_Build(t *testing.T) { srcs: ["bar"], sbox: true, } + rule_builder_test { + name: "foo_sbox_inputs", + srcs: ["bar"], + sbox: true, + sbox_inputs: true, + } ` result := GroupFixturePreparers( @@ -565,7 +595,10 @@ func TestRuleBuilder_Build(t *testing.T) { fs.AddToFixture(), ).RunTest(t) - check := func(t *testing.T, params TestingBuildParams, wantCommand, wantOutput, wantDepfile string, wantRestat bool, extraImplicits, extraCmdDeps []string) { + check := func(t *testing.T, params TestingBuildParams, rspFile2Params TestingBuildParams, + wantCommand, wantOutput, wantDepfile, wantRspFile, wantRspFile2 string, + wantRestat bool, extraImplicits, extraCmdDeps []string) { + t.Helper() command := params.RuleParams.Command re := regexp.MustCompile(" # hash of input list: [a-z0-9]*$") @@ -578,9 +611,19 @@ func TestRuleBuilder_Build(t *testing.T) { AssertBoolEquals(t, "RuleParams.Restat", wantRestat, params.RuleParams.Restat) + wantInputs := []string{"rsp_in"} + AssertArrayString(t, "Inputs", wantInputs, params.Inputs.Strings()) + wantImplicits := append([]string{"bar"}, extraImplicits...) + // The second rsp file and the files listed in it should be in implicits + wantImplicits = append(wantImplicits, "rsp_in2", wantRspFile2) AssertPathsRelativeToTopEquals(t, "Implicits", wantImplicits, params.Implicits) + wantRspFileContent := "$in" + AssertStringEquals(t, "RspfileContent", wantRspFileContent, params.RuleParams.RspfileContent) + + AssertStringEquals(t, "Rspfile", wantRspFile, params.RuleParams.Rspfile) + AssertPathRelativeToTopEquals(t, "Output", wantOutput, params.Output) if len(params.ImplicitOutputs) != 0 { @@ -592,18 +635,42 @@ func TestRuleBuilder_Build(t *testing.T) { if params.Deps != blueprint.DepsGCC { t.Errorf("want Deps = %q, got %q", blueprint.DepsGCC, params.Deps) } + + rspFile2Content := ContentFromFileRuleForTests(t, rspFile2Params) + AssertStringEquals(t, "rspFile2 content", "rsp_in2\n", rspFile2Content) } t.Run("module", func(t *testing.T) { outFile := "out/soong/.intermediates/foo/gen/foo" - check(t, result.ModuleForTests("foo", "").Rule("rule").RelativeToTop(), - "cp bar "+outFile, - outFile, outFile+".d", true, nil, nil) + rspFile := "out/soong/.intermediates/foo/rsp" + rspFile2 := "out/soong/.intermediates/foo/rsp2" + module := result.ModuleForTests("foo", "") + check(t, module.Rule("rule").RelativeToTop(), module.Output(rspFile2).RelativeToTop(), + "cp bar "+outFile+" @"+rspFile+" @"+rspFile2, + outFile, outFile+".d", rspFile, rspFile2, true, nil, nil) }) t.Run("sbox", func(t *testing.T) { outDir := "out/soong/.intermediates/foo_sbox" outFile := filepath.Join(outDir, "gen/foo_sbox") depFile := filepath.Join(outDir, "gen/foo_sbox.d") + rspFile := filepath.Join(outDir, "rsp") + rspFile2 := filepath.Join(outDir, "rsp2") + manifest := filepath.Join(outDir, "sbox.textproto") + sbox := filepath.Join("out", "soong", "host", result.Config.PrebuiltOS(), "bin/sbox") + sandboxPath := shared.TempDirForOutDir("out/soong") + + cmd := `rm -rf ` + outDir + `/gen && ` + + sbox + ` --sandbox-path ` + sandboxPath + ` --manifest ` + manifest + module := result.ModuleForTests("foo_sbox", "") + check(t, module.Output("gen/foo_sbox").RelativeToTop(), module.Output(rspFile2).RelativeToTop(), + cmd, outFile, depFile, rspFile, rspFile2, false, []string{manifest}, []string{sbox}) + }) + t.Run("sbox_inputs", func(t *testing.T) { + outDir := "out/soong/.intermediates/foo_sbox_inputs" + outFile := filepath.Join(outDir, "gen/foo_sbox_inputs") + depFile := filepath.Join(outDir, "gen/foo_sbox_inputs.d") + rspFile := filepath.Join(outDir, "rsp") + rspFile2 := filepath.Join(outDir, "rsp2") manifest := filepath.Join(outDir, "sbox.textproto") sbox := filepath.Join("out", "soong", "host", result.Config.PrebuiltOS(), "bin/sbox") sandboxPath := shared.TempDirForOutDir("out/soong") @@ -611,13 +678,18 @@ func TestRuleBuilder_Build(t *testing.T) { cmd := `rm -rf ` + outDir + `/gen && ` + sbox + ` --sandbox-path ` + sandboxPath + ` --manifest ` + manifest - check(t, result.ModuleForTests("foo_sbox", "").Output("gen/foo_sbox").RelativeToTop(), - cmd, outFile, depFile, false, []string{manifest}, []string{sbox}) + module := result.ModuleForTests("foo_sbox_inputs", "") + check(t, module.Output("gen/foo_sbox_inputs").RelativeToTop(), module.Output(rspFile2).RelativeToTop(), + cmd, outFile, depFile, rspFile, rspFile2, false, []string{manifest}, []string{sbox}) }) t.Run("singleton", func(t *testing.T) { outFile := filepath.Join("out/soong/singleton/gen/baz") - check(t, result.SingletonForTests("rule_builder_test").Rule("rule").RelativeToTop(), - "cp bar "+outFile, outFile, outFile+".d", true, nil, nil) + rspFile := filepath.Join("out/soong/singleton/rsp") + rspFile2 := filepath.Join("out/soong/singleton/rsp2") + singleton := result.SingletonForTests("rule_builder_test") + check(t, singleton.Rule("rule").RelativeToTop(), singleton.Output(rspFile2).RelativeToTop(), + "cp bar "+outFile+" @"+rspFile+" @"+rspFile2, + outFile, outFile+".d", rspFile, rspFile2, true, nil, nil) }) } diff --git a/java/java_test.go b/java/java_test.go index 13b3e2a90..5a130448f 100644 --- a/java/java_test.go +++ b/java/java_test.go @@ -628,7 +628,7 @@ prebuilt_stubs_sources { } t.Run("empty/missing directory", func(t *testing.T) { - test(t, "empty-directory", []string{}) + test(t, "empty-directory", nil) }) t.Run("non-empty set of sources", func(t *testing.T) { From 5bedfa2d45fe1186485ada0729cac0a86bccfb4e Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 23 Mar 2021 17:07:14 -0700 Subject: [PATCH 6/7] Simplify lint rules using improved RuleBuilder rsp support With the improved RuleBuilder rsp support a manual resources.list file is not necessary, use FlagWithRspFileInputList instead. The switch to RBE support in RuleBuilder in Iab4e09d961891ef182643583d4d456e413bc5e39 obsoleted tracking remoteInputs and remoteRSPInputs, remove them. writeLintProjectXML was written to allow it to be applied to a separate rule than the one that ran lint, but it is not used that way. Using the same rule for both means that manual tracking of the input dependencies described by the project.xml rule but read by the lint rule is not necessary, just treat them as inputs to the single rule. Test: m lint-check Test: m USE_RBE=true RBE_LINT=true RBE_LINT_EXEC_STRATEGY=remote lint-check Change-Id: If1827b9dede3ebcd0792b6b4b8114d3199f6570b --- java/lint.go | 85 ++++++++-------------------------------------------- 1 file changed, 13 insertions(+), 72 deletions(-) diff --git a/java/lint.go b/java/lint.go index 938e2b0d7..475e8dc7c 100644 --- a/java/lint.go +++ b/java/lint.go @@ -182,11 +182,6 @@ type lintPaths struct { cacheDir android.WritablePath homeDir android.WritablePath srcjarDir android.WritablePath - - deps android.Paths - - remoteInputs android.Paths - remoteRSPInputs android.Paths } func lintRBEExecStrategy(ctx android.ModuleContext) string { @@ -194,39 +189,6 @@ func lintRBEExecStrategy(ctx android.ModuleContext) string { } func (l *linter) writeLintProjectXML(ctx android.ModuleContext, rule *android.RuleBuilder) lintPaths { - var deps android.Paths - var remoteInputs android.Paths - var remoteRSPInputs android.Paths - - // Paths passed to trackInputDependency will be added as dependencies of the rule that runs - // lint and passed as inputs to the remote execution proxy. - trackInputDependency := func(paths ...android.Path) { - deps = append(deps, paths...) - remoteInputs = append(remoteInputs, paths...) - } - - // Paths passed to trackRSPDependency will be added as dependencies of the rule that runs - // lint, but the RSP file will be used by the remote execution proxy to find the files so that - // it doesn't overflow command line limits. - trackRSPDependency := func(paths android.Paths, rsp android.Path) { - deps = append(deps, paths...) - remoteRSPInputs = append(remoteRSPInputs, rsp) - } - - var resourcesList android.WritablePath - if len(l.resources) > 0 { - // The list of resources may be too long to put on the command line, but - // we can't use the rsp file because it is already being used for srcs. - // Insert a second rule to write out the list of resources to a file. - resourcesList = android.PathForModuleOut(ctx, "resources.list") - resListRule := android.NewRuleBuilder(pctx, ctx) - resListRule.Command().Text("cp"). - FlagWithRspFileInputList("", resourcesList.ReplaceExtension(ctx, "rsp"), l.resources). - Output(resourcesList) - resListRule.Build("lint_resources_list", "lint resources list") - trackRSPDependency(l.resources, resourcesList) - } - projectXMLPath := android.PathForModuleOut(ctx, "lint", "project.xml") // Lint looks for a lint.xml file next to the project.xml file, give it one. configXMLPath := android.PathForModuleOut(ctx, "lint", "lint.xml") @@ -235,20 +197,6 @@ func (l *linter) writeLintProjectXML(ctx android.ModuleContext, rule *android.Ru srcJarDir := android.PathForModuleOut(ctx, "lint", "srcjars") srcJarList := zipSyncCmd(ctx, rule, srcJarDir, l.srcJars) - // TODO(ccross): this is a little fishy. The files extracted from the srcjars are referenced - // by the project.xml and used by the later lint rule, but the lint rule depends on the srcjars, - // not the extracted files. - trackRSPDependency(l.srcJars, srcJarList) - - // TODO(ccross): some of the files in l.srcs are generated sources and should be passed to - // lint separately. - srcsList := android.PathForModuleOut(ctx, "lint", "srcs.list") - srcsListRsp := android.PathForModuleOut(ctx, "lint-srcs.list.rsp") - rule.Command().Text("cp"). - FlagWithRspFileInputList("", srcsListRsp, l.srcs). - Output(srcsList) - trackRSPDependency(l.srcs, srcsList) - rule.Temporary(srcsList) cmd := rule.Command(). BuiltTool("lint-project-xml"). @@ -263,32 +211,31 @@ func (l *linter) writeLintProjectXML(ctx android.ModuleContext, rule *android.Ru cmd.Flag("--test") } if l.manifest != nil { - cmd.FlagWithArg("--manifest ", cmd.PathForInput(l.manifest)) - trackInputDependency(l.manifest) + cmd.FlagWithInput("--manifest ", l.manifest) } if l.mergedManifest != nil { - cmd.FlagWithArg("--merged_manifest ", cmd.PathForInput(l.mergedManifest)) - trackInputDependency(l.mergedManifest) + cmd.FlagWithInput("--merged_manifest ", l.mergedManifest) } - cmd.FlagWithInput("--srcs ", srcsList) + // TODO(ccross): some of the files in l.srcs are generated sources and should be passed to + // lint separately. + srcsList := android.PathForModuleOut(ctx, "lint-srcs.list") + cmd.FlagWithRspFileInputList("--srcs ", srcsList, l.srcs) cmd.FlagWithInput("--generated_srcs ", srcJarList) - if resourcesList != nil { - cmd.FlagWithInput("--resources ", resourcesList) + if len(l.resources) > 0 { + resourcesList := android.PathForModuleOut(ctx, "lint-resources.list") + cmd.FlagWithRspFileInputList("--resources ", resourcesList, l.resources) } if l.classes != nil { - cmd.FlagWithArg("--classes ", cmd.PathForInput(l.classes)) - trackInputDependency(l.classes) + cmd.FlagWithInput("--classes ", l.classes) } - cmd.FlagForEachArg("--classpath ", cmd.PathsForInputs(l.classpath)) - trackInputDependency(l.classpath...) + cmd.FlagForEachInput("--classpath ", l.classpath) - cmd.FlagForEachArg("--extra_checks_jar ", cmd.PathsForInputs(l.extraLintCheckJars)) - trackInputDependency(l.extraLintCheckJars...) + cmd.FlagForEachInput("--extra_checks_jar ", l.extraLintCheckJars) cmd.FlagWithArg("--root_dir ", "$PWD") @@ -309,11 +256,6 @@ func (l *linter) writeLintProjectXML(ctx android.ModuleContext, rule *android.Ru configXML: configXMLPath, cacheDir: cacheDir, homeDir: homeDir, - - deps: deps, - - remoteInputs: remoteInputs, - remoteRSPInputs: remoteRSPInputs, } } @@ -424,8 +366,7 @@ func (l *linter) lint(ctx android.ModuleContext) { Flag("--exitcode"). Flags(l.properties.Lint.Flags). Implicit(annotationsZipPath). - Implicit(apiVersionsXMLPath). - Implicits(lintPaths.deps) + Implicit(apiVersionsXMLPath) rule.Temporary(lintPaths.projectXML) rule.Temporary(lintPaths.configXML) From 7ee54ffd708252e085d17e64c8e89ba9ef19d105 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 23 Mar 2021 17:41:40 -0700 Subject: [PATCH 7/7] Replace ANDROID_SDK_HOME with ANDROID_PREFS_ROOT for metalava Fixes warnings: Warning: Using ANDROID_SDK_HOME for the location of the '.android' preferences location is deprecated, please use ANDROID_PREFS_ROOT instead. Support for ANDROID_SDK_HOME is deprecated and will be removed in 6.0 Test: m android_stubs_current Change-Id: Ie8721dcda0578c670dfc796675ba43cda16883f6 --- java/droiddoc.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/droiddoc.go b/java/droiddoc.go index 34f017dc5..0becb84e1 100644 --- a/java/droiddoc.go +++ b/java/droiddoc.go @@ -1209,7 +1209,7 @@ func metalavaCmd(ctx android.ModuleContext, rule *android.RuleBuilder, javaVersi rule.Command().Text("mkdir -p").Flag(homeDir.String()) cmd := rule.Command() - cmd.FlagWithArg("ANDROID_SDK_HOME=", homeDir.String()) + cmd.FlagWithArg("ANDROID_PREFS_ROOT=", homeDir.String()) if ctx.Config().UseRBE() && ctx.Config().IsEnvTrue("RBE_METALAVA") { rule.Remoteable(android.RemoteRuleSupports{RBE: true}) @@ -1234,7 +1234,7 @@ func metalavaCmd(ctx android.ModuleContext, rule *android.RuleBuilder, javaVersi RSPFiles: []string{implicitsRsp.String()}, ToolchainInputs: []string{config.JavaCmd(ctx).String()}, Platform: map[string]string{remoteexec.PoolKey: pool}, - EnvironmentVariables: []string{"ANDROID_SDK_HOME"}, + EnvironmentVariables: []string{"ANDROID_PREFS_ROOT"}, }).NoVarTemplate(ctx.Config().RBEWrapper())) }