rebaseline.py: use argparse command-line flags for more flexibility

rebaseline.py: use argparse command-line flags for more flexibility

--tests is the only mandatory argument.

If you used to run this:
rebaseline.py aaclip bigmatrix

Run this instead:
rebaseline.py --tests aaclip bigmatrix

That's the only change you NEED to make.

And then, if you WANT to specify --configs, --subdirs, etc. you CAN.

R=senorblanco@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=9348

[epoger, 2013-05-29 19:10:21]

Ready for review at patchset 1.

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode127
tools/rebaseline.py:127: print '# ' + expectations_subdir + ':'
These printlines are now prefixed with '#' so that, in case dry_run is turned
on, you could just execute stdout to actually run all the operations.

[Stephen White, 2013-05-29 19:47:37]

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode63
tools/rebaseline.py:63: raise Exception('at least one test must be specified')
Pardon my python-ignorance, but does raising an exception print a stack trace,
or just an error message? The latter would be preferable, unless this is not
reachable from the command line, in which case it's fine.

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode127
tools/rebaseline.py:127: print '# ' + expectations_subdir + ':'
On 2013/05/29 19:10:21, epoger wrote:
> These printlines are now prefixed with '#' so that, in case dry_run is turned
> on, you could just execute stdout to actually run all the operations.

Nice. :)

We might want to clean up the output a bit more, especially for the
failed-to-fetch cases (they're a bit verbose).

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode168
tools/rebaseline.py:168: '"--tests aaclip bigmatrix"')
Hmm.. does this mean you always need a --tests argument?  Is there any way to
get the old behaviour back with argparse? (ie., anything that's not an argument
is a test?)

If not, I guess it's ok, but you should probably post to skia-dev that it's
changed.

[Stephen White, 2013-05-29 19:48:19]

On 2013/05/29 19:47:37, Stephen White wrote:
> https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py
> File tools/rebaseline.py (right):
> 
> https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode63
> tools/rebaseline.py:63: raise Exception('at least one test must be specified')
> Pardon my python-ignorance, but does raising an exception print a stack trace,
> or just an error message? The latter would be preferable, unless this is not
> reachable from the command line, in which case it's fine.
> 
> https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode127
> tools/rebaseline.py:127: print '# ' + expectations_subdir + ':'
> On 2013/05/29 19:10:21, epoger wrote:
> > These printlines are now prefixed with '#' so that, in case dry_run is
turned
> > on, you could just execute stdout to actually run all the operations.
> 
> Nice. :)
> 
> We might want to clean up the output a bit more, especially for the
> failed-to-fetch cases (they're a bit verbose).
> 
> https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode168
> tools/rebaseline.py:168: '"--tests aaclip bigmatrix"')
> Hmm.. does this mean you always need a --tests argument?  Is there any way to
> get the old behaviour back with argparse? (ie., anything that's not an
argument
> is a test?)

Guh, read the patch before the description, obviously that's the new behaviour.
:)
> 
> If not, I guess it's ok, but you should probably post to skia-dev that it's
> changed.

[epoger, 2013-05-30 15:39:27]

Thanks, Stephen.  Whaddaya say?

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode63
tools/rebaseline.py:63: raise Exception('at least one test must be specified')
On 2013/05/29 19:47:38, Stephen White wrote:
> Pardon my python-ignorance, but does raising an exception print a stack trace,
> or just an error message? The latter would be preferable, unless this is not
> reachable from the command line, in which case it's fine.

Here's what it looked like as of patchset 1:


$ python tools/rebaseline.py 
Traceback (most recent call last):
  File "tools/rebaseline.py", line 171, in <module>
    subdirs=args.subdirs, dry_run=args.dry_run)
  File "tools/rebaseline.py", line 63, in __init__
    raise Exception('at least one test must be specified')
Exception: at least one test must be specified


I agree that it's ugly, but on the other hand it *does* give extra insight into
exactly where the problem was encountered.

I have added "required=True" to the parser.add_argument() call, so now it yields
this output:


$ python tools/rebaseline.py 
usage: rebaseline.py [-h] [--configs CONFIG [CONFIG ...]] [--dry_run]
                     [--subdirs SUBDIR [SUBDIR ...]] --tests TEST [TEST ...]
rebaseline.py: error: argument --tests is required

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode168
tools/rebaseline.py:168: '"--tests aaclip bigmatrix"')
On 2013/05/29 19:47:38, Stephen White wrote:
> Hmm.. does this mean you always need a --tests argument?  Is there any way to
> get the old behaviour back with argparse? (ie., anything that's not an
argument
> is a test?)
> 
> If not, I guess it's ok, but you should probably post to skia-dev that it's
> changed.

The problem is, what if someone calls it like this?

rebaseline.py --configs one two three

How do you know which arguments are in the list of configs, and which ones are
the "leftovers" to use as the list of tests?

rebaseline.py --configs one two --tests three

... now we know.

So, I would like to move callers over to using "optional" arguments rather than
"positional" arguments, as soon as possible.

Good news: now that --tests is a "required" "optional" argument (!), callers
using the old way will get a splash of cold water in their face:


$ python tools/rebaseline.py one two
usage: rebaseline.py [-h] [--configs CONFIG [CONFIG ...]] [--dry_run]
                     [--subdirs SUBDIR [SUBDIR ...]] --tests TEST [TEST ...]
rebaseline.py: error: argument --tests is required


In addition, I will post to skia-team (and CC known rebaseliners directly) upon
commit.  Sound OK?

https://codereview.chromium.org/15675010/diff/5001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/15675010/diff/5001/tools/rebaseline.py#newcod...
tools/rebaseline.py:154: parser.add_argument('--configs', metavar='CONFIG',
nargs='+',
This means: if "--configs" is given, it MUST be followed by at least one value.

[Stephen White, 2013-05-30 15:44:27]

LGTM

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode63
tools/rebaseline.py:63: raise Exception('at least one test must be specified')
On 2013/05/30 15:39:28, epoger wrote:
> On 2013/05/29 19:47:38, Stephen White wrote:
> > Pardon my python-ignorance, but does raising an exception print a stack
trace,
> > or just an error message? The latter would be preferable, unless this is not
> > reachable from the command line, in which case it's fine.
> 
> Here's what it looked like as of patchset 1:
> 
> 
> $ python tools/rebaseline.py 
> Traceback (most recent call last):
>   File "tools/rebaseline.py", line 171, in <module>
>     subdirs=args.subdirs, dry_run=args.dry_run)
>   File "tools/rebaseline.py", line 63, in __init__
>     raise Exception('at least one test must be specified')
> Exception: at least one test must be specified
> 
> 
> I agree that it's ugly, but on the other hand it *does* give extra insight
into
> exactly where the problem was encountered.
> 
> I have added "required=True" to the parser.add_argument() call, so now it
yields
> this output:
> 
> 
> $ python tools/rebaseline.py 
> usage: rebaseline.py [-h] [--configs CONFIG [CONFIG ...]] [--dry_run]
>                      [--subdirs SUBDIR [SUBDIR ...]] --tests TEST [TEST ...]
> rebaseline.py: error: argument --tests is required
> 

Much nicer, thanks!

https://codereview.chromium.org/15675010/diff/1/tools/rebaseline.py#newcode168
tools/rebaseline.py:168: '"--tests aaclip bigmatrix"')
On 2013/05/30 15:39:28, epoger wrote:
> On 2013/05/29 19:47:38, Stephen White wrote:
> > Hmm.. does this mean you always need a --tests argument?  Is there any way
to
> > get the old behaviour back with argparse? (ie., anything that's not an
> argument
> > is a test?)
> > 
> > If not, I guess it's ok, but you should probably post to skia-dev that it's
> > changed.
> 
> The problem is, what if someone calls it like this?
> 
> rebaseline.py --configs one two three
> 
> How do you know which arguments are in the list of configs, and which ones are
> the "leftovers" to use as the list of tests?
> 
> rebaseline.py --configs one two --tests three
> 
> ... now we know.
> 
> So, I would like to move callers over to using "optional" arguments rather
than
> "positional" arguments, as soon as possible.
> 
> Good news: now that --tests is a "required" "optional" argument (!), callers
> using the old way will get a splash of cold water in their face:
> 
> 
> $ python tools/rebaseline.py one two
> usage: rebaseline.py [-h] [--configs CONFIG [CONFIG ...]] [--dry_run]
>                      [--subdirs SUBDIR [SUBDIR ...]] --tests TEST [TEST ...]
> rebaseline.py: error: argument --tests is required
> 
> 
> In addition, I will post to skia-team (and CC known rebaseliners directly)
upon
> commit.  Sound OK?

Sounds great!

[epoger, 2013-05-30 15:46:21]

Committed patchset #2 manually as r9348 (presubmit successful).