Adds ability for builders to only compile targets affected by change

Adds ability for builders to only compile targets affected by change

Currently opt-in.

BUG=109173
TEST=none
R=iannucci@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=290920

[sky, 2014-08-19 18:14:10]

Many of the diffs are because the chrome target isn't getting forced into the
build any more. This was done by unconditionally adding the telemetry and nacl
tests, which force chrome in the list. Since telemetry and nacl are now only
added if we're building the chrome target (or all), there are a bunch of other
diffs. I did add a test that makes sure chrome ends up in the list, so that you
can see telemetry and nacl are added.

[iannucci, 2014-08-20 17:54:40]

looks generally good with some grumblings. If this is super-urgent to land, I
would accept filing cleanup bugs and adding TODOs to the code (but I might
grumble about it :p)

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/filter/example.py (right):

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/filter/example.py:26: api.filter.compile_targets)
nit: these asserts seem overly verbose... couldn't you just do

assert api.properties.get(..., []) == api.filter....

and then omit the properties in the test cases? If the properties are omitted
(as opposed to set-to-None), then get will return the default on the right hand
side.

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/filter/example.py:35:
api.properties(example_matching_compile_targets=None) +
fyi: you can do multiple properties per call here:

api.properties(
  key=value,
  other=value,
  ...
)

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:499: return
I would generally recommend returning something (probably a list of tests) and
then extend the list in the caller, rather than passing a list as a mutate-able
entity to the function. e.g.

tests.extend(tests_in_compile_targets(test, compile_targets))

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:675: if not api.platform.is_win and not
bot_config.get('exclude_compile_all'):
TODO: the bot_config stuff has gotten excessively complex with lots of
hard-coded strings in lots of places. It needs to be documented and/or
symbolized.

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:700: if 'all' in compile_targets:
TBH, I would rather 'magic' compile targets like 'all' be prefixed with
something (like '$all', or '!all') to indicate their specialness at a glance.

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:732: 'chromium_chromeos_clang']
this should be

if bot_config.get('do_telemetry_tests'):  # or some equally obvious string
  # add telemetry tests

What's the point of a config with orthogonal properties if you conflate property
meanings? :(

In fact, on the 'magic values' note, could this just be '$telemetry_tests' in
the test list or something? (and similarly for every explicitly named block of
tests in this function)

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:733: and not
buildername.startswith('win8')):
This is exactly the sort of logic that configs should prevent...

[sky, 2014-08-20 20:26:33]

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/filter/example.py (right):

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/filter/example.py:26: api.filter.compile_targets)
On 2014/08/20 17:54:39, iannucci wrote:
> nit: these asserts seem overly verbose... couldn't you just do
> 
> assert api.properties.get(..., []) == api.filter....
> 
> and then omit the properties in the test cases? If the properties are omitted
> (as opposed to set-to-None), then get will return the default on the right
hand
> side.

I only wanted to do the interesting part of the assert (the part after the or)
if the property was actually provided. But I had this a bit wrong as both [] and
None are the same. Updated.

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/filter/example.py:35:
api.properties(example_matching_compile_targets=None) +
On 2014/08/20 17:54:39, iannucci wrote:
> fyi: you can do multiple properties per call here:
> 
> api.properties(
>   key=value,
>   other=value,
>   ...
> )
> 

Done.

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:499: return
On 2014/08/20 17:54:40, iannucci wrote:
> I would generally recommend returning something (probably a list of tests) and
> then extend the list in the caller, rather than passing a list as a
mutate-able
> entity to the function. e.g.
> 
> tests.extend(tests_in_compile_targets(test, compile_targets))

Done.

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:675: if not api.platform.is_win and not
bot_config.get('exclude_compile_all'):
On 2014/08/20 17:54:40, iannucci wrote:
> TODO: the bot_config stuff has gotten excessively complex with lots of
> hard-coded strings in lots of places. It needs to be documented and/or
> symbolized.

No argument there.

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:700: if 'all' in compile_targets:
On 2014/08/20 17:54:40, iannucci wrote:
> TBH, I would rather 'magic' compile targets like 'all' be prefixed with
> something (like '$all', or '!all') to indicate their specialness at a glance.

The specialness is handled by the build system. Are you suggesting we some how
preprocess these?

https://codereview.chromium.org/485873004/diff/40001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:732: 'chromium_chromeos_clang']
On 2014/08/20 17:54:40, iannucci wrote:
> this should be
> 
> if bot_config.get('do_telemetry_tests'):  # or some equally obvious string
>   # add telemetry tests
> 
> What's the point of a config with orthogonal properties if you conflate
property
> meanings? :(
> 
> In fact, on the 'magic values' note, could this just be '$telemetry_tests' in
> the test list or something? (and similarly for every explicitly named block of
> tests in this function)

Agreed. I didn't add these. I'm happy to change in a followup patch. How does
that sound?

[iannucci, 2014-08-20 20:40:09]

lgtm, followups sgtm, I was just making sure we're all on the same page :D

[sky, 2014-08-20 21:35:39]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-08-20 21:36:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sky@chromium.org/485873004/80001

[commit-bot: I haz the power, 2014-08-20 21:38:14]

Committed patchset #5 (80001) as 290920