Adding _Check* function for invalid OS_MACROs in src/PRESUBMIT.py

PRESUBMIT.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Top-level presubmit script for Chromium.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into gcl.
 """
 
@@ skipping 147 matching lines @@
        'pool or the FILE thread instead.',
       ),
       True,
       (
         r"^content[\\\/]shell[\\\/]shell_browser_main\.cc$",
       ),
     ),
 )
 
 
+_VALID_OS_MACROS = (
+    # Please keep sorted.
+    'OS_ANDROID',
+    'OS_BSD',
+    'OS_CAT',       # For testing.
+    'OS_CHROMEOS',
+    'OS_FREEBSD',
+    'OS_IOS',
+    'OS_LINUX',
+    'OS_MACOSX',
+    'OS_NACL',
+    'OS_OPENBSD',
+    'OS_POSIX',
+    'OS_SOLARIS',
+    'OS_SUN',       # Not in build/build_config.h but in skia.
+    'OS_WIN',
+)
+
+
 def _CheckNoProductionCodeUsingTestOnlyFunctions(input_api, output_api):
   """Attempts to prevent use of functions intended only for testing in
   non-testing code. For now this is just a best-effort implementation
   that ignores header files and may have some false positives. A
   better implementation would probably need a proper C++ parser.
   """
   # We only scan .cc files and the like, as the declaration of
   # for-testing functions in header files are hard to distinguish from
   # calls to such functions without a proper C++ parser.
   file_inclusion_pattern = r'.+%s' % _IMPLEMENTATION_EXTENSIONS
@@ skipping 529 matching lines @@
   results.extend(_CheckNoPragmaOnce(input_api, output_api))
   results.extend(_CheckNoTrinaryTrueFalse(input_api, output_api))
   results.extend(_CheckUnwantedDependencies(input_api, output_api))
   results.extend(_CheckFilePermissions(input_api, output_api))
   results.extend(_CheckNoAuraWindowPropertyHInHeaders(input_api, output_api))
   results.extend(_CheckIncludeOrder(input_api, output_api))
   results.extend(_CheckForVersionControlConflicts(input_api, output_api))
   results.extend(_CheckPatchFiles(input_api, output_api))
   results.extend(_CheckHardcodedGoogleHostsInLowerLayers(input_api, output_api))
   results.extend(_CheckNoAbbreviationInPngFileName(input_api, output_api))
+  results.extend(_CheckForInvalidOSMacros(input_api, output_api))
 
   if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
     results.extend(input_api.canned_checks.RunUnitTestsInDirectory(
         input_api, output_api,
         input_api.PresubmitLocalPath(),
         whitelist=[r'^PRESUBMIT_test\.py$']))
   return results
 
 
 def _CheckSubversionConfig(input_api, output_api):
@@ skipping 72 matching lines @@
 def _CheckPatchFiles(input_api, output_api):
   problems = [f.LocalPath() for f in input_api.AffectedFiles()
       if f.LocalPath().endswith(('.orig', '.rej'))]
   if problems:
     return [output_api.PresubmitError(
         "Don't commit .rej and .orig files.", problems)]
   else:
     return []
 
 
+def _CheckForInvalidOSMacrosInFile(input_api, f):
+  """Check for sensible looking, totally invalid OS macros."""
+  results = []
+  pattern = input_api.re.compile(r'\b(OS_[A-Z]{3,})\b')

[Scott Hess - ex-Googler, 2013/03/21 04:20:00]

Have you run this over the existing codebase?  I haven't actually checked for
cases where we have OS_SOMETHING not inside of defined(OS_SOMETHING).  If it's
not in a defined() clause, then it's completely incorrect anyhow, so I'm not
sure whether I care about warning on those cases, but I do think it's worthwhile
to not have people routinely overriding the presubmit because of false
positives.

[Dan Beam, 2013/03/21 04:29:09]

I think people putting OS_BLAH in comments is equally bad at propagating the
wrong names as

  #if defined(OS_BLAH)

which is why I'm intentionally not looking for

  ^#(?:el|end)?if(?:n?def)?

And no, I haven't run it on the whole codebase (yet).

[Scott Hess - ex-Googler, 2013/03/21 05:03:51]

I can see both ways, but if the comment doesn't contain "defined(OS_BLAH)", then
they cannot easily copy/paste it to propagate the problem.  And even if they
copy/paste and add defined(), the presubmit will catch them on it.

+  for lnum, line in f.ChangedContents():
+    for match in pattern.finditer(line):
+      if not match.group(1) in _VALID_OS_MACROS:
+        results.append('    %s:%d %s' % (f.LocalPath(), lnum, match.group(1)))
+  return results
+
+
+def _CheckForInvalidOSMacros(input_api, output_api):
+  """Check all affected files for invalid OS macros."""
+  bad_macros = []
+  for f in input_api.AffectedFiles():
+    if not f.LocalPath().endswith('.py'):
+      bad_macros.extend(_CheckForInvalidOSMacrosInFile(input_api, f))
+
+  if not bad_macros:
+    return []
+
+  return [output_api.PresubmitError(
+      'Possibly invalid OS macros found. Please add your macro to\n'
+      'src/PRESUBMIT.py or update your code.', bad_macros)]
+
+
 def CheckChangeOnUpload(input_api, output_api):
   results = []
   results.extend(_CommonChecks(input_api, output_api))
   return results
 
 
 def CheckChangeOnCommit(input_api, output_api):
   results = []
   results.extend(_CommonChecks(input_api, output_api))
   # TODO(thestig) temporarily disabled, doesn't work in third_party/
@@ skipping 51 matching lines @@
       'win_rel',
       'win:compile',
   ]
 
   # Match things like path/aura/file.cc and path/file_aura.cc.
   # Same for chromeos.
   if any(re.search('[/_](aura|chromeos)', f) for f in files):
     trybots += ['linux_chromeos_clang:compile', 'linux_chromeos_asan']
 
   return trybots