Make check_gn_headers.py more robust on the bots

build/check_gn_headers.py

 #!/usr/bin/env python
 # Copyright 2017 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.
 
 """Find header files missing in GN.
 
 This script gets all the header files from ninja_deps, which is from the true
 dependency generated by the compiler, and report if they don't exist in GN.
 """
@@ skipping 20 matching lines @@
     for line in iter(popen.stdout.readline, ''):
       yield line.rstrip()
 
     popen.stdout.close()
     return_code = popen.wait()
     if return_code:
       raise subprocess.CalledProcessError(return_code, cmd)
 
   ans, err = set(), None
   try:
-    ans = ParseNinjaDepsOutput(NinjaSource())
+    ans = ParseNinjaDepsOutput(NinjaSource(), out_dir)
   except Exception as e:
     err = str(e)
   q.put((ans, err))
 
 
-def ParseNinjaDepsOutput(ninja_out):
+def ParseNinjaDepsOutput(ninja_out, out_dir):
   """Parse ninja output and get the header files"""
   all_headers = set()
 
   prefix = '..' + os.sep + '..' + os.sep
 
   is_valid = False
   for line in ninja_out:
     if line.startswith('    '):
       if not is_valid:
         continue
       if line.endswith('.h') or line.endswith('.hh'):
         f = line.strip()
         if f.startswith(prefix):
           f = f[6:]  # Remove the '../../' prefix
           # build/ only contains build-specific files like build_config.h
           # and buildflag.h, and system header files, so they should be
           # skipped.
+          if f.startswith(out_dir) or f.startswith('out'):
+            continue
           if not f.startswith('build'):
             all_headers.add(f)
     else:
       is_valid = line.endswith('(VALID)')
 
   return all_headers
 
 
 def GetHeadersFromGN(out_dir, q):
   """Return all the header files from GN"""
@@ skipping 46 matching lines @@
          'python', '-c', 'import os;print os.environ["GCLIENT_DEP_PATH"]'])
     for i in gclient_out.split('\n'):
       if i.startswith('src/'):
         i = i[4:]
         prefixes.add(i)
   except Exception as e:
     err = str(e)
   q.put((prefixes, err))
 
 
+def IsBuildClean(out_dir):
+  cmd = ['ninja', '-C', out_dir, '-n']
+  out = subprocess.check_output(cmd)
+  return 'no work to do.' in out
+
+
 def ParseWhiteList(whitelist):
   out = set()
   for line in whitelist.split('\n'):
     line = re.sub(r'#.*', '', line).strip()
     if line:
       out.add(line)
   return out
 
 
 def FilterOutDepsedRepo(files, deps):
   return {f for f in files if not any(f.startswith(d) for d in deps)}
 
 
 def GetNonExistingFiles(lst):
   out = set()
   for f in lst:
     if not os.path.isfile(f):
       out.add(f)
   return out
 
 
 def main():
+
+  def DumpJson(data):
+    if args.json:
+      with open(args.json, 'w') as f:
+        json.dump(data, f)
+
+  def PrintError(msg):
+    DumpJson([])
+    parser.error(msg)
+
   parser = argparse.ArgumentParser(description='''
       NOTE: Use ninja to build all targets in OUT_DIR before running
       this script.''')
   parser.add_argument('--out-dir', metavar='OUT_DIR', default='out/Release',
                       help='output directory of the build')
   parser.add_argument('--json',
                       help='JSON output filename for missing headers')
   parser.add_argument('--whitelist', help='file containing whitelist')
+  parser.add_argument('--skip-dirty-check', action='store_true',
+                      help='skip checking whether the build is dirty')
 
   args, _extras = parser.parse_known_args()
 
   if not os.path.isdir(args.out_dir):
     parser.error('OUT_DIR "%s" does not exist.' % args.out_dir)
 
+  if not args.skip_dirty_check and not IsBuildClean(args.out_dir):
+    dirty_msg = 'OUT_DIR looks dirty. You need to build all there.'
+    if args.json:
+      # Assume running on the bots. Silently skip this step.
+      # This is possible because "analyze" step can be wrong due to
+      # underspecified header files. See crbug.com/725877
+      print dirty_msg
+      DumpJson([])
+      return 0
+    else:
+      # Assume running interactively.
+      parser.error(dirty_msg)
+
   d_q = Queue()
   d_p = Process(target=GetHeadersFromNinja, args=(args.out_dir, d_q,))
   d_p.start()
 
   gn_q = Queue()
   gn_p = Process(target=GetHeadersFromGN, args=(args.out_dir, gn_q,))
   gn_p.start()
 
   deps_q = Queue()
   deps_p = Process(target=GetDepsPrefixes, args=(deps_q,))
   deps_p.start()
 
   d, d_err = d_q.get()
   gn, gn_err = gn_q.get()
   missing = d - gn
   nonexisting = GetNonExistingFiles(gn)
 
   deps, deps_err = deps_q.get()
   missing = FilterOutDepsedRepo(missing, deps)
   nonexisting = FilterOutDepsedRepo(nonexisting, deps)
 
   d_p.join()
   gn_p.join()
   deps_p.join()
 
   if d_err:
-    parser.error(d_err)
+    PrintError(d_err)

[Dirk Pranke, 2017/06/03 01:16:37]

I'd pass the parser into PrintError; relying on the enclosed lexical scope is a
little dodgy in Python and it's not really necessary here.

[wychen, 2017/06/03 07:37:37]

It requires more than just |parser|. It also needs to conditionally dump an
empty json file, so args.json is also needed. Then it feels a bit repetitive.

I'll land this as is for now. If you think passing these looks better, I'm more
than happy to fix it in a following CL.

   if gn_err:
-    parser.error(gn_err)
+    PrintError(gn_err)
   if deps_err:
-    parser.error(deps_err)
+    PrintError(deps_err)
   if len(GetNonExistingFiles(d)) > 0:
-    parser.error('''Found non-existing files in ninja deps. You should
-        build all in OUT_DIR.''')
+    print 'Non-existing files in ninja deps:', GetNonExistingFiles(d)
+    PrintError('Found non-existing files in ninja deps. You should ' +
+               'build all in OUT_DIR.')
   if len(d) == 0:
-    parser.error('OUT_DIR looks empty. You should build all there.')
+    PrintError('OUT_DIR looks empty. You should build all there.')
   if any((('/gen/' in i) for i in nonexisting)):
-    parser.error('OUT_DIR looks wrong. You should build all there.')
+    PrintError('OUT_DIR looks wrong. You should build all there.')
 
   if args.whitelist:
     whitelist = ParseWhiteList(open(args.whitelist).read())
     missing -= whitelist
+    nonexisting -= whitelist
 
   missing = sorted(missing)
   nonexisting = sorted(nonexisting)
 
-  if args.json:
-    with open(args.json, 'w') as f:
-      json.dump(missing, f)
+  DumpJson(sorted(missing + nonexisting))
 
   if len(missing) == 0 and len(nonexisting) == 0:
     return 0
 
   if len(missing) > 0:
     print '\nThe following files should be included in gn files:'
     for i in missing:
       print i
 
   if len(nonexisting) > 0:
     print '\nThe following non-existing files should be removed from gn files:'
     for i in nonexisting:
       print i
 
   return 1
 
 
 if __name__ == '__main__':
   sys.exit(main())