Restrict try bots for platform specific CLs

PRESUBMIT.py

Index: PRESUBMIT.py
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index ab24b57ecc65c99bc9570cd00872f4ca9e0f769c..9531f1190f02bae1833803ba3eafce8483096d81 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -437,29 +437,25 @@ def GetPreferredTrySlaves(project, change):
   only_objc_files = all(f.endswith(('.mm', '.m')) for f in affected_files)
   if only_objc_files:
     return ['mac_rel']
+  android_re = r'/android/|_android(_\w+)?\.\w+$'
+  if all(re.search(android_re, f) for f in affected_files):

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

why not?
if all('android' in f for f in affected_files):

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

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:

+    return ['android']
   preferred = ['win_rel', 'linux_rel', 'mac_rel', 'linux_clang:compile']
-  aura_re = '_aura[^/]*[.][^/]*'
+  aura_re = '_aura[^/]*[.]\w+$'

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

Reading this regexp, I don't see why '[.]' is used, as it is the same as '.'.

What's the goal of adding \w?

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

"." 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+$'.

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

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

   if any(re.search(aura_re, f) for f in affected_files):
     preferred.append('linux_chromeos')
-  # Nothing in chrome/
-  android_re_list = ('^base/',
-                     '^build/common.gypi$',
-                     '^content/',
-                     '^ipc/',
-                     '^jingle/',
-                     '^media/',
-                     '^net/',
-                     '^sql/')
-  # Nothing that looks like win-only or aura-only
-  win_re = '_win\.(cc|h)$'
-  possibly_android = True
-  for non_android_re in (aura_re, win_re):
-    if all(re.search(non_android_re, f) for f in affected_files):
-      possibly_android = False
-      break
-  if possibly_android:
-    for f in change.AffectedFiles():
-      if any(re.search(r, f.LocalPath()) for r in android_re_list):
-        preferred.append('android')
-        break
+  # Skip android if everything is aura or windows
+  win_or_aura_re = aura_re + '|_win(_\w+)?\.(cc|h)$'

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

aura_re already contains a $

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

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]

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.

+  # Ensure CL contains some relevant files (i.e. not purely ^chrome)
+  android_maybe_re = ('^base/'
+                      '|^build/common.gypi$'
+                      '|^content/'
+                      '|^ipc/'
+                      '|^jingle/'
+                      '|^media/'
+                      '|^net/'
+                      '|^sql/')
+  if (not all(re.search(win_or_aura_re, f) for f in affected_files)
+      and any(re.search(android_maybe_re, f) for f in affected_files)):
+    preferred.append('android')
   return preferred