New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 593198 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Make output placeholders like json outputs index-able by names.

Project Member Reported by st...@chromium.org, Mar 9 2016

Issue description

Currently, if we have multiple placeholders of the same modules (eg: json.output) in a step, the results are positional as a list in the step result (eg: json.output_all https://code.google.com/p/chromium/codesearch#chromium/infra/recipes-py/recipe_modules/json/example.py&l=42).

My proposal is to have them indexed by names. This will be more explicit/convenient and less error-prone. For example:

  result = api.python.inline(                                                    
      'foo',                                                                     
      """                                                                        
      import json                                                                
      import sys                                                                 
      with open(sys.argv[1], 'w') as f:                                          
        f.write(json.dumps([1, 2, 3]))                                           
      with open(sys.argv[2], 'w') as f:                                          
        f.write(json.dumps(['x', 'y', %s]))                                      
      """ % repr(FULLWIDTH_Z),                                                   
      args=[api.json.output(identifier='name1'), api.json.output(identifier='name2')],   
  )
  assert result.json.outputs['name1'] == [1, 2, 3]                                   
  assert result.json.outputs['name2'] == ['x', 'y', FULLWIDTH_Z] 


Sine a couple of recipes/recipe_modules already use json.output_all, my plan is to do it in three steps:
1. In recipe framework, add support for index, eg: json.outputs[name].
2. Update recipes/recipe_modules to use name index instead of positional, eg: use json.outputs instead json.output_all
3. In recipe framework, do the clean-up, eg: remove json.output_all.
 
Is there a reason that you wouldn't just put both of the json outputs into the same json? E.g.

with open(sys.argv[1], 'w') as f:
  f.write({
  'first': json.dumps([1, 2, 3]),
  'second': json.dumps(["x", "y"]),
  })

?

I might just be missing the larger use case for this.

Comment 2 by st...@chromium.org, Mar 9 2016

The use case I run into is in https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/compile.py

Currently, goma dumps its status into a json.output, while I want to dump some info from ninja into another json.output.
I don't think we want to combine both into one.

Comment 3 by st...@chromium.org, Mar 9 2016

Some internal recipes/recipe_modules also have two json.output as parameters in the command line like this.
Yep, I think there are going to be more cases like this soon too (like monitoring library pooping some data to one json file, while the actual script needs to produce some data into another file)
Yeah, ok. Makes sense, go for it! :)

Comment 6 by st...@chromium.org, Mar 22 2016

Now all the changes are ready.

Changes on recipe_engine side:
https://codereview.chromium.org/1785543004/
https://codereview.chromium.org/1773273003/

Changes on recipes/recipe_moduels side:
build/ : https://codereview.chromium.org/1820073003/
build_internal/ : https://chromereviews.googleplex.com/380557013/
depot_tools/ : no change is needed as I have checked.

Comment 7 by st...@chromium.org, Mar 22 2016

Cc: chanli@chromium.org kateso...@chromium.org lijeffrey@chromium.org
Summary: Make output placeholders like json outputs index-able by names. (was: Make placeholders like json outputs index-able by names.)
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 23 2016

The following revision refers to this bug:
  http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=85665

------------------------------------------------------------------
r85665 | stgao@google.com | 2016-03-23T19:55:25.850554Z

-----------------------------------------------------------------

Comment 10 by st...@chromium.org, Mar 23 2016

Status: Fixed (was: Assigned)

Sign in to add a comment