Helper script which can source a bash script and dump environment as JSON.

Helper script which can source a bash script and dump environment as JSON.

NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223193

[iannucci, 2013-09-14 00:08:35]

lgtm % comments

https://chromiumcodereview.appspot.com/23618049/diff/1/build/env_dump.py
File build/env_dump.py (right):

https://chromiumcodereview.appspot.com/23618049/diff/1/build/env_dump.py#newc...
build/env_dump.py:23: options, args = parser.parse_args()
maybe just do

if options.output_json is None:
  parser.error("--output-json is required")

and ditch the stdout path.

https://chromiumcodereview.appspot.com/23618049/diff/1/build/env_dump.py#newc...
build/env_dump.py:30: return
let's do an else: instead of the early return

[Siva Chandra, 2013-09-14 00:20:43]

Addressed the comments and put it on the CQ.

[commit-bot: I haz the power, 2013-09-14 00:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/23618049/9001

[iannucci, 2013-09-14 00:23:31]

(one more comment)

https://chromiumcodereview.appspot.com/23618049/diff/9001/build/env_dump.py
File build/env_dump.py (right):

https://chromiumcodereview.appspot.com/23618049/diff/9001/build/env_dump.py#n...
build/env_dump.py:29: with open(options.output_json, 'w') as f:
Actually, I would do

if options.dump_mode:
  if args:
    parser.error("Cannot specify args with --dump-mode")
  ..

[Siva Chandra, 2013-09-14 00:29:32]

OK. Again on the CQ after the last comment.

[commit-bot: I haz the power, 2013-09-14 00:32:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sivachandra@chromium.org/23618049/15001

[commit-bot: I haz the power, 2013-09-14 01:02:26]

Change committed as 223193

[Isaac (away), 2013-09-15 08:32:24]

drive by comments -- mostly for the case where we replace current android env
dump script with the one.

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py
File build/env_dump.py (right):

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:34: envsetup_cmd = ' '.join(args)
' '.join(pipes.quote(a) for a in args) 

would be safer, in case args has any spaces or special characters.

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:37: '. %s; ./%s -d -f %s' % (envsetup_cmd, __file__,
options.output_json)]
maybe the '.' at the beginning of line 37 should be passed as part of args,
rather than hard coded. wdyt?

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:40: sys.exit('Error running %s and dumping env', envsetup_cmd)
AFAIK, sys.exit only takes one arg. Running new scripts through gpylint (or
maybe even the lenient chromium pylint) will catch this. Also might be cleaner
to use 

return '<str>' % envsetup_cmd

[Isaac (away), 2013-09-15 08:34:01]

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py
File build/env_dump.py (right):

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:34: envsetup_cmd = ' '.join(args)
Or: ' '.join(map(pipes.quote, args))

[iannucci, 2013-09-15 09:16:47]

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py
File build/env_dump.py (right):

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:34: envsetup_cmd = ' '.join(args)
On 2013/09/15 08:32:25, Isaac wrote:
> ' '.join(pipes.quote(a) for a in args) 
> 
> would be safer, in case args has any spaces or special characters.

+1, I missed this.

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:37: '. %s; ./%s -d -f %s' % (envsetup_cmd, __file__,
options.output_json)]
On 2013/09/15 08:32:25, Isaac wrote:
> maybe the '.' at the beginning of line 37 should be passed as part of args,
> rather than hard coded. wdyt?

-1 on this. I would argue that the '.' (sourcing the file under bash) is part of
the implementation of this particular script.  We could also make this script
(for example):
  * Interpret the file directly.
  * Copy the file elsewhere and append our special dumper command, then invoke
the copy.

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:40: sys.exit('Error running %s and dumping env', envsetup_cmd)
On 2013/09/15 08:32:25, Isaac wrote:
> AFAIK, sys.exit only takes one arg. Running new scripts through gpylint (or
> maybe even the lenient chromium pylint) will catch this. Also might be cleaner
> to use 
> 
> return '<str>' % envsetup_cmd

Oh, yeah, sys.exit takes an error code, not a string...

[Isaac (away), 2013-09-15 09:28:04]

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py
File build/env_dump.py (right):

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:37: '. %s; ./%s -d -f %s' % (envsetup_cmd, __file__,
options.output_json)]
On 2013/09/15 09:16:47, iannucci wrote:
> On 2013/09/15 08:32:25, Isaac wrote:
> > maybe the '.' at the beginning of line 37 should be passed as part of args,
> > rather than hard coded. wdyt?
> 
> -1 on this. I would argue that the '.' (sourcing the file under bash) is part
of
> the implementation of this particular script.  We could also make this script
> (for example):
>   * Interpret the file directly.
>   * Copy the file elsewhere and append our special dumper command, then invoke
> the copy.

:-D

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:40: sys.exit('Error running %s and dumping env', envsetup_cmd)
On 2013/09/15 09:16:47, iannucci wrote:
> On 2013/09/15 08:32:25, Isaac wrote:
> > AFAIK, sys.exit only takes one arg. Running new scripts through gpylint (or
> > maybe even the lenient chromium pylint) will catch this. Also might be
cleaner
> > to use 
> > 
> > return '<str>' % envsetup_cmd
> 
> Oh, yeah, sys.exit takes an error code, not a string...

It actually can take a string (siva showed me this in a previous code review),
which is shorthand for print string to stderr, exit 1. But my local python fails
on 2 args.

[Siva Chandra, 2013-09-15 09:39:13]

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py
File build/env_dump.py (right):

https://chromiumcodereview.appspot.com/23618049/diff/15001/build/env_dump.py#...
build/env_dump.py:40: sys.exit('Error running %s and dumping env', envsetup_cmd)
On 2013/09/15 09:16:47, iannucci wrote:
> On 2013/09/15 08:32:25, Isaac wrote:
> > AFAIK, sys.exit only takes one arg. Running new scripts through gpylint (or
> > maybe even the lenient chromium pylint) will catch this. Also might be
cleaner
> > to use 
> > 
> > return '<str>' % envsetup_cmd
> 
> Oh, yeah, sys.exit takes an error code, not a string...

sys.exit takes a string: http://docs.python.org/2.6/library/sys.html#sys.exit

This is clearly my bad, mixed up printf style syntax (',' for a '%') here.

Will send a CL to fix this and the above "quote" fix.