Add option to specify defaults configs to --config flag in gm. This makes it easier to run gm defau…

Add option to specify defaults configs to --config flag in gm. This makes it easier to run gm defaults configs with a delta. Also make ~ exclude a config.

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

[bsalomon, 2013-04-20 19:15:15]

If we're happy with ~ we could remove --excludeConfig. But maybe the bot scripts
are using it?

[reed1, 2013-04-22 12:25:03]

Can you show some example invocations using default and ~?

[bsalomon, 2013-04-22 12:54:30]

On 2013/04/22 12:25:03, reed1 wrote:

> Can you show some example invocations using default and ~?

Do you mean here or in the --help text? 

The set of all configs on my windows release build is:
8888 565 gpu|msaa16 msaa4 gpudebug angle anglemsaa16 pdf 

The default configs are:
8888 565 gpu msaa16 angle anglemsaa16 pdf

This runs all the defaults except pdf:
out\Release\gm --config defaults ~pdf

This substitutes in msaa4 for msaa16 in the default set:
out/Release/gm --config defaults ~msaa16 msaa4

[reed1, 2013-04-22 13:26:18]

lgtm

[bsalomon, 2013-04-22 15:18:02]

Add option to specify defaults configs to --config flag in gm. This makes it
easier to run gm defaults configs with a delta. Also make ~ exclude a config.

[bsalomon, 2013-04-22 15:22:00]

The latest patch does the following additional things:

1) Now if you pass --config you must fully construct the set of configs to run.
(The old behavior was to assume defaults until a config without ~ was found)

2) The help string gives an example of removing one config from the defaults and
adding another.

3) The set of configs that will be run is printed.

4) exit with -1 if the config set is empty.

Adding Elliot because I changed the output of GM and I don't know if this will
mess up self testing.

[epoger, 2013-04-23 16:07:48]

On 2013/04/22 15:22:00, bsalomon wrote:
> Adding Elliot because I changed the output of GM and I don't know if this will
> mess up self testing.

Thanks for asking... can you run gm/tests/run.sh and see if it fails?  It should
work, on either Mac or Linux.

If it does fail, running gm/tests/rebaseline.sh a few times should resolve it. 
(Currently, you have to run rebaseline.sh once per failing test in run.sh)

[epoger, 2013-04-23 16:29:40]

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp#newcode1203
gm/gmmain.cpp:1203: } else if (firstNonDefault.isEmpty()) {
you could just remember *a* nonDefault config, which would be slightly simpler:

else {
  oneNonDefault = gRec[i].fName;
}

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp#newcode1666
gm/gmmain.cpp:1666: if (exclude) {
OMG.  The caller can pass "~defaults" to exclude all of the default configs?  My
mind has just been blown.

It looks to me like, once a config has been excluded, it cannot be re-included
(excludeConfigs is applied at the end).  So, that means if the caller specifies
"~defaults angle", then angle may or may not run depending on whether it was in
the default set.  Right?

If so, that seems problematic.  Is there some way we can get all the behavior
you want without allowing "~defaults"?

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp#newcode1794
gm/gmmain.cpp:1794: SkString configStr("These configs will be run: ");
Maybe this should be guarded by if (FLAGS_verbose)?

Either way, this will definitely require running gm/tests/rebaseline.sh.

And you may want to consider adding some new tests gm/tests/run.sh, to make sure
that this new feature stays in working order as other changes are made...

[bsalomon, 2013-04-24 15:05:48]

Add option to specify defaults configs to --config flag in gm. This makes it
easier to run gm defaults configs with a delta. Also make ~ exclude a config.

[bsalomon, 2013-04-24 15:13:54]

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp#newcode1203
gm/gmmain.cpp:1203: } else if (firstNonDefault.isEmpty()) {
On 2013/04/23 16:29:40, epoger wrote:
> you could just remember *a* nonDefault config, which would be slightly
simpler:
> 
> else {
>   oneNonDefault = gRec[i].fName;
> }

Done.

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp#newcode1666
gm/gmmain.cpp:1666: if (exclude) {
On 2013/04/23 16:29:40, epoger wrote:
> OMG.  The caller can pass "~defaults" to exclude all of the default configs? 
My
> mind has just been blown.
> 
> It looks to me like, once a config has been excluded, it cannot be re-included
> (excludeConfigs is applied at the end).  So, that means if the caller
specifies
> "~defaults angle", then angle may or may not run depending on whether it was
in
> the default set.  Right?
> 
> If so, that seems problematic.  Is there some way we can get all the behavior
> you want without allowing "~defaults"?

The existing --excludeConfig works this way (exclude always overrides any
includes). I added ~defaults simply for orthogonality of  "~" and "defaults". It
isn't useful. I've barred ~defaults and added a comment to the help print about
~ overriding any non-~.

https://codereview.chromium.org/13842014/diff/12001/gm/gmmain.cpp#newcode1794
gm/gmmain.cpp:1794: SkString configStr("These configs will be run: ");
On 2013/04/23 16:29:40, epoger wrote:
> Maybe this should be guarded by if (FLAGS_verbose)?
> 
> Either way, this will definitely require running gm/tests/rebaseline.sh.
> 
> And you may want to consider adding some new tests gm/tests/run.sh, to make
sure
> that this new feature stays in working order as other changes are made...

If we're already printing each test as it runs in the non-verbose output then it
feels OK to me to print the configs over which those tests will be run.

txt outputs updated.

I didn't add new tests. TBH from where I sit these self-tests that rely on
specific output text feel like more of a burden than a benefit. I'm willing to
concede that I'm missing some info. But it feels fragile and for this feature is
perhaps not a good test. Just because gm prints that 8888 will be run, it
doesn't mean that it actually runs. I suppose we could test that the images are
written but I'm not sure this feature warrants that level of testing.

[epoger, 2013-04-24 16:18:26]

LGTM

https://codereview.chromium.org/13842014/diff/18001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/13842014/diff/18001/gm/gmmain.cpp#newcode177
gm/gmmain.cpp:177: static bool encode_to_dct_stream(SkWStream* stream, const
SkBitmap& bitmap, const SkIRect& rect);
Looks like this got picked up from
https://code.google.com/p/skia/source/diff?spec=svn8839&r=8835&format=side&pa...
... in future code reviews, I would suggest doing a separate "gcl upload" for
syncing your workspace to a newer rev.  It makes the per-patchset diffs more
useful.   (yadda yadda IMHO yadda yadda)

https://codereview.chromium.org/13842014/diff/18001/gm/tests/outputs/intentio...
File gm/tests/outputs/intentionally-skipped-tests/output-expected/stdout
(right):

https://codereview.chromium.org/13842014/diff/18001/gm/tests/outputs/intentio...
gm/tests/outputs/intentionally-skipped-tests/output-expected/stdout:5: GM: ...
over  2 configs ["8888", "565"]
Note that in verbose mode, we were already reporting which configs were run.

[bsalomon, 2013-04-24 18:07:18]

Committed patchset #5 manually as r8842 (presubmit successful).

[bsalomon, 2013-04-24 18:07:33]

https://codereview.chromium.org/13842014/diff/18001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/13842014/diff/18001/gm/gmmain.cpp#newcode177
gm/gmmain.cpp:177: static bool encode_to_dct_stream(SkWStream* stream, const
SkBitmap& bitmap, const SkIRect& rect);
On 2013/04/24 16:18:26, epoger wrote:
> Looks like this got picked up from
>
https://code.google.com/p/skia/source/diff?spec=svn8839&r=8835&format=side&pa...
> ... in future code reviews, I would suggest doing a separate "gcl upload" for
> syncing your workspace to a newer rev.  It makes the per-patchset diffs more
> useful.   (yadda yadda IMHO yadda yadda)


Sorry 'bout that... I moved the CL from my home machine to an office machine
with a later base rev.

https://codereview.chromium.org/13842014/diff/18001/gm/tests/outputs/intentio...
File gm/tests/outputs/intentionally-skipped-tests/output-expected/stdout
(right):

https://codereview.chromium.org/13842014/diff/18001/gm/tests/outputs/intentio...
gm/tests/outputs/intentionally-skipped-tests/output-expected/stdout:5: GM: ...
over  2 configs ["8888", "565"]
On 2013/04/24 16:18:26, epoger wrote:
> Note that in verbose mode, we were already reporting which configs were run.

But only after they all ran (which without --match is a long time). The idea
here is for a dev to see the configs right away before waiting for the tests to
run. I suppose we could consider moving the existing print in a future CL.