Filter added to prune files before applying network annotation extractor.

tools/traffic_annotation/auditor/traffic_annotation_auditor.py

Index: tools/traffic_annotation/auditor/traffic_annotation_auditor.py
diff --git a/tools/traffic_annotation/auditor/traffic_annotation_auditor.py b/tools/traffic_annotation/auditor/traffic_annotation_auditor.py
index 631b6dc3552a445570e7854ab5d83ad0473dd21c..51c47168136f958938be8a8de74a1282294dc8ec 100755
--- a/tools/traffic_annotation/auditor/traffic_annotation_auditor.py
+++ b/tools/traffic_annotation/auditor/traffic_annotation_auditor.py
@@ -7,9 +7,14 @@
 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.
@@ -40,34 +45,56 @@ def _ComputeStringHash(unique_id):
   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.

 
 
@@ -215,14 +242,12 @@ def _WriteHashCodesFile(annotations, metadata, file_path):
       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():
@@ -246,6 +271,10 @@ def main():
                       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:
@@ -267,7 +296,8 @@ def main():
     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: