Restrict try bots for platform specific CLs

Restrict try bots for platform specific CLs

- clean up logic for determining preferred trybots
- allow windows only CLs to try only on win_rel
- less restrictive filter for mac only CLs

BUG=
TEST=

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

[M-A Ruel, 2012-06-25 12:15:13]

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode441
PRESUBMIT.py:441: if all(re.search(android_re, f) for f in affected_files):
why not?
if all('android' in f for f in affected_files):

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode444
PRESUBMIT.py:444: aura_re = '_aura[^/]*[.]\w+$'
Reading this regexp, I don't see why '[.]' is used, as it is the same as '.'.

What's the goal of adding \w?

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode448
PRESUBMIT.py:448: win_or_aura_re = aura_re + '|_win(_\w+)?\.(cc|h)$'
aura_re already contains a $

[Isaac (away), 2012-06-25 17:02:17]

Thanks for looking this over.  I wrote some more comments & explanations.

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode441
PRESUBMIT.py:441: if all(re.search(android_re, f) for f in affected_files):
We could do that, and it'd probably be good enough.

It's not quite as safe as the regex which requires a folder named exactly
android and not "/androidhacks/" or a file ending in _android (or
_android_tests) rather than one named androidhacks.cc.

On 2012/06/25 12:15:13, Marc-Antoine Ruel wrote:
> why not?
> if all('android' in f for f in affected_files):

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode444
PRESUBMIT.py:444: aura_re = '_aura[^/]*[.]\w+$'
On 2012/06/25 12:15:13, Marc-Antoine Ruel wrote:
> Reading this regexp, I don't see why '[.]' is used, as it is the same as '.'.
> 
> What's the goal of adding \w?

"." matches any character while [.] or \. match a literal period.  Previously
they used [^/]* which looks for  0 or more chars that are not a slash.  But
since this is a fie extension it should really be 1 or more alphanumerics and
then a $.

\w is more restrictive than [^\]
+ is more restrictive than *


In fact the [^/] class is too broad in the other location too.  They're trying
to match files like 'aura_window_display.cc' and 'aurax11.cc' and only need \w
and underscores such as aura_re = '_aura[_\w]*[.]\w+$'.

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode448
PRESUBMIT.py:448: win_or_aura_re = aura_re + '|_win(_\w+)?\.(cc|h)$'
On 2012/06/25 12:15:13, Marc-Antoine Ruel wrote:
> aura_re already contains a $

There's a pipe symbol which means this is essentially 2 separate regexs put
together.  Each one needs a $ token.

[M-A Ruel, 2012-06-26 13:23:39]

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode444
PRESUBMIT.py:444: aura_re = '_aura[^/]*[.]\w+$'
On 2012/06/25 17:02:17, Isaac wrote:
> "." matches any character while [.] or \. match a literal period.  

optional nit: I really prefer '\.' to '[.]'. '[.]' is confusing IMHO. I know you
didn't write it but it makes sense to fix it now.

> In fact the [^/] class is too broad in the other location too.  They're trying
> to match files like 'aura_window_display.cc' and 'aurax11.cc' and only need \w
> and underscores such as aura_re = '_aura[_\w]*[.]\w+$'.

What about foo_aura_linux.cc ?

http://codereview.chromium.org/10667007/diff/4002/PRESUBMIT.py#newcode448
PRESUBMIT.py:448: win_or_aura_re = aura_re + '|_win(_\w+)?\.(cc|h)$'
On 2012/06/25 17:02:17, Isaac wrote:
> On 2012/06/25 12:15:13, Marc-Antoine Ruel wrote:
> > aura_re already contains a $
> 
> There's a pipe symbol which means this is essentially 2 separate regexs put
> together.  Each one needs a $ token.

Ok, but I think it's a loss of readability. I'd prefer something clearer and
more verbose. I don't mind having this code being longer since this is the kind
of code that grows organically so it needs to be self-maintainable.

[Isaac (away), 2012-06-27 00:18:08]

I rewrote this function based on your feedback.

ptal

[M-A Ruel, 2012-06-27 00:26:53]

thanks! lgtm

[commit-bot: I haz the power, 2012-06-27 00:43:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/10667007/12001

[commit-bot: I haz the power, 2012-06-27 02:13:12]

Change committed as 144372

[Isaac (away), 2012-06-27 04:12:18]

I'm confused -- I thought this would affect the commit queue, but it seems to
only affect git try.  Does commit queue always build all targets?

[M-A Ruel, 2012-07-03 15:55:16]

On 2012/06/27 04:12:18, Isaac wrote:
> I'm confused -- I thought this would affect the commit queue, but it seems to
> only affect git try.  Does commit queue always build all targets?

The commit queue uses http://dev.chromium.org/developers/testing/commit-queue.
There's "projects.py" that defines which builder will be used.