Add ability to replace all resources of a type by apazylbe · Pull Request #2063 · google/gapid

apazylbe · 2018-07-13T16:38:57Z

Currently only works for shaders
UpdateResourceBinary should read the resource data from
standard input and output the modified resource data to
standard output.
Also modifying replace_resource to consume spir-v
disassembly to replace a single shader for consistency

I know MultiResourceData is not the best of names, but couldn't come up with anything better.
Suggestions are welcome.

ben-clayton · 2018-07-13T16:40:08Z

cmd/gapit/flags.go

+		Handle               string `help:"required. handle of the resource to replace"`
+		ResourcePath         string `help:"file path for the new resource"`
+		At                   int    `help:"command index to replace the resource(s) at"`
+		UpdateResourceBinary string `help:"binary to run for every shader; consumes resource data from standard input and writes to standard output"`


Is it just shaders or all resources?

ben-clayton · 2018-07-13T16:41:41Z

cmd/gapit/replace_resource.go


-	if verb.Handle == "" {
-		app.Usage(ctx, "-handle argument is required")
+	if verb.Handle == "" && verb.UpdateResourceBinary == "" {


Is it acceptable for both be specified? If not, perhaps this should be:

if (verb.Handle == "") != (verb.UpdateResourceBinary == "")

ben-clayton · 2018-07-13T16:48:35Z

cmd/gapit/replace_resource.go

-						return fmt.Errorf("Multiple resources matched: %s, %s", matchedResource.GetHandle(), v.GetHandle())
+			if verb.Handle != "" {
+				var matchedResource *service.Resource
+				for _, v := range types.GetResources() {


We already have a function to look for a resource by type and ID - perhaps we should add a more generic one to search using a predicate? We can then have a version that errors if theres more or less than one found.
Something like:

// FindAll returns all the resources that match the predicate f. func (r *Resources) FindAll(f func(api.ResourceType, Resource) bool) []*Resource { ... } // FindSingle returns the single resource that matches the predicate f. // If there are 0 or multiple resources found, FindSingle returns an error. func (r *Resources) FindSingle(f func(api.ResourceType, Resource) bool) (*Resource, error) { ... } // Find looks up a resource by type and identifier. func (r *Resources) Find(ty api.ResourceType, id id.ID) *Resource { r, _ := r.FindSingle(func(t api.ResourceType, r Resource) bool { return t == ty && r.ID.ID() == id }) return r }

ben-clayton · 2018-07-13T16:57:21Z

cmd/gapit/replace_resource.go

+					return log.Errf(ctx, err, "Could not read resource file %s", verb.ResourcePath)
+				}
+				resourceData = api.NewResourceData(&api.Shader{Type: api.ShaderType_Spirv, Source: string(newResourceBytes)})
+			} else if verb.UpdateResourceBinary != "" {


Consider a type switch instead of chained if statements:

switch { case verb.Handle != "": ... case verb.UpdateResourceBinary != "": ... }

ben-clayton · 2018-07-13T17:04:37Z

cmd/gapit/replace_resource.go

+	return nil
+}
+
+func (verb *replaceResourceVerb) getNewResourceData(ctx context.Context, resourceData *api.ResourceData) (string, error) {


Docs please

ben-clayton · 2018-07-13T17:13:56Z

gapis/api/service.proto

  }
 }

+message MultiResourceData {


Docs please

ben-clayton · 2018-07-13T17:14:54Z

gapis/resolve/set.go

+
+		cmdIdx := p.After.Indices[0]
+		// If we change resource data, subcommands do not affect this, so change
+		// the main comand.


ben-clayton · 2018-07-13T17:16:20Z

gapis/service/path/path.go

 	}
 }

+func (n *Command) ResourcesAfter(ids []*ID) *MultiResourceData {


Docs please

ben-clayton · 2018-07-13T17:16:29Z

gapis/service/path/path.go

 	fmt.Fprintf(f, "%v.resource-data<%x>", n.Parent(), n.ID)
 }

+func (n MultiResourceData) Format(f fmt.State, c rune) {


Docs please

ben-clayton · 2018-07-13T17:16:45Z

gapis/service/path/validate.go

 	)
 }

+func (n *MultiResourceData) Validate() error {


Docs please

ben-clayton · 2018-07-14T13:53:37Z

cmd/gapit/replace_resource.go


-	if verb.Handle == "" {
-		app.Usage(ctx, "-handle argument is required")
+	if verb.Handle == verb.UpdateResourceBinary {


Is this right?

Changed, my intention is to error if both are non empty or if both are empty.

ben-clayton · 2018-07-14T13:55:59Z

cmd/gapit/replace_resource.go

-						return fmt.Errorf("Multiple resources matched: %s, %s", matchedResource.GetHandle(), v.GetHandle())
+			switch {
+			case verb.Handle != "":
+				matchedResource, err := resources.FindSingle(func(t api.ResourceType, r service.Resource) bool {


This doesn't need to be in a for loop now, right?

Right, thanks!

ben-clayton

Please reply to review comments with a 'done' if they've been addressed - this way the reviewer knows if things have been missed, or not yet done.
Thanks!

ben-clayton · 2018-07-17T10:32:01Z

cmd/gapit/replace_resource.go


-	if verb.Handle == "" {
-		app.Usage(ctx, "-handle argument is required")
+	if (verb.Handle != "" && verb.UpdateResourceBinary != "") || (verb.Handle == "" && verb.UpdateResourceBinary == "") {


Which is equivalent to:
if (verb.Handle == "") != (verb.UpdateResourceBinary == "")

Can keep as-is if you find this clearer.

Sorry misread your original suggestion.
Updated, though I added : (verb.Handle == "") == (verb.UpdateResourceBinary == "")
since if condition should be true for error cases

Yup, just testing 😉

ben-clayton · 2018-07-17T10:32:54Z

cmd/gapit/replace_resource.go

+	switch {
+	case verb.Handle != "":
+		matchedResource, err := resources.FindSingle(func(t api.ResourceType, r service.Resource) bool {
+			return strings.Contains(r.GetHandle(), verb.Handle)


I think you need to check type here too?

ben-clayton · 2018-07-17T10:35:06Z

cmd/gapit/replace_resource.go

+// getNewResourceData runs the update resource binary on the old resource data
+// and returns the newly generated resource data
+func (verb *replaceResourceVerb) getNewResourceData(ctx context.Context, resourceData string) (string, error) {
+	updateCmd := exec.Command(verb.UpdateResourceBinary)


Did my example code not work for you?

It does. Sorry, totally missed that comment.

ben-clayton · 2018-07-17T10:36:11Z

gapis/api/resource.go

 	}
 }
+
+func NewMultiResourceData(resources []*ResourceData) *MultiResourceData {


Still missing docs.

ben-clayton

Thank you for the changes!

Currently only works for shaders UpdateResourceBinary should read the resource data from standard input and output the modified resource data to standard output. Also modifying replace_resource to consume spir-v disassembly to replace a single shader for consistency

ben-clayton reviewed Jul 13, 2018

View reviewed changes

apazylbe force-pushed the replace_all_shaders branch 2 times, most recently from 31c9204 to f10168d Compare July 13, 2018 20:53

ben-clayton reviewed Jul 14, 2018

View reviewed changes

apazylbe force-pushed the replace_all_shaders branch from f10168d to 9bd4a7b Compare July 16, 2018 14:26

ben-clayton reviewed Jul 17, 2018

View reviewed changes

apazylbe force-pushed the replace_all_shaders branch from 9bd4a7b to fc76a51 Compare July 17, 2018 14:02

ben-clayton approved these changes Jul 17, 2018

View reviewed changes

apazylbe added the kokoro:run label Jul 17, 2018

kokoro-team removed the kokoro:run label Jul 17, 2018

apazylbe force-pushed the replace_all_shaders branch from fc76a51 to 9b89cfb Compare July 17, 2018 15:20

apazylbe merged commit bca43f3 into google:master Jul 17, 2018

apazylbe deleted the replace_all_shaders branch July 17, 2018 18:26

Comments

Conversation

apazylbe commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-clayton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-clayton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants