Add ability to replace all resources of a type#2063

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

+		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


-	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

-						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

+					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

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


Docs please

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

  }
 }

+message MultiResourceData {


Docs please

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

+
+		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

 	}
 }

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


Docs please

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

 	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

 	)
 }

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


Docs please

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


-	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

-						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


-	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

+	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

+// 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

 	}
 }
+
+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

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