[codesize] Strip out debug symbols before bloaty runs
This adds a new CIPD package for a subset of binutils
(we want "strip" which removes debug symbols). I'm not sure
if this is the best approach because binutils appears to
have some extra dynamic library deps that might not work
when we update our swarming machines. We might need to bundle
those deps into the CIPD package.
Before: http://screen/98JoTV9endfUZQi
After: http://screen/6Ym4WszZ7kZbSbX
Change-Id: Ic97511c44c45c641b270e48593e9d0bebf659e3e
Bug: skia:13620
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/565901
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
diff --git a/infra/bots/assets/binutils_linux_x64/VERSION b/infra/bots/assets/binutils_linux_x64/VERSION
new file mode 100755
index 0000000..56a6051
--- /dev/null
+++ b/infra/bots/assets/binutils_linux_x64/VERSION
@@ -0,0 +1 @@
+1
\ No newline at end of file
diff --git a/infra/bots/assets/binutils_linux_x64/create.py b/infra/bots/assets/binutils_linux_x64/create.py
new file mode 100755
index 0000000..4516658
--- /dev/null
+++ b/infra/bots/assets/binutils_linux_x64/create.py
@@ -0,0 +1,60 @@
+#!/usr/bin/env python
+#
+# Copyright 2022 Google LLC
+#
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+
+"""Create the asset."""
+
+
+import argparse
+import glob
+import os
+import shutil
+import subprocess
+import sys
+
+FILE_DIR = os.path.dirname(os.path.abspath(__file__))
+INFRA_BOTS_DIR = os.path.realpath(os.path.join(FILE_DIR, os.pardir, os.pardir))
+sys.path.insert(0, INFRA_BOTS_DIR)
+import utils
+
+# https://packages.debian.org/buster/amd64/binutils-x86-64-linux-gnu/download
+# The older version from buster has fewer dynamic library dependencies.
+URL = 'https://ftp.debian.org/debian/pool/main/b/binutils/binutils-x86-64-linux-gnu_2.31.1-16_amd64.deb'
+SHA256 = 'c1da1cffff8a024b5eca0a6795558d9e0ec88fbd24fe059490dc665dc5cac92f'
+
+# https://packages.debian.org/buster/amd64/binutils-x86-64-linux-gnu/filelist
+to_copy = {
+ # If we need other files, we can add them to this mapping.
+ 'x86_64-linux-gnu-strip': 'strip',
+}
+
+
+def create_asset(target_dir):
+ with utils.tmp_dir():
+ subprocess.check_call(['wget', '--output-document=binutils.deb', '--quiet', URL])
+ output = subprocess.check_output(['sha256sum', 'binutils.deb'], encoding='utf-8')
+ actual_hash = output.split(' ')[0]
+ if actual_hash != SHA256:
+ raise Exception('SHA256 does not match (%s != %s)' % (actual_hash, SHA256))
+ # A .deb file is just a re-named .ar file...
+ subprocess.check_call(['ar', 'x', 'binutils.deb'])
+ # with a control.tar.xz and a data.tar.xz in it. The binaries are in the data one.
+ subprocess.check_call(['tar', '-xf', 'data.tar.xz'])
+ for (orig, copy) in to_copy.items():
+ shutil.copy(os.path.join('usr', 'bin', orig), os.path.join(target_dir, copy))
+
+
+def main():
+ parser = argparse.ArgumentParser()
+ parser.add_argument('--target_dir', '-t', required=True)
+ args = parser.parse_args()
+ create_asset(args.target_dir)
+
+
+if __name__ == '__main__':
+ main()
+
diff --git a/infra/bots/gen_tasks_logic/gen_tasks_logic.go b/infra/bots/gen_tasks_logic/gen_tasks_logic.go
index fd3eb85..6ae5e3e 100644
--- a/infra/bots/gen_tasks_logic/gen_tasks_logic.go
+++ b/infra/bots/gen_tasks_logic/gen_tasks_logic.go
@@ -1505,6 +1505,8 @@
// in this function; no changes to the task driver would be necessary.
"--binary_name", b.parts["binary_name"],
"--bloaty_cipd_version", bloatyCipdPkg.Version,
+ "--bloaty_binary", "bloaty/bloaty",
+ "--strip_binary", "binutils_linux_x64/strip",
"--repo", specs.PLACEHOLDER_REPO,
"--revision", specs.PLACEHOLDER_REVISION,
"--patch_issue", specs.PLACEHOLDER_ISSUE,
@@ -1515,6 +1517,7 @@
b.cache(CACHES_WORKDIR...)
b.cipd(CIPD_PKG_LUCI_AUTH)
b.asset("bloaty")
+ b.asset("binutils_linux_x64")
b.serviceAccount("skia-external-codesize@skia-swarming-bots.iam.gserviceaccount.com")
b.timeout(20 * time.Minute)
b.attempts(1)
diff --git a/infra/bots/recipe_modules/build/default.py b/infra/bots/recipe_modules/build/default.py
index 6d0c7db..2c8c6ae 100644
--- a/infra/bots/recipe_modules/build/default.py
+++ b/infra/bots/recipe_modules/build/default.py
@@ -191,6 +191,9 @@
extra_cflags.append('-O1')
if compiler != 'MSVC' and configuration == 'OptimizeForSize':
extra_cflags.append('-Oz')
+ # build IDs are required for Bloaty if we want to use strip to ignore debug symbols.
+ # https://github.com/google/bloaty/blob/master/doc/using.md#debugging-stripped-binaries
+ extra_ldflags.append('-Wl,--build-id=sha1')
if 'Exceptions' in extra_tokens:
extra_cflags.append('/EHsc')
diff --git a/infra/bots/recipe_modules/build/examples/full.expected/Build-Debian10-Clang-x86_64-OptimizeForSize.json b/infra/bots/recipe_modules/build/examples/full.expected/Build-Debian10-Clang-x86_64-OptimizeForSize.json
index 5532d5d..a3f5430 100644
--- a/infra/bots/recipe_modules/build/examples/full.expected/Build-Debian10-Clang-x86_64-OptimizeForSize.json
+++ b/infra/bots/recipe_modules/build/examples/full.expected/Build-Debian10-Clang-x86_64-OptimizeForSize.json
@@ -51,7 +51,7 @@
"[START_DIR]/cache/work/skia/bin/gn",
"gen",
"[START_DIR]/cache/work/skia/out/Build-Debian10-Clang-x86_64-OptimizeForSize/OptimizeForSize",
- "--args=cc=\"[START_DIR]/clang_linux/bin/clang\" cc_wrapper=\"[START_DIR]/ccache_linux/bin/ccache\" cxx=\"[START_DIR]/clang_linux/bin/clang++\" extra_cflags=[\"-B[START_DIR]/clang_linux/bin\", \"-DPLACEHOLDER_clang_linux_version=42\", \"-Oz\"] extra_ldflags=[\"-B[START_DIR]/clang_linux/bin\", \"-fuse-ld=lld\", \"-L[START_DIR]/clang_linux/lib\"] is_debug=false target_cpu=\"x86_64\" werror=true"
+ "--args=cc=\"[START_DIR]/clang_linux/bin/clang\" cc_wrapper=\"[START_DIR]/ccache_linux/bin/ccache\" cxx=\"[START_DIR]/clang_linux/bin/clang++\" extra_cflags=[\"-B[START_DIR]/clang_linux/bin\", \"-DPLACEHOLDER_clang_linux_version=42\", \"-Oz\"] extra_ldflags=[\"-B[START_DIR]/clang_linux/bin\", \"-fuse-ld=lld\", \"-Wl,--build-id=sha1\", \"-L[START_DIR]/clang_linux/lib\"] is_debug=false target_cpu=\"x86_64\" werror=true"
],
"cwd": "[START_DIR]/cache/work/skia",
"env": {
diff --git a/infra/bots/task_drivers/codesize/codesize.go b/infra/bots/task_drivers/codesize/codesize.go
index 6ec36f6..75557f8 100644
--- a/infra/bots/task_drivers/codesize/codesize.go
+++ b/infra/bots/task_drivers/codesize/codesize.go
@@ -23,6 +23,7 @@
"flag"
"fmt"
"os"
+ "path/filepath"
"strconv"
"time"
@@ -89,6 +90,8 @@
compileTaskNameNoPatch = flag.String("compile_task_name_no_patch", "", "Name of the *-NoPatch compile task that produced the binary to diff against (ignored when the task is not a tryjob).")
binaryName = flag.String("binary_name", "", "Name of the binary to analyze (e.g. \"dm\").")
bloatyCIPDVersion = flag.String("bloaty_cipd_version", "", "Version of the \"bloaty\" CIPD package used.")
+ bloatyBinary = flag.String("bloaty_binary", "", "Path to the bloaty binary.")
+ stripBinary = flag.String("strip_binary", "", "Path to the strip binary (part of binutils).")
output = flag.String("o", "", "If provided, dump a JSON blob of step data to the given file. Prints to stdout if '-' is given.")
local = flag.Bool("local", true, "True if running locally (as opposed to on the bots).")
@@ -97,6 +100,10 @@
ctx := td.StartRun(projectID, taskID, taskName, output, local)
defer td.EndRun(ctx)
+ if *bloatyBinary == "" || *stripBinary == "" {
+ td.Fatal(ctx, skerr.Fmt("Must specify --bloaty_binary and --strip_binary"))
+ }
+
// The repository state contains the commit hash and patch/patchset if available.
repoState, err := checkout.GetRepoState(checkoutFlags)
if err != nil {
@@ -117,7 +124,7 @@
gcsClient := gcsclient.New(store, gcsBucketName)
// Make a Gerrit client.
- gerrit, err := gerrit.NewGerrit(repoState.Server, httpClient)
+ gerritClient, err := gerrit.NewGerrit(repoState.Server, httpClient)
if err != nil {
td.Fatal(ctx, skerr.Wrap(err))
}
@@ -127,7 +134,7 @@
args := runStepsArgs{
repoState: repoState,
- gerrit: gerrit,
+ gerrit: gerritClient,
gitilesRepo: gitilesRepo,
gcsClient: gcsClient,
swarmingTaskID: os.Getenv("SWARMING_TASK_ID"),
@@ -137,7 +144,9 @@
compileTaskName: *compileTaskName,
compileTaskNameNoPatch: *compileTaskNameNoPatch,
binaryName: *binaryName,
+ bloatyPath: *bloatyBinary,
bloatyCIPDVersion: *bloatyCIPDVersion,
+ stripPath: *stripBinary,
}
if err := runSteps(ctx, args); err != nil {
@@ -159,6 +168,8 @@
compileTaskNameNoPatch string
binaryName string
bloatyCIPDVersion string
+ bloatyPath string
+ stripPath string
}
// runSteps runs the main steps of this task driver.
@@ -204,7 +215,7 @@
}
// Run Bloaty and capture its output.
- bloatyOutput, bloatyArgs, err := runBloaty(ctx, args.binaryName)
+ bloatyOutput, bloatyArgs, err := runBloaty(ctx, args.stripPath, args.bloatyPath, args.binaryName)
if err != nil {
return skerr.Wrap(err)
}
@@ -234,7 +245,7 @@
var bloatyDiffOutput string
if args.repoState.IsTryJob() {
// Diff the binary built at the current changelist/patchset vs. at tip-of-tree.
- bloatyDiffOutput, metadata.BloatyDiffArgs, err = runBloatyDiff(ctx, args.binaryName)
+ bloatyDiffOutput, metadata.BloatyDiffArgs, err = runBloatyDiff(ctx, args.stripPath, args.bloatyPath, args.binaryName)
if err != nil {
return skerr.Wrap(err)
}
@@ -274,32 +285,62 @@
}
// runBloaty runs Bloaty against the given binary and returns the Bloaty output in TSV format and
-// the Bloaty command-line arguments used.
-func runBloaty(ctx context.Context, binaryName string) (string, []string, error) {
- err := td.Do(ctx, td.Props("List files under $PWD/build"), func(ctx context.Context) error {
+// the Bloaty command-line arguments used. It uses the strip command to strip out debug symbols,
+// so they do not inflate the file size numbers.
+func runBloaty(ctx context.Context, stripPath, bloatyPath, binaryName string) (string, []string, error) {
+ binaryWithSymbols := filepath.Join("build", binaryName)
+ binaryNoSymbols := filepath.Join("build", binaryName+"_stripped")
+ err := td.Do(ctx, td.Props("Create stripped version of binary"), func(ctx context.Context) error {
runCmd := &exec.Command{
- Name: "ls",
- Args: []string{"build"},
+ Name: "cp",
+ Args: []string{binaryWithSymbols, binaryNoSymbols},
InheritEnv: true,
LogStdout: true,
LogStderr: true,
}
_, err := exec.RunCommand(ctx, runCmd)
- return err
+ if err != nil {
+ return skerr.Wrap(err)
+ }
+ runCmd = &exec.Command{
+ Name: stripPath,
+ Args: []string{binaryNoSymbols},
+ InheritEnv: true,
+ LogStdout: true,
+ LogStderr: true,
+ }
+ _, err = exec.RunCommand(ctx, runCmd)
+ if err != nil {
+ return skerr.Wrap(err)
+ }
+ runCmd = &exec.Command{
+ Name: "ls",
+ Args: []string{"-al", "build"},
+ InheritEnv: true,
+ LogStdout: true,
+ LogStderr: true,
+ }
+ _, err = exec.RunCommand(ctx, runCmd)
+ if err != nil {
+ return skerr.Wrap(err)
+ }
+
+ return nil
})
if err != nil {
- return "", []string{}, skerr.Wrap(err)
+ return "", nil, skerr.Wrap(err)
}
runCmd := &exec.Command{
- Name: "bloaty/bloaty",
+ Name: bloatyPath,
Args: []string{
- "build/" + binaryName,
+ binaryNoSymbols,
"-d",
"compileunits,symbols",
"-n",
"0",
"--tsv",
+ "--debug-file=" + binaryWithSymbols,
},
InheritEnv: true,
LogStdout: true,
@@ -312,7 +353,7 @@
bloatyOutput, err = exec.RunCommand(ctx, runCmd)
return err
}); err != nil {
- return "", []string{}, skerr.Wrap(err)
+ return "", nil, skerr.Wrap(err)
}
return bloatyOutput, runCmd.Args, nil
@@ -320,30 +361,60 @@
// runBloatyDiff invokes Bloaty to diff the given binary built at the current changelist/patchset
// vs. at tip of tree, and returns the plain-text Bloaty output and the command-line arguments
-// used.
-func runBloatyDiff(ctx context.Context, binaryName string) (string, []string, error) {
- err := td.Do(ctx, td.Props("List files under $PWD/build_nopatch"), func(ctx context.Context) error {
+// used. Like before, it strips the debug symbols out before computing that diff.
+func runBloatyDiff(ctx context.Context, stripPath, bloatyPath, binaryName string) (string, []string, error) {
+ // These were created from the runBloaty step
+ binaryWithPatchWithSymbols := filepath.Join("build", binaryName)
+ binaryWithPatchWithNoSymbols := filepath.Join("build", binaryName+"_stripped")
+ // These will be created next
+ binaryWithNoPatchWithSymbols := filepath.Join("build_nopatch", binaryName)
+ binaryWithNoPatchWithNoSymbols := filepath.Join("build_nopatch", binaryName+"_stripped")
+ err := td.Do(ctx, td.Props("Create stripped version of no_patch binary"), func(ctx context.Context) error {
runCmd := &exec.Command{
- Name: "ls",
- Args: []string{"build_nopatch"},
+ Name: "cp",
+ Args: []string{binaryWithNoPatchWithSymbols, binaryWithNoPatchWithNoSymbols},
InheritEnv: true,
LogStdout: true,
LogStderr: true,
}
_, err := exec.RunCommand(ctx, runCmd)
+ if err != nil {
+ return skerr.Wrap(err)
+ }
+ runCmd = &exec.Command{
+ Name: stripPath,
+ Args: []string{binaryWithNoPatchWithNoSymbols},
+ InheritEnv: true,
+ LogStdout: true,
+ LogStderr: true,
+ }
+ _, err = exec.RunCommand(ctx, runCmd)
+ if err != nil {
+ return skerr.Wrap(err)
+ }
+ runCmd = &exec.Command{
+ Name: "ls",
+ Args: []string{"-al", "build_nopatch"},
+ InheritEnv: true,
+ LogStdout: true,
+ LogStderr: true,
+ }
+ _, err = exec.RunCommand(ctx, runCmd)
return err
})
if err != nil {
- return "", []string{}, skerr.Wrap(err)
+ return "", nil, skerr.Wrap(err)
}
runCmd := &exec.Command{
- Name: "bloaty/bloaty",
+ Name: bloatyPath,
Args: []string{
- "build/" + binaryName,
- "--",
- "build_nopatch/" + binaryName,
+ binaryWithPatchWithNoSymbols,
+ "--debug-file=" + binaryWithPatchWithSymbols,
"-d", "compileunits,symbols", "-n", "0", "-s", "file",
+ "--",
+ binaryWithNoPatchWithNoSymbols,
+ "--debug-file=" + binaryWithNoPatchWithSymbols,
},
InheritEnv: true,
LogStdout: true,
@@ -355,7 +426,7 @@
bloatyOutput, err = exec.RunCommand(ctx, runCmd)
return err
}); err != nil {
- return "", []string{}, skerr.Wrap(err)
+ return "", nil, skerr.Wrap(err)
}
return bloatyOutput, runCmd.Args, nil
diff --git a/infra/bots/task_drivers/codesize/codesize_test.go b/infra/bots/task_drivers/codesize/codesize_test.go
index ed89a66..4914680 100644
--- a/infra/bots/task_drivers/codesize/codesize_test.go
+++ b/infra/bots/task_drivers/codesize/codesize_test.go
@@ -33,9 +33,6 @@
)
func TestRunSteps_PostSubmit_Success(t *testing.T) {
- // An empty inputPatch indicates this is a post-submit task.
- inputPatch := types.Patch{}
-
// The revision is assigned deterministically by the GitBuilder in test().
const (
expectedBloatyFileGCSPath = "2022/01/31/01/693abc06538769c662ca1871d347323b133a5d3c/Build-Debian10-Clang-x86_64-Release/dm.tsv"
@@ -54,12 +51,13 @@
"binary_name": "dm",
"bloaty_cipd_version": "1",
"bloaty_args": [
- "build/dm",
+ "build/dm_stripped",
"-d",
"compileunits,symbols",
"-n",
"0",
- "--tsv"
+ "--tsv",
+ "--debug-file=build/dm"
],
"patch_issue": "",
"patch_server": "",
@@ -70,18 +68,85 @@
"author": "test (test@google.com)",
"subject": "Fake commit subject"
}`
+ const expectedBloatyFileContents = "I'm a fake Bloaty output!"
- test(t, inputPatch, expectedBloatyFileGCSPath, "" /* =expectedBloatyDiffFileGCSPath */, expectedJSONMetadataFileGCSPath, expectedJSONMetadataFileContents)
+ // Make sure we use UTC instead of the system timezone.
+ fakeNow := time.Date(2022, time.January, 31, 2, 2, 3, 0, time.FixedZone("UTC+1", 60*60))
+
+ repoState := types.RepoState{
+ Repo: "https://skia.googlesource.com/skia.git",
+ }
+ mockGerrit, mockGitiles, repoState := setupMockGit(t, repoState)
+
+ commandCollector := exec.CommandCollector{}
+ // Mock "bloaty" invocations to output the appropriate contents to the fake stdout.
+ commandCollector.SetDelegateRun(func(ctx context.Context, cmd *exec.Command) error {
+ if filepath.Base(cmd.Name) == "bloaty" {
+ _, err := cmd.CombinedOutput.Write([]byte(expectedBloatyFileContents))
+ return err
+ }
+ // "ls" and any other commands directly executed by the task driver produce no mock outputs.
+ return nil
+ })
+
+ mockGCSClient := test_gcsclient.NewMockClient()
+ expectUpload(t, mockGCSClient, expectedBloatyFileGCSPath, expectedBloatyFileContents)
+ expectUpload(t, mockGCSClient, expectedJSONMetadataFileGCSPath, expectedJSONMetadataFileContents)
+
+ // Realistic but arbitrary arguments.
+ args := runStepsArgs{
+ repoState: repoState,
+ gerrit: mockGerrit.Gerrit,
+ gitilesRepo: mockGitiles,
+ gcsClient: mockGCSClient,
+ swarmingTaskID: "58dccb0d6a3f0411",
+ swarmingServer: "https://chromium-swarm.appspot.com",
+ taskID: "CkPp9ElAaEXyYWNHpXHU",
+ taskName: "CodeSize-dm-Debian10-Clang-x86_64-Release",
+ compileTaskName: "Build-Debian10-Clang-x86_64-Release",
+ compileTaskNameNoPatch: "Build-Debian10-Clang-x86_64-Release-NoPatch",
+ binaryName: "dm",
+ bloatyCIPDVersion: "1",
+ bloatyPath: "/path/to/bloaty",
+ stripPath: "/path/to/strip",
+ }
+
+ res := td.RunTestSteps(t, false, func(ctx context.Context) error {
+ ctx = now.TimeTravelingContext(fakeNow).WithContext(ctx)
+ ctx = td.WithExecRunFn(ctx, commandCollector.Run)
+
+ err := runSteps(ctx, args)
+ assert.NoError(t, err)
+ return err
+ })
+ require.Empty(t, res.Errors)
+ require.Empty(t, res.Exceptions)
+
+ // Filter out all Git commands.
+ var commands []*exec.Command
+ for _, c := range commandCollector.Commands() {
+ if filepath.Base(c.Name) != "git" {
+ commands = append(commands, c)
+ }
+ }
+
+ // We expect the following sequence of commands: "ls", "cp", "strip", "bloaty".
+ require.Len(t, commands, 4)
+ // We copy the binary and strip the debug symbols from the copy.
+ assertCommandEqual(t, commands[0], "cp", "build/dm", "build/dm_stripped")
+ assertCommandEqual(t, commands[1], "/path/to/strip", "build/dm_stripped")
+ // listing the contents of the directory with the binaries is useful for debugging.
+ assertCommandEqual(t, commands[2], "ls", "-al", "build")
+ // Assert that Bloaty was invoked on the stripped binary, using the original binary for the
+ // file names and other debug information.
+ assertCommandEqual(t, commands[3], "/path/to/bloaty",
+ "build/dm_stripped", "-d", "compileunits,symbols", "-n", "0", "--tsv", "--debug-file=build/dm")
+
+ // Assert that the .json and .tsv files were uploaded to GCS.
+ mockGCSClient.AssertExpectations(t)
}
func TestRunSteps_Tryjob_Success(t *testing.T) {
- inputPatch := types.Patch{
- Issue: "12345",
- PatchRepo: "https://skia.googlesource.com/skia.git",
- Patchset: "3",
- Server: "https://skia-review.googlesource.com",
- }
-
const (
expectedBloatyFileGCSPath = "2022/01/31/01/tryjob/12345/3/CkPp9ElAaEXyYWNHpXHU/Build-Debian10-Clang-x86_64-Release/dm.tsv"
expectedBloatyDiffFileGCSPath = "2022/01/31/01/tryjob/12345/3/CkPp9ElAaEXyYWNHpXHU/Build-Debian10-Clang-x86_64-Release/dm.diff.txt"
@@ -101,23 +166,26 @@
"binary_name": "dm",
"bloaty_cipd_version": "1",
"bloaty_args": [
- "build/dm",
+ "build/dm_stripped",
"-d",
"compileunits,symbols",
"-n",
"0",
- "--tsv"
+ "--tsv",
+ "--debug-file=build/dm"
],
"bloaty_diff_args": [
- "build/dm",
- "--",
- "build_nopatch/dm",
+ "build/dm_stripped",
+ "--debug-file=build/dm",
"-d",
"compileunits,symbols",
"-n",
"0",
"-s",
- "file"
+ "file",
+ "--",
+ "build_nopatch/dm_stripped",
+ "--debug-file=build_nopatch/dm"
],
"patch_issue": "12345",
"patch_server": "https://skia-review.googlesource.com",
@@ -128,37 +196,140 @@
"author": "test (test@google.com)",
"subject": "Fake commit subject"
}`
-
- test(t, inputPatch, expectedBloatyFileGCSPath, expectedBloatyDiffFileGCSPath, expectedJSONMetadataFileGCSPath, expectedJSONMetadataFileContents)
-}
-
-// test assumes a post-submit task when expectedBloatyDiffFileGCSPath is empty, or a tryjob when
-// set.
-func test(t *testing.T, patch types.Patch, expectedBloatyFileGCSPath, expectedBloatyDiffFileGCSPath, expectedJSONMetadataFileGCSPath, expectedJSONMetadataFileContents string) {
- isTryjob := expectedBloatyDiffFileGCSPath != ""
-
const expectedBloatyFileContents = "I'm a fake Bloaty output!"
const expectedBloatyDiffFileContents = "Fake Bloaty diff output over here!"
// Make sure we use UTC instead of the system timezone.
fakeNow := time.Date(2022, time.January, 31, 2, 2, 3, 0, time.FixedZone("UTC+1", 60*60))
- commitTimestamp := time.Date(2022, time.January, 30, 23, 59, 0, 0, time.UTC)
repoState := types.RepoState{
- Patch: patch,
- Repo: "https://skia.googlesource.com/skia.git",
+ Patch: types.Patch{
+ Issue: "12345",
+ PatchRepo: "https://skia.googlesource.com/skia.git",
+ Patchset: "3",
+ Server: "https://skia-review.googlesource.com",
+ },
+ Repo: "https://skia.googlesource.com/skia.git",
}
+ mockGerrit, mockGitiles, repoState := setupMockGit(t, repoState)
+
+ // Mock "bloaty" invocations.
+ commandCollector := exec.CommandCollector{}
+ commandCollector.SetDelegateRun(func(ctx context.Context, cmd *exec.Command) error {
+ if filepath.Base(cmd.Name) == "bloaty" {
+ // This argument indicates it's a binary diff invocation, see
+ // https://github.com/google/bloaty/blob/f01ea59bdda11708d74a3826c23d6e2db6c996f0/doc/using.md#size-diffs.
+ if util.In("--", cmd.Args) {
+ cmd.CombinedOutput.Write([]byte(expectedBloatyDiffFileContents))
+ } else {
+ cmd.CombinedOutput.Write([]byte(expectedBloatyFileContents))
+ }
+ return nil
+ }
+ // "ls" and any other commands directly executed by the task driver produce no mock outputs.
+ return nil
+ })
+
+ mockGCSClient := test_gcsclient.NewMockClient()
+ expectUpload(t, mockGCSClient, expectedBloatyFileGCSPath, expectedBloatyFileContents)
+ expectUpload(t, mockGCSClient, expectedBloatyDiffFileGCSPath, expectedBloatyDiffFileContents)
+ expectUpload(t, mockGCSClient, expectedJSONMetadataFileGCSPath, expectedJSONMetadataFileContents)
+
+ // Realistic but arbitrary arguments.
+ args := runStepsArgs{
+ repoState: repoState,
+ gerrit: mockGerrit.Gerrit,
+ gitilesRepo: mockGitiles,
+ gcsClient: mockGCSClient,
+ swarmingTaskID: "58dccb0d6a3f0411",
+ swarmingServer: "https://chromium-swarm.appspot.com",
+ taskID: "CkPp9ElAaEXyYWNHpXHU",
+ taskName: "CodeSize-dm-Debian10-Clang-x86_64-Release",
+ compileTaskName: "Build-Debian10-Clang-x86_64-Release",
+ compileTaskNameNoPatch: "Build-Debian10-Clang-x86_64-Release-NoPatch",
+ binaryName: "dm",
+ bloatyCIPDVersion: "1",
+ bloatyPath: "/path/to/bloaty",
+ stripPath: "/path/to/strip",
+ }
+
+ res := td.RunTestSteps(t, false, func(ctx context.Context) error {
+ ctx = now.TimeTravelingContext(fakeNow).WithContext(ctx)
+ ctx = td.WithExecRunFn(ctx, commandCollector.Run)
+
+ err := runSteps(ctx, args)
+ assert.NoError(t, err)
+ return err
+ })
+ require.Empty(t, res.Errors)
+ require.Empty(t, res.Exceptions)
+
+ // Filter out all Git commands.
+ var commands []*exec.Command
+ for _, c := range commandCollector.Commands() {
+ if filepath.Base(c.Name) != "git" {
+ commands = append(commands, c)
+ }
+ }
+
+ // We expect the following sequence of commands: "cp", "strip", "ls", "bloaty",
+ // "cp", "strip", "ls", "bloaty".
+ require.Len(t, commands, 8)
+
+ // We copy the binary and strip the debug symbols from the copy.
+ assertCommandEqual(t, commands[0], "cp", "build/dm", "build/dm_stripped")
+ assertCommandEqual(t, commands[1], "/path/to/strip", "build/dm_stripped")
+ // listing the contents of the directory with the binaries is useful for debugging.
+ assertCommandEqual(t, commands[2], "ls", "-al", "build")
+
+ // Assert that Bloaty was invoked on the binary with the right arguments
+ assertCommandEqual(t, commands[3], "/path/to/bloaty",
+ "build/dm_stripped", "-d", "compileunits,symbols", "-n", "0", "--tsv", "--debug-file=build/dm")
+
+ assertCommandEqual(t, commands[4], "cp", "build_nopatch/dm", "build_nopatch/dm_stripped")
+ assertCommandEqual(t, commands[5], "/path/to/strip", "build_nopatch/dm_stripped")
+ // Assert that "ls build_nopatch" was executed to list the contents of the directory with the
+ // binaries built by the compile task at tip-of-tree, for debugging purposes.
+ assertCommandEqual(t, commands[6], "ls", "-al", "build_nopatch")
+ // We perform a diff between the two binaries (the -- is how bloaty does that).
+ assertCommandEqual(t, commands[7], "/path/to/bloaty",
+ "build/dm_stripped", "--debug-file=build/dm",
+ "-d", "compileunits,symbols", "-n", "0", "-s", "file",
+ "--", "build_nopatch/dm_stripped", "--debug-file=build_nopatch/dm")
+
+ // Assert that the .json, .tsv and .diff.txt files were uploaded to GCS.
+ mockGCSClient.AssertExpectations(t)
+}
+
+func expectUpload(t *testing.T, client *test_gcsclient.GCSClient, path, contents string) {
+ client.On("SetFileContents", testutils.AnyContext, path, gcs.FILE_WRITE_OPTS_TEXT, mock.Anything).Run(func(args mock.Arguments) {
+ fileContents := string(args.Get(3).([]byte))
+ assert.Equal(t, contents, fileContents)
+ }).Return(nil)
+}
+
+func assertCommandEqual(t *testing.T, actualCmd *exec.Command, expectedCmd string, expectedArgs ...string) {
+ assert.Equal(t, expectedCmd, actualCmd.Name)
+ assert.Equal(t, expectedArgs, actualCmd.Args)
+}
+
+func setupMockGit(t *testing.T, repoState types.RepoState) (*gerrit_testutils.MockGerrit, *gitiles.Repo, types.RepoState) {
+ commitTimestamp := time.Date(2022, time.January, 30, 23, 59, 0, 0, time.UTC)
// Seed a fake Git repository.
gitBuilder := git_testutils.GitInit(t, context.Background())
- defer gitBuilder.Cleanup()
+ t.Cleanup(func() {
+ gitBuilder.Cleanup()
+ })
gitBuilder.Add(context.Background(), "README.md", "I'm a fake repository.")
repoState.Revision = gitBuilder.CommitMsgAt(context.Background(), "Fake commit subject", commitTimestamp)
// Mock a Gerrit client.
tmp, err := ioutil.TempDir("", "")
require.NoError(t, err)
- defer testutils.RemoveAll(t, tmp)
+ t.Cleanup(func() {
+ testutils.RemoveAll(t, tmp)
+ })
mockGerrit := gerrit_testutils.NewGerrit(t, tmp)
mockGerrit.MockGetIssueProperties(&gerrit.ChangeInfo{
Issue: 12345,
@@ -193,137 +364,5 @@
mockRepo := gitiles_testutils.NewMockRepo(t, gitBuilder.RepoUrl(), git.GitDir(gitBuilder.Dir()), urlMock)
mockRepo.MockGetCommit(context.Background(), repoState.Revision)
mockGitiles := gitiles.NewRepo(gitBuilder.RepoUrl(), urlMock.Client())
-
- // Mock "bloaty" invocations.
- commandCollector := exec.CommandCollector{}
- commandCollector.SetDelegateRun(func(ctx context.Context, cmd *exec.Command) error {
- if filepath.Base(cmd.Name) == "bloaty" {
- // This argument indicates it's a binary diff invocation, see
- // https://github.com/google/bloaty/blob/f01ea59bdda11708d74a3826c23d6e2db6c996f0/doc/using.md#size-diffs.
- if util.In("--", cmd.Args) {
- cmd.CombinedOutput.Write([]byte(expectedBloatyDiffFileContents))
- } else {
- cmd.CombinedOutput.Write([]byte(expectedBloatyFileContents))
- }
- return nil
- }
- // "ls" and any other commands directly executed by the task driver produce no mock outputs.
- return nil
- })
-
- mockGCSClient := test_gcsclient.NewMockClient()
- defer mockGCSClient.AssertExpectations(t)
-
- // Mock the GCS client call to upload the Bloaty output.
- mockGCSClient.On(
- "SetFileContents",
- testutils.AnyContext,
- expectedBloatyFileGCSPath,
- gcs.FILE_WRITE_OPTS_TEXT,
- mock.Anything,
- ).Run(func(args mock.Arguments) {
- fileContents := string(args.Get(3).([]byte))
- assert.Equal(t, expectedBloatyFileContents, fileContents)
- }).Return(nil)
-
- if isTryjob {
- // Mock the GCS client call to upload the Bloaty diff output.
- mockGCSClient.On(
- "SetFileContents",
- testutils.AnyContext,
- expectedBloatyDiffFileGCSPath,
- gcs.FILE_WRITE_OPTS_TEXT,
- mock.Anything,
- ).Run(func(args mock.Arguments) {
- fileContents := string(args.Get(3).([]byte))
- assert.Equal(t, expectedBloatyDiffFileContents, fileContents)
- }).Return(nil)
- }
-
- // Mock the GCS client call to upload the JSON metadata file.
- mockGCSClient.On(
- "SetFileContents",
- testutils.AnyContext,
- expectedJSONMetadataFileGCSPath,
- gcs.FILE_WRITE_OPTS_TEXT,
- mock.Anything,
- ).Run(func(args mock.Arguments) {
- fileContents := string(args.Get(3).([]byte))
- assert.Equal(t, expectedJSONMetadataFileContents, fileContents)
- }).Return(nil)
-
- // Realistic but arbitrary arguments.
- args := runStepsArgs{
- repoState: repoState,
- gerrit: mockGerrit.Gerrit,
- gitilesRepo: mockGitiles,
- gcsClient: mockGCSClient,
- swarmingTaskID: "58dccb0d6a3f0411",
- swarmingServer: "https://chromium-swarm.appspot.com",
- taskID: "CkPp9ElAaEXyYWNHpXHU",
- taskName: "CodeSize-dm-Debian10-Clang-x86_64-Release",
- compileTaskName: "Build-Debian10-Clang-x86_64-Release",
- compileTaskNameNoPatch: "Build-Debian10-Clang-x86_64-Release-NoPatch",
- binaryName: "dm",
- bloatyCIPDVersion: "1",
- }
-
- res := td.RunTestSteps(t, false, func(ctx context.Context) error {
- ctx = now.TimeTravelingContext(fakeNow).WithContext(ctx)
- ctx = td.WithExecRunFn(ctx, commandCollector.Run)
-
- return runSteps(ctx, args)
- })
-
- require.Empty(t, res.Errors)
- require.Empty(t, res.Exceptions)
-
- // Filter out all Git commands.
- var commands []*exec.Command
- for _, c := range commandCollector.Commands() {
- if filepath.Base(c.Name) != "git" {
- commands = append(commands, c)
- }
- }
-
- if isTryjob {
- // We expect the following sequence of commands: "ls", "bloaty", "ls", "bloaty".
- require.Len(t, commands, 4)
- } else {
- // We expect the following sequence of commands: "ls", "bloaty".
- require.Len(t, commands, 2)
- }
-
- // Assert that "ls build" was executed to list the contents of the directory with the binaries
- // built by the compile task, for debugging purposes.
- lsCmd := commands[0]
- assert.Equal(t, "ls", lsCmd.Name)
- assert.Equal(t, []string{"build"}, lsCmd.Args)
-
- // Assert that Bloaty was invoked with the expected arguments.
- bloatyCmd := commands[1]
- assert.Equal(t, "bloaty/bloaty", bloatyCmd.Name)
- assert.Equal(t, []string{"build/dm", "-d", "compileunits,symbols", "-n", "0", "--tsv"}, bloatyCmd.Args)
-
- if isTryjob {
- // Assert that "ls build_nopatch" was executed to list the contents of the directory with the
- // binaries built by the compile task at tip-of-tree, for debugging purposes.
- lsCmd = commands[2]
- assert.Equal(t, "ls", lsCmd.Name)
- assert.Equal(t, []string{"build_nopatch"}, lsCmd.Args)
-
- // Assert that Bloaty was invoked with the expected arguments.
- bloatyCmd = commands[3]
- assert.Equal(t, "bloaty/bloaty", bloatyCmd.Name)
- assert.Equal(t, []string{"build/dm", "--", "build_nopatch/dm",
- "-d", "compileunits,symbols", "-n", "0", "-s", "file"}, bloatyCmd.Args)
- }
-
- if isTryjob {
- // Assert that the .json, .tsv and .diff.txt files were uploaded to GCS.
- mockGCSClient.AssertNumberOfCalls(t, "SetFileContents", 3)
- } else {
- // Assert that the .json and .tsv files were uploaded to GCS.
- mockGCSClient.AssertNumberOfCalls(t, "SetFileContents", 2)
- }
+ return mockGerrit, mockGitiles, repoState
}
diff --git a/infra/bots/tasks.json b/infra/bots/tasks.json
index 80a316f..301239d 100755
--- a/infra/bots/tasks.json
+++ b/infra/bots/tasks.json
@@ -19584,6 +19584,11 @@
"version": "git_revision:34ecdc8775563915792e05ba9d921342808ae2dc"
},
{
+ "name": "skia/bots/binutils_linux_x64",
+ "path": "binutils_linux_x64",
+ "version": "version:1"
+ },
+ {
"name": "skia/bots/bloaty",
"path": "bloaty",
"version": "version:1"
@@ -19606,6 +19611,10 @@
"dm",
"--bloaty_cipd_version",
"version:1",
+ "--bloaty_binary",
+ "bloaty/bloaty",
+ "--strip_binary",
+ "binutils_linux_x64/strip",
"--repo",
"<(REPO)",
"--revision",
@@ -19649,6 +19658,11 @@
"version": "git_revision:34ecdc8775563915792e05ba9d921342808ae2dc"
},
{
+ "name": "skia/bots/binutils_linux_x64",
+ "path": "binutils_linux_x64",
+ "version": "version:1"
+ },
+ {
"name": "skia/bots/bloaty",
"path": "bloaty",
"version": "version:1"
@@ -19671,6 +19685,10 @@
"fm",
"--bloaty_cipd_version",
"version:1",
+ "--bloaty_binary",
+ "bloaty/bloaty",
+ "--strip_binary",
+ "binutils_linux_x64/strip",
"--repo",
"<(REPO)",
"--revision",
@@ -19714,6 +19732,11 @@
"version": "git_revision:34ecdc8775563915792e05ba9d921342808ae2dc"
},
{
+ "name": "skia/bots/binutils_linux_x64",
+ "path": "binutils_linux_x64",
+ "version": "version:1"
+ },
+ {
"name": "skia/bots/bloaty",
"path": "bloaty",
"version": "version:1"
@@ -19736,6 +19759,10 @@
"skottie_tool",
"--bloaty_cipd_version",
"version:1",
+ "--bloaty_binary",
+ "bloaty/bloaty",
+ "--strip_binary",
+ "binutils_linux_x64/strip",
"--repo",
"<(REPO)",
"--revision",
@@ -19779,6 +19806,11 @@
"version": "git_revision:34ecdc8775563915792e05ba9d921342808ae2dc"
},
{
+ "name": "skia/bots/binutils_linux_x64",
+ "path": "binutils_linux_x64",
+ "version": "version:1"
+ },
+ {
"name": "skia/bots/bloaty",
"path": "bloaty",
"version": "version:1"
@@ -19801,6 +19833,10 @@
"skottie_tool_cpu",
"--bloaty_cipd_version",
"version:1",
+ "--bloaty_binary",
+ "bloaty/bloaty",
+ "--strip_binary",
+ "binutils_linux_x64/strip",
"--repo",
"<(REPO)",
"--revision",
@@ -19844,6 +19880,11 @@
"version": "git_revision:34ecdc8775563915792e05ba9d921342808ae2dc"
},
{
+ "name": "skia/bots/binutils_linux_x64",
+ "path": "binutils_linux_x64",
+ "version": "version:1"
+ },
+ {
"name": "skia/bots/bloaty",
"path": "bloaty",
"version": "version:1"
@@ -19866,6 +19907,10 @@
"skottie_tool_gpu",
"--bloaty_cipd_version",
"version:1",
+ "--bloaty_binary",
+ "bloaty/bloaty",
+ "--strip_binary",
+ "binutils_linux_x64/strip",
"--repo",
"<(REPO)",
"--revision",
