From 643a3a079903bd882a7b23ecca9500a853ab23d9 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Fri, 8 Feb 2019 19:24:37 +0000 Subject: [PATCH 01/11] Maintain extra file attributes when creating a zip file. --- core/os/file/zip.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/core/os/file/zip.go b/core/os/file/zip.go index f94ebeda31..58461f6567 100644 --- a/core/os/file/zip.go +++ b/core/os/file/zip.go @@ -42,8 +42,12 @@ func ZIP(out io.Writer, in Path) error { if r == "." { r = in.Basename() } - - fw, err := w.Create(r) + h, err := zip.FileInfoHeader(info) + if err != nil { + return err + } + h.Name = r + fw, err := w.CreateHeader(h) if err != nil { return err } From e592ccec44af9d9ae0a27bd98341ea3d39478797 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Fri, 8 Feb 2019 19:25:28 +0000 Subject: [PATCH 02/11] Don't call the cleanup function on error. It's nil and causes a panic. --- gapis/trace/android/trace.go | 1 - 1 file changed, 1 deletion(-) diff --git a/gapis/trace/android/trace.go b/gapis/trace/android/trace.go index 834d74ceb0..c32336b9db 100644 --- a/gapis/trace/android/trace.go +++ b/gapis/trace/android/trace.go @@ -401,7 +401,6 @@ func (t *androidTracer) SetupTrace(ctx context.Context, o *service.TraceOptions) if len(o.GetUploadApplication()) > 0 { pkg, cleanup, err = t.InstallPackage(ctx, o) if err != nil { - cleanup() return ret, nil, err } } From f1b91843111420ae5dc53195a721ce34f111fbd7 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Fri, 8 Feb 2019 19:27:32 +0000 Subject: [PATCH 03/11] Allow the ABI to be nil in the layout resolvers. Return the host's binaries in case the ABI is nil. --- core/app/layout/layout.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/app/layout/layout.go b/core/app/layout/layout.go index 092de85d10..3148e66394 100644 --- a/core/app/layout/layout.go +++ b/core/app/layout/layout.go @@ -126,7 +126,7 @@ var osToDir = map[device.OSKind]string{ } func (l pkgLayout) Gapir(ctx context.Context, abi *device.ABI) (file.Path, error) { - if hostOS(ctx) == abi.OS { + if abi == nil || hostOS(ctx) == abi.OS { return l.root.Join(withExecutablePlatformSuffix("gapir", hostOS(ctx))), nil } return l.root.Join(osToDir[abi.OS], withExecutablePlatformSuffix("gapir", abi.OS)), nil @@ -141,7 +141,7 @@ func (l pkgLayout) GapidApk(ctx context.Context, abi *device.ABI) (file.Path, er } func (l pkgLayout) Library(ctx context.Context, lib LibraryType, abi *device.ABI) (file.Path, error) { - if hostOS(ctx) == abi.OS { + if abi == nil || hostOS(ctx) == abi.OS { return l.root.Join("lib", withLibraryPlatformSuffix(libTypeToName[lib], hostOS(ctx))), nil } return l.root.Join(osToDir[abi.OS], "lib", withLibraryPlatformSuffix(libTypeToName[lib], abi.OS)), nil @@ -361,7 +361,7 @@ func (l *ZipLayout) Gapit(ctx context.Context) (*zip.File, error) { // Gapir returns the path to the gapir binary in this layout. func (l *ZipLayout) Gapir(ctx context.Context, abi *device.ABI) (*zip.File, error) { - if l.os == abi.OS { + if abi == nil || l.os == abi.OS { return l.file(withExecutablePlatformSuffix("gapir", l.os)) } return l.file(osToDir[abi.OS] + "/" + withExecutablePlatformSuffix("gapir", abi.OS)) @@ -379,7 +379,7 @@ func (l *ZipLayout) GapidApk(ctx context.Context, abi *device.ABI) (*zip.File, e // Library returns the path to the requested library. func (l *ZipLayout) Library(ctx context.Context, lib LibraryType, abi *device.ABI) (*zip.File, error) { - if l.os == abi.OS { + if abi == nil || l.os == abi.OS { return l.file("lib/" + withLibraryPlatformSuffix(libTypeToName[lib], l.os)) } return l.file(osToDir[abi.OS] + "lib/" + withLibraryPlatformSuffix(libTypeToName[lib], abi.OS)) From c16227d906e522e6744ebbb4120439698a3e89f6 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Fri, 8 Feb 2019 19:28:15 +0000 Subject: [PATCH 04/11] Remove an unneeded error check. The error cannot possibly be non-nil at that point. --- gapis/trace/android/trace.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/gapis/trace/android/trace.go b/gapis/trace/android/trace.go index c32336b9db..cd87156b9e 100644 --- a/gapis/trace/android/trace.go +++ b/gapis/trace/android/trace.go @@ -423,9 +423,6 @@ func (t *androidTracer) SetupTrace(ctx context.Context, o *service.TraceOptions) match = re.FindStringSubmatch(o.GetUri()) if len(match) == 4 { - if err != nil { - return ret, nil, err - } packages, err := t.b.InstalledPackages(ctx) if err != nil { return ret, nil, err From 7c550cd9ee3557e6445393ac6e395d1e09cba0d0 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Fri, 8 Feb 2019 19:29:49 +0000 Subject: [PATCH 05/11] Allow gapit trace to trace APK files. If given a URI of the form `apk:` it will send the APK to the gapis to be installed on the device and traced. --- cmd/gapit/trace.go | 105 +++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 47 deletions(-) diff --git a/cmd/gapit/trace.go b/cmd/gapit/trace.go index 1d5240f15c..4c46f8c3f8 100644 --- a/cmd/gapit/trace.go +++ b/cmd/gapit/trace.go @@ -20,6 +20,7 @@ import ( "flag" "fmt" "io" + "io/ioutil" "os" "strings" "time" @@ -46,6 +47,8 @@ func init() { }) } +type target func(opts *service.TraceOptions) + func (verb *traceVerb) Run(ctx context.Context, flags flag.FlagSet) error { client, err := getGapis(ctx, verb.Gapis, GapirFlags{}) if err != nil { @@ -63,32 +66,26 @@ func (verb *traceVerb) Run(ctx context.Context, flags flag.FlagSet) error { traceURI = flags.Arg(0) } - var traceDevice *path.Device out := "capture.gfxtrace" - var port uint32 - var uri string + var target target if verb.Local.Port != 0 { serverInfo, err := client.GetServerInfo(ctx) if err != nil { return err } - traceDevice = serverInfo.GetServerLocalDevice() + traceDevice := serverInfo.GetServerLocalDevice() if traceDevice.GetID() == nil { return fmt.Errorf("The server was not started with a local device for tracing") } - port = uint32(verb.Local.Port) - } else { - type info struct { - uri string - device *path.Device - deviceName string - name string + target = func(opts *service.TraceOptions) { + opts.Device = traceDevice + opts.App = &service.TraceOptions_Port{ + Port: uint32(verb.Local.Port), + } } - var found []info - + } else { // Find the actual trace URI from all of the devices - devices, err := filterDevices(ctx, &verb.DeviceFlags, client) if err != nil { return err @@ -99,12 +96,32 @@ func (verb *traceVerb) Run(ctx context.Context, flags flag.FlagSet) error { } if len(devices) == 1 && strings.HasPrefix(traceURI, "port:") { - found = append(found, info{ - uri: traceURI, - device: devices[0], - name: "capture", - }) + target = func(opts *service.TraceOptions) { + opts.Device = devices[0] + opts.App = &service.TraceOptions_Uri{ + Uri: traceURI, + } + } + } else if len(devices) == 1 && strings.HasPrefix(traceURI, "apk:") { + data, err := ioutil.ReadFile(traceURI[4:]) + if err != nil { + return log.Errf(ctx, err, "Failed to read APK at %s", traceURI[4:]) + } + target = func(opts *service.TraceOptions) { + opts.Device = devices[0] + opts.App = &service.TraceOptions_UploadApplication{ + UploadApplication: data, + } + } } else { + type info struct { + uri string + device *path.Device + deviceName string + name string + } + var found []info + for _, dev := range devices { targets, err := client.FindTraceTargets(ctx, &service.FindTraceTargetsRequest{ Device: dev, @@ -137,28 +154,32 @@ func (verb *traceVerb) Run(ctx context.Context, flags flag.FlagSet) error { }) } } - } - if len(found) == 0 { - return fmt.Errorf("Could not find %+v to trace on any device", traceURI) - } + if len(found) == 0 { + return fmt.Errorf("Could not find %+v to trace on any device", traceURI) + } - if len(found) > 1 { - sb := strings.Builder{} - fmt.Fprintf(&sb, "Found %v candidates: \n", traceURI) - for i, f := range found { - if i == 0 || found[i-1].deviceName != f.deviceName { - fmt.Fprintf(&sb, " %v:\n", f.deviceName) + if len(found) > 1 { + sb := strings.Builder{} + fmt.Fprintf(&sb, "Found %v candidates: \n", traceURI) + for i, f := range found { + if i == 0 || found[i-1].deviceName != f.deviceName { + fmt.Fprintf(&sb, " %v:\n", f.deviceName) + } + fmt.Fprintf(&sb, " %v\n", f.uri) } - fmt.Fprintf(&sb, " %v\n", f.uri) + return log.Errf(ctx, nil, "%v", sb.String()) } - return log.Errf(ctx, nil, "%v", sb.String()) - } - fmt.Printf("Tracing %+v\n", found[0].uri) - out = found[0].name + ".gfxtrace" - uri = found[0].uri - traceDevice = found[0].device + fmt.Printf("Tracing %+v\n", found[0].uri) + out = found[0].name + ".gfxtrace" + target = func(opts *service.TraceOptions) { + opts.Device = found[0].device + opts.App = &service.TraceOptions_Uri{ + Uri: found[0].uri, + } + } + } } if verb.Out != "" { @@ -166,7 +187,6 @@ func (verb *traceVerb) Run(ctx context.Context, flags flag.FlagSet) error { } options := &service.TraceOptions{ - Device: traceDevice, Apis: []string{}, AdditionalCommandLineArgs: verb.AdditionalArgs, Cwd: verb.WorkingDir, @@ -186,16 +206,7 @@ func (verb *traceVerb) Run(ctx context.Context, flags flag.FlagSet) error { ServerLocalSavePath: out, PipeName: verb.PipeName, } - - if uri != "" { - options.App = &service.TraceOptions_Uri{ - uri, - } - } else { - options.App = &service.TraceOptions_Port{ - port, - } - } + target(options) switch verb.API { case "vulkan": From 274ef0d93e6bea08c7b65872a300218d1651fd3f Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Mon, 11 Feb 2019 10:38:17 +0000 Subject: [PATCH 06/11] Don't look for user input if `-for` is specified to gapit trace. This is how it used to work for automated scripts. --- cmd/gapit/trace.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/gapit/trace.go b/cmd/gapit/trace.go index 4c46f8c3f8..b4c51ac45f 100644 --- a/cmd/gapit/trace.go +++ b/cmd/gapit/trace.go @@ -236,7 +236,7 @@ func (verb *traceVerb) Run(ctx context.Context, flags flag.FlagSet) error { } log.I(ctx, "Trace Status %+v", status) - handlerInstalled := false + handlerInstalled := verb.For > 0 return task.Retry(ctx, 0, time.Second*3, func(ctx context.Context) (retry bool, err error) { status, err = handler.Event(ctx, service.TraceEvent_Status) From b023c0f2495376002f5ae299596aac6ed742be30 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Mon, 11 Feb 2019 10:46:53 +0000 Subject: [PATCH 07/11] When installing an APK, update the trace URI. This way the tracing logic for an newly installed app will "just work" as if the URI had been specified in the request. --- core/os/android/apk/analysis.go | 12 ++++++++++++ gapis/trace/android/trace.go | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/core/os/android/apk/analysis.go b/core/os/android/apk/analysis.go index 7933f2aa93..7eee5ded77 100644 --- a/core/os/android/apk/analysis.go +++ b/core/os/android/apk/analysis.go @@ -68,3 +68,15 @@ func engine(files []*zip.File) string { } return "" } + +func (i *Information) URI() string { + var uri string + if i.Action != "" { + uri = i.Action + ":" + } + uri += i.Package + if i.Activity != "" { + uri += "/" + i.Activity + } + return uri +} diff --git a/gapis/trace/android/trace.go b/gapis/trace/android/trace.go index cd87156b9e..f326d58539 100644 --- a/gapis/trace/android/trace.go +++ b/gapis/trace/android/trace.go @@ -346,6 +346,10 @@ func (t *androidTracer) InstallPackage(ctx context.Context, o *service.TraceOpti pkg.Uninstall(ctx) } } + + o.App = &service.TraceOptions_Uri{ + Uri: info.URI(), + } return pkg, cleanup, nil } From e23b3eee1cee822ceabdff2b80080844f105ca74 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Mon, 11 Feb 2019 10:48:18 +0000 Subject: [PATCH 08/11] Fix a case error in robot. --- test/robot/job/device.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/robot/job/device.go b/test/robot/job/device.go index d1127e9d29..e2d72b4e15 100644 --- a/test/robot/job/device.go +++ b/test/robot/job/device.go @@ -106,7 +106,7 @@ func (l *devices) get(ctx context.Context, info *device.Instance) (*Device, erro entry := l.find(ctx, info) if entry == nil { entry = &Device{ - Id: l.uniqueName(ctx, info.Id.ID().String()), + Id: l.uniqueName(ctx, info.ID.ID().String()), Information: info, } if err := l.ledger.Add(ctx, entry); err != nil { From ca71542805b617e674206d7fbe92c505bd6b064d Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Mon, 11 Feb 2019 10:49:13 +0000 Subject: [PATCH 09/11] Fix the extraction of the tools in robot for the new layout. --- test/robot/build/artifact.go | 6 ++++-- test/robot/replay/client.go | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/test/robot/build/artifact.go b/test/robot/build/artifact.go index dce9410fdf..7b2a046085 100644 --- a/test/robot/build/artifact.go +++ b/test/robot/build/artifact.go @@ -128,7 +128,9 @@ func (a *artifacts) get(ctx context.Context, id string, builderAbi *device.ABI) l := layout.NewZipLayout(zipFile, builderAbi.OS) toolSet := ToolSet{Abi: builderAbi, Host: new(HostToolSet)} - if toolSet.Host.Gapir, err = a.getID(ctx, l.Gapir, "gapir"); err != nil { + if toolSet.Host.Gapir, err = a.getID(ctx, func(ctx context.Context) (*zip.File, error) { + return l.Gapir(ctx, nil) + }, "gapir"); err != nil { return nil, err } if toolSet.Host.Gapis, err = a.getID(ctx, l.Gapis, "gapis"); err != nil { @@ -138,7 +140,7 @@ func (a *artifacts) get(ctx context.Context, id string, builderAbi *device.ABI) return nil, err } if toolSet.Host.VirtualSwapChainLib, err = a.getID(ctx, func(ctx context.Context) (*zip.File, error) { - return l.Library(ctx, layout.LibVirtualSwapChain) + return l.Library(ctx, layout.LibVirtualSwapChain, nil) }, "libVirtualSwapChain"); err != nil { return nil, err } diff --git a/test/robot/replay/client.go b/test/robot/replay/client.go index 68d5c0977f..14afe6ce7c 100644 --- a/test/robot/replay/client.go +++ b/test/robot/replay/client.go @@ -120,7 +120,7 @@ func doReplay(ctx context.Context, action string, in *Input, store *stash.Client if err != nil { return nil, err } - gapir, err := extractedLayout.Gapir(ctx) + gapir, err := extractedLayout.Gapir(ctx, nil) if err != nil { return nil, err } From 39dccb65d189bda0a12cade3f7b87406077e94db Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Mon, 11 Feb 2019 10:49:47 +0000 Subject: [PATCH 10/11] Tracing now needs gapis. --- test/robot/scheduler/trace.go | 1 + test/robot/trace/client.go | 8 ++++++++ test/robot/trace/trace.proto | 2 ++ 3 files changed, 11 insertions(+) diff --git a/test/robot/scheduler/trace.go b/test/robot/scheduler/trace.go index a034d489df..cdad0b31ad 100644 --- a/test/robot/scheduler/trace.go +++ b/test/robot/scheduler/trace.go @@ -33,6 +33,7 @@ func (s schedule) doTrace(ctx context.Context, subj *monitor.Subject, tools *bui input := &trace.Input{ Subject: subj.Id, Obb: subj.Obb, + Gapis: tools.Host.Gapis, Gapit: tools.Host.Gapit, GapidApk: androidTools.GapidApk, Package: s.pkg.Id, diff --git a/test/robot/trace/client.go b/test/robot/trace/client.go index c091a88515..111f0f2c90 100644 --- a/test/robot/trace/client.go +++ b/test/robot/trace/client.go @@ -119,6 +119,11 @@ func doTrace(ctx context.Context, action string, in *Input, store *stash.Client, return nil, err } + gapis, err := extractedLayout.Gapis(ctx) + if err != nil { + return nil, err + } + gapit, err := extractedLayout.Gapit(ctx) if err != nil { return nil, err @@ -137,6 +142,9 @@ func doTrace(ctx context.Context, action string, in *Input, store *stash.Client, if err := store.GetFile(ctx, in.Subject, subject); err != nil { return nil, err } + if err := store.GetFile(ctx, in.Gapis, gapis); err != nil { + return nil, err + } if err := store.GetFile(ctx, in.Gapit, gapit); err != nil { return nil, err } diff --git a/test/robot/trace/trace.proto b/test/robot/trace/trace.proto index 7188ee002c..bc8155612f 100644 --- a/test/robot/trace/trace.proto +++ b/test/robot/trace/trace.proto @@ -27,6 +27,8 @@ import "core/os/device/device.proto"; message Input { // Subject is the stash id of the trace subject. string subject = 1; + // Gapis is the stash id of the graphics server tool to use. + string gapis = 8; // Gapit is the stash id of the graphics analysis tool to use. string gapit = 2; // GapidApk is the stash id of the gapid.apk to use. From 595e7ba1befd723c6c4752b98f6d2ff439f5fcd2 Mon Sep 17 00:00:00 2001 From: Pascal Muetschard Date: Mon, 11 Feb 2019 10:50:08 +0000 Subject: [PATCH 11/11] Fix the robot invocation of `gapit trace`. --- test/robot/trace/client.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/robot/trace/client.go b/test/robot/trace/client.go index 111f0f2c90..1600c72d9f 100644 --- a/test/robot/trace/client.go +++ b/test/robot/trace/client.go @@ -155,12 +155,11 @@ func doTrace(ctx context.Context, action string, in *Input, store *stash.Client, params := []string{ "trace", "-out", tracefile.System(), - "-apk", subject.System(), "-for", traceTime.String(), "-disable-pcs", "-observe-frames", strconv.Itoa(observeEveryNthFrame), "-record-errors", - "-gapii-device", d.Instance().Serial, + "-serial", d.Instance().Serial, "-api", in.GetHints().GetAPI(), } @@ -174,9 +173,11 @@ func doTrace(ctx context.Context, action string, in *Input, store *stash.Client, defer func() { file.Remove(obb) }() - params = append(params, "-obb", obb.System()) + // TODO fix this + // params = append(params, "-obb", obb.System()) + return log.Errf(ctx, nil, "OBBs are currently not supported") } - cmd := shell.Command(gapit.System(), params...) + cmd := shell.Command(gapit.System(), append(params, "apk:"+subject.System())...) outBuf := &bytes.Buffer{} errBuf := &bytes.Buffer{} outputObj := &Output{}