Pass the chromeos build variable to isolate.py in _run targets, and update browser_tests.isolate to…

Pass the chromeos build variable to isolate.py in _run targets, and update browser_tests.isolate to test it.

This doesn't change the conditional contents of browser_tests.isolate
on any platform. All tests formerly under OS=="linux" are now under
OS=="linux" and (chromeos==0 or chromeos==1).


BUG=158329
TEST=build _run targets, look for new variable in .isolated.state


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

[M-A Ruel, 2013-02-15 20:09:12]

Sorry, was distracted.

lgtm

https://codereview.chromium.org/12208124/diff/1/chrome/browser_tests.isolate
File chrome/browser_tests.isolate (right):

https://codereview.chromium.org/12208124/diff/1/chrome/browser_tests.isolate#...
chrome/browser_tests.isolate:6: ['(OS=="linux" and chromeos==1) or ((OS=="linux"
or OS=="mac" or OS=="win") and chromeos==0)', {
I'd prefer if the code could understand that with OS==mac or OS==win, the
variable chromeos is unbounded. I guess it's not easily doable because
isolate.py doesn't know about that.

https://codereview.chromium.org/12208124/diff/1/chrome/browser_tests.isolate#...
chrome/browser_tests.isolate:178: ['OS=="linux" and (chromeos==0 or
chromeos==1)', {
It'd be nice if we could tell isolate.py the possible values for each variables
so it would know the space to search for minimization.

[benrg, 2013-02-15 20:38:12]

On 2013/02/15 20:09:12, Marc-Antoine Ruel wrote:
> I'd prefer if the code could understand that with OS==mac or OS==win, the
> variable chromeos is unbounded. I guess it's not easily doable because
> isolate.py doesn't know about that.
> [...]
> It'd be nice if we could tell isolate.py the possible values for each
variables
> so it would know the space to search for minimization.

I remember we talked about this in our face-to-face meeting but I can't remember
what we decided - that it would be a good idea for a follow-up change?

The two obvious approaches are:

{
  'conditions': [
    # Always just one condition at the top level, covering all configurations
    ['(X="foo" and (Y==0 or Y==1)) or (X=="bar" and Y==0)', {
      'conditions': [
        ['X=="bar"', ...],
        ...
      ],
    ],
  }
}

or

{
  'variables': {
    'config_variables': ['X', 'Y'],
    'configs': [("foo", 0), ("foo", 1), ("bar", 0)],
  },
  'conditions': [...]
}

The latter may be more readable but it has the disadvantage that gyp won't
understand what it means.

By the way, I remember you mentioning that gyp doesn't support the in operator,
but it looks like it does, though I only see one use of it in the Chrome tree
(in skia.gyp). It would be easy to add support for it to isolate.py and it would
go a fair way toward deuglifying the conditions.

[M-A Ruel, 2013-02-15 20:45:54]

On 2013/02/15 20:38:12, benrg wrote:
> On 2013/02/15 20:09:12, Marc-Antoine Ruel wrote:
> > I'd prefer if the code could understand that with OS==mac or OS==win, the
> > variable chromeos is unbounded. I guess it's not easily doable because
> > isolate.py doesn't know about that.
> > [...]
> > It'd be nice if we could tell isolate.py the possible values for each
> variables
> > so it would know the space to search for minimization.
> 
> I remember we talked about this in our face-to-face meeting but I can't
remember
> what we decided - that it would be a good idea for a follow-up change?

Indeed in a follow-up, not here.
 

> The two obvious approaches are:
> 
> {
>   'conditions': [
>     # Always just one condition at the top level, covering all configurations
>     ['(X="foo" and (Y==0 or Y==1)) or (X=="bar" and Y==0)', {
>       'conditions': [
>         ['X=="bar"', ...],
>         ...
>       ],
>     ],
>   }
> }
> 
> or
> 
> {
>   'variables': {
>     'config_variables': ['X', 'Y'],
>     'configs': [("foo", 0), ("foo", 1), ("bar", 0)],
>   },
>   'conditions': [...]
> }
> 
> The latter may be more readable but it has the disadvantage that gyp won't
> understand what it means.

We had talked about the later. I think if it can make the conditions the
shortest possible, it'd be good. Note that these variables can be put in a
build/common.isolate that is included everywhere so there would no need to
duplicate this. I think it'd be quite maintainable this way and relatively
self-explanative, which is very important.

> By the way, I remember you mentioning that gyp doesn't support the in
operator,
> but it looks like it does, though I only see one use of it in the Chrome tree
> (in skia.gyp). It would be easy to add support for it to isolate.py and it
would
> go a fair way toward deuglifying the conditions.

That'd be great.

[commit-bot: I haz the power, 2013-02-20 01:47:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/12208124/6001

[commit-bot: I haz the power, 2013-02-20 01:47:43]

Presubmit check for 12208124-6001 failed and returned exit status 1.

INFO:root:Found 2 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[benrg, 2013-02-20 17:50:10]

Ben, requesting OWNER approval for chrome/browser_tests.isolate. There are no
real changes, just reordering and no-op changes to the conditions.

[Ben Goodger (Google), 2013-02-20 17:52:33]

lgtm

[commit-bot: I haz the power, 2013-02-20 19:26:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/12208124/6001

[commit-bot: I haz the power, 2013-02-20 20:40:35]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-20 22:19:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/12208124/6001

[commit-bot: I haz the power, 2013-02-21 00:14:41]

Change committed as 183668