Filter added to prune files before applying network annotation extractor.

tools/traffic_annotation/auditor/traffic_annotation_auditor.py

 #!/usr/bin/env python
 # Copyright (c) 2017 The Chromium Authors. All rights reserved.

[msramek, 2017/05/31 11:31:36]

Ditto.

[Ramin Halavati, 2017/05/31 12:28:25]

Done.

 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """This script is used to extract network traffic annotations from Chrome.
 Please refer to README.md for running steps."""
 
 import argparse
+import datetime
 import os
 import subprocess
 import sys
+import tempfile
+
+from annotation_relevent_filter import NetworkTrafficAnnotationFileFilter

[msramek, 2017/05/31 11:31:36]

How about renaming the module to traffic_annotation_file_filter, and the class
to TrafficAnnotationFileFilter? That way they're consistent with each other, and
with this file.

[Ramin Halavati, 2017/05/31 12:28:26]

Done.

+
 
 # These two lines are required to import protobuf from third_party directory
 # instead of the one installed with python.
 from prepare_protobuf import PrepareProtobuf
 PrepareProtobuf()
 
 from google.protobuf import text_format
 import traffic_annotation_pb2
 
 
@@ skipping 10 matching lines @@
   'net/traffic_annotation/network_traffic_annotation.h'.
   args:
     unique_id: str The string to be converted to hash code.
 
   Returns:
     unsigned int Hash code of the input string
   """
   return _RecursiveHash(unique_id) if len(unique_id) else -1
 
 
-def _RunClangTool(src_dir, build_dir, path_filters):
+def _RunClangTool(src_dir, build_dir, path_filters, prefilter_files):
   """Executes the clang tool to extract annotations.
   Args:
     src_dir: str Path to the src directory of Chrome.
     build_dir: str Path to the build directory.
     path_filters: list of str List of paths to source directories for
         extraction.
+    prefilter_files: bool Flag stating if source files should be first filtered
+        using annotation related keywords and then given to clang tool.
 
   Returns:
     raw_annotations: str Output of clang tool (extracted content and metadata of
         annotations).
   """
   raw_annotations = ""
-  for path in path_filters:
-    args = [
+  args = [
         src_dir + "/tools/clang/scripts/run_tool.py",
         "--generate-compdb",
         "--tool=traffic_annotation_extractor",
-        "-p", build_dir,
-        path]
-    if sys.platform == "win32":
+        "-p=" + build_dir,
+        None]
+  if sys.platform == "win32":
       args.insert(0, "python")
+
+  if prefilter_files:
+    source_filenames = tempfile.NamedTemporaryFile(mode='w+t', delete=False)

[msramek, 2017/05/31 11:31:36]

nit: please be consistent in the usage of single / double quotes

[Ramin Halavati, 2017/05/31 12:28:25]

Done.

+    file_filter = NetworkTrafficAnnotationFileFilter(src_dir, False)
+
+    for path in path_filters:
+      files_list = file_filter.GetFilteredFilesList(path)
+      for filename in files_list:
+        source_filenames.write("%s\n" % filename)
+    source_filenames.close()
+    args[-1] = "--source-filenames=" + source_filenames.name

[msramek, 2017/05/31 11:31:36]

Adding None and then optionally overwriting it seems strange. Why not just
append at the end?

[Ramin Halavati, 2017/05/31 12:28:25]

Because when prefilter_files is not selected, this value is replaced for each
provided path. I can remove None and keep adding and removing it in the else
part.

[msramek, 2017/05/31 14:55:24]

Oh, I see. Could you just comment on the line 70? Something like:

None]  # Placeholder for a path argument.

[Ramin Halavati, 2017/05/31 19:19:13]

Done.

     command = subprocess.Popen(args, stdout=subprocess.PIPE,
                                stderr=subprocess.PIPE)
     stdout_text, stderr_text = command.communicate()
     raw_annotations += stdout_text
     if stderr_text:
       print stderr_text
+    os.remove(source_filenames.name)
+  else:
+    for path in path_filters:
+      args[-1] = path
+      command = subprocess.Popen(args, stdout=subprocess.PIPE,
+                                stderr=subprocess.PIPE)

[msramek, 2017/05/31 11:31:36]

style: offset

[Ramin Halavati, 2017/05/31 12:28:26]

Done.

+      stdout_text, stderr_text = command.communicate()

[msramek, 2017/05/31 11:31:36]

Why is this part duplicated? It can be extracted after the if-else statement.

[Ramin Halavati, 2017/05/31 12:28:25]

In the else part, we call it separately for each path_filter and concatenate the
results. I could add a helper function or collect them in a set and execute them
without duplicate code, but don't know which is neater.

+      raw_annotations += stdout_text
+      if stderr_text:
+        print stderr_text
   return raw_annotations

[msramek, 2017/05/31 11:31:36]

Maybe return null if command exited with an error?

[Ramin Halavati, 2017/05/31 12:28:25]

Clang issues some errors for files that are not part of compiledb, but we don't
care about them. I can remove printing stderr, but thought it may attract some
attention sometime.

[msramek, 2017/05/31 14:55:24]

Acknowledged. Then keep it as it is :)

[Ramin Halavati, 2017/05/31 19:19:13]

Done.

 
 
 def _ParsRawAnnotations(raw_annotations):
   """Parses raw annotations texts which are received from the clang tool.
   Args:
     raw_annotations: str Serialization of annotations and metadata. Each
         annotation should have either of the following lines:
         1- "==== NEW ANNOTATION ===="
         2- File path.
         3- Name of the function including this position.
@@ skipping 126 matching lines @@
 
 def _WriteHashCodesFile(annotations, metadata, file_path):
   """Writes unique ids and hash codes of annotations into a simple text file.
   args:
     annotations: ExtractedNetworkTrafficAnnotation A protobuf including all
       extracted annotations.
     metadata: list of dict Metadata for annotations, as specified in the outputs
       of _ParsRawAnnotations function.
     file_path: str File path to the brief summary file.
   """
-  with open(file_path, 'w') as summary_file:
-    for annotation, meta in zip(annotations.network_traffic_annotation,
-                                metadata):
-      summary_file.write(
-          "%s,%s\n" % (annotation.unique_id, meta['unique_id_hash']))
-    for keyword in ("test", "test_partial", "undefined", "missing"):
-      summary_file.write(
-          "%s,%s\n" % (keyword, _ComputeStringHash(keyword)))
+  hash_list = []
+  for annotation, meta in zip(annotations.network_traffic_annotation, metadata):
+    hash_list += ["%s,%s" % (annotation.unique_id, meta['unique_id_hash'])]
+  for keyword in ("test", "test_partial", "undefined", "missing"):
+    hash_list += ["%s,%s" % (keyword, _ComputeStringHash(keyword))]
+  open(file_path, 'w').write("\n".join(sorted(hash_list)))
 
 
 def main():
   parser = argparse.ArgumentParser(description='Traffic Annotation Auditor.')
   parser.add_argument('--build-dir',
                       help='Path to the build directory.')
   parser.add_argument('--extractor-output',
                       help='Optional path to the temporary file that extracted '
                            'annotations will be stored into.')
   parser.add_argument('--extractor-input',
                       help='Optional path to the file that temporary extracted '
                            'annotations are already stored in. If this is '
                            'provided, clang tool is not run and this is used '
                            'as input.')
   parser.add_argument('--summary-file',
                       help='Path to the output file with all annotations.')
   parser.add_argument('--hash-codes-file',
                       help='Path to the output file with the list of unique '
                            'ids and their hash codes.')
   parser.add_argument('path_filters',
                       nargs='*',
                       help='Optional paths to filter what files the tool is '
                            'run on.')
+  parser.add_argument('--prefilter-files', action='store_true',
+                      help='Checks source files for patterns of annotations '
+                           'and network functions that may require annotation '
+                           'and limits running clang tool only on them.')
   args = parser.parse_args()
 
   if not args.summary_file and not args.hash_codes_file:
     print "Warning: Output file not specified."
 
   # If a pre-extracted input file is provided, load it.
   if args.extractor_input:
     with open(args.extractor_input, 'r') as raw_file:
       raw_annotations = raw_file.read()
   else:
     # Either extacted input file or build directory should be provided.
     if not args.build_dir:
       print "You must either specify the build directory to run the clang " \
             "tool and extract annotations, or specify the input directory " \
             "where extracted annotation files already exist.\n"
       return 1
 
     # Get Chrome source directory with relative path from this file.
     chrome_source = os.path.abspath(os.path.join(os.path.dirname(
         os.path.realpath(__file__)), "..", "..", ".."))
     raw_annotations = _RunClangTool(chrome_source, args.build_dir,
-        args.path_filters if args.path_filters else ["./"])
+        args.path_filters if args.path_filters else [""],

[msramek, 2017/05/31 11:31:36]

Can't this [""] be the "default" argument to argparse?

[Ramin Halavati, 2017/05/31 12:28:25]

Done.

+        args.prefilter_files)
 
   if args.extractor_output:
     with open(args.extractor_output, 'w') as raw_file:
       raw_file.write(raw_annotations)
 
   annotations, metadata, errors = _ParsRawAnnotations(raw_annotations)
 
   if not annotations:
     print "Could not extract any annotation."
     if errors:
       print "Errors:\n%s" % "\n".join(errors)
     return 1
 
   if args.summary_file:
     _WriteSummaryFile(annotations, metadata, errors, args.summary_file)
 
   if args.hash_codes_file:
     _WriteHashCodesFile(annotations, metadata, args.hash_codes_file)
 
   return 0
 
 
 if __name__ == '__main__':
   sys.exit(main())