Add more intelligent problem tracking.

scripts/maint/practracker/practracker.py

@@ -5,7 +5,7 @@ Tor code best-practices tracker
 
 Go through the various .c files and collect metrics about them. If the metrics
 violate some of our best practices and they are not found in the optional
-exceptions file ("./exceptions.txt"), then log a violation about them.
+exceptions file ("./exceptions.txt"), then log a problem about them.
 
 The exceptions file is meant to be initialized with the current state of the
 source code as follows: ./practracker.py > ./exceptions.txt
@@ -22,6 +22,7 @@ import os, sys
 
 import metrics
 import util
+import problem
 
 # We don't want to run metrics for unittests, automatically-generated C files,
 # external libraries or git leftovers.
@@ -41,76 +42,74 @@ MAX_INCLUDE_COUNT = 50
 
 #######################################################
 
-def print_violation_if_not_exception(violation_str, exceptions_str):
-    # Check if this violation is already in the optional exceptions file
-    if exceptions_str and violation_str in exceptions_str:
-        return
-
-    print violation_str
+ProblemVault = None
 
 #######################################################
 
-def consider_file_size(fname, f, exceptions_str):
+def consider_file_size(fname, f):
     file_size = metrics.file_len(f)
     if file_size > MAX_FILE_SIZE:
-        violation_str = "violation file-size %s %d" % (fname, file_size)
-        print_violation_if_not_exception(violation_str, exceptions_str)
+        v = problem.FileSizeProblem(fname, file_size)
+        ProblemVault.register_problem(v)
 
-def consider_includes(fname, f, exceptions_str):
+def consider_includes(fname, f):
     include_count = metrics.get_include_count(f)
 
     if include_count > MAX_INCLUDE_COUNT:
-        violation_str = "violation include-count %s %d" % (fname, include_count)
-        print_violation_if_not_exception(violation_str, exceptions_str)
+        v = problem.IncludeCountProblem(fname, include_count)
+        ProblemVault.register_problem(v)
 
-def consider_function_size(fname, f, exceptions_str):
+def consider_function_size(fname, f):
     for name, lines in metrics.function_lines(f):
         # Don't worry about functions within our limits
         if lines <= MAX_FUNCTION_SIZE:
             continue
 
-        # That's a big function! Issue a violation!
+        # That's a big function! Issue a problem!
         canonical_function_name = "%s:%s()" % (fname,name)
-        violation_str = "violation function-size %s %s" % (lines, canonical_function_name)
-        print_violation_if_not_exception(violation_str, exceptions_str)
+        v = problem.FunctionSizeProblem(canonical_function_name, lines)
+        ProblemVault.register_problem(v)
 
 #######################################################
 
-def consider_all_metrics(files_list, exceptions_str):
+def consider_all_metrics(files_list):
     """Consider metrics for all files"""
     for fname in files_list:
         with open(fname, 'r') as f:
-            consider_metrics_for_file(fname, f, exceptions_str)
+            consider_metrics_for_file(fname, f)
 
-def consider_metrics_for_file(fname, f, exceptions_str):
+def consider_metrics_for_file(fname, f):
     """
     Get metrics for file with filename 'fname' and file descriptor 'f'.
     """
     # Get file length
-    consider_file_size(fname, f, exceptions_str)
+    consider_file_size(fname, f)
 
     # Consider number of #includes
     f.seek(0)
-    consider_includes(fname, f, exceptions_str)
+    consider_includes(fname, f)
 
     # Get function length
     f.seek(0)
-    consider_function_size(fname, f, exceptions_str)
+    consider_function_size(fname, f)
 
 def main():
+    global ProblemVault
+
     # 1) Get all the .c files we care about
     files_list = util.get_tor_c_files(TOR_TOPDIR, EXCLUDE_SOURCE_DIRS)
 
-    # 2) Read an optional exceptions file so that we don't warn about the past
-    exceptions_str = None
+    # 2) Initialize problem vault and load an optional exceptions file so that
+    # we don't warn about the past
     try:
         with open(EXCEPTIONS_FILE, 'r') as exception_f:
-            exceptions_str = exception_f.read()
+            ProblemVault = problem.ProblemVault(exception_f)
     except IOError:
         print "No exception file provided"
+        ProblemVault = problem.ProblemVault(None)
 
-    # 3) Go through all the files and report violations if they are not exceptions
-    consider_all_metrics(files_list, exceptions_str)
+    # 3) Go through all the files and report problems if they are not exceptions
+    consider_all_metrics(files_list)
 
 if __name__ == '__main__':
     main()

scripts/maint/practracker/problem.py

@@ -0,0 +1,96 @@
+"""
+In this file we define a ProblemVault class where we store all the
+exceptions and all the problems we find with the code.
+
+The ProblemVault is capable of registering problems and also figuring out if a
+problem is worse than a registered exception so that it only warns when things
+get worse.
+"""
+
+class ProblemVault(object):
+    """
+    Singleton where we store the various new problems we
+    found in the code, and also the old problems we read from the exception
+    file.
+    """
+    def __init__(self, exception_file):
+        # Exception dictionary: { problem.key() : Problem object }
+        self.exceptions = {}
+
+        if exception_file:
+            self.register_exceptions(exception_file)
+
+    def register_exceptions(self, exception_file):
+        # Register exceptions
+        for line in exception_file:
+            problem = get_old_problem_from_exception_str(line)
+            if problem is None:
+                continue
+
+            # XXX this might overwrite problems with the same key (e.g. MOCK_IMPL)
+            self.exceptions[problem.key()] = problem
+            #print "Registering exception: %s" % problem
+
+    def register_problem(self, problem):
+        # This is a new problem, print it
+        if problem.key() not in self.exceptions:
+            print problem
+            return
+
+        # If it's an old problem, we don't warn if the situation got better
+        # (e.g. we went from 4k LoC to 3k LoC), but we do warn if the
+        # situation worsened (e.g. we went from 60 includes to 80).
+        if problem.is_worse_than(self.exceptions[problem.key()]):
+            print problem
+            return
+#        else:
+#            print "OK %s better than %s" % (problem, self.exceptions[problem.key()])
+
+
+class Problem(object):
+    def __init__(self, problem_type, problem_location, metric_value):
+        self.problem_location = problem_location
+        self.metric_value = int(metric_value)
+        self.problem_type = problem_type
+
+    def is_worse_than(self, other_problem):
+        """Return True if this is a worse problem than other_problem"""
+        if self.metric_value > other_problem.metric_value:
+            return True
+        return False
+
+    def key(self):
+        """Generate a unique key that describes this problem that can be used as a dictionary key"""
+        return "%s:%s" % (self.problem_location, self.problem_type)
+
+    def __str__(self):
+        return "problem %s %s %s" % (self.problem_type, self.problem_location, self.metric_value)
+
+class FileSizeProblem(Problem):
+    def __init__(self, problem_location, metric_value):
+        super(FileSizeProblem, self).__init__("file-size", problem_location, metric_value)
+
+class IncludeCountProblem(Problem):
+    def __init__(self, problem_location, metric_value):
+        super(IncludeCountProblem, self).__init__("include-count", problem_location, metric_value)
+
+class FunctionSizeProblem(Problem):
+    def __init__(self, problem_location, metric_value):
+        super(FunctionSizeProblem, self).__init__("function-size", problem_location, metric_value)
+
+def get_old_problem_from_exception_str(exception_str):
+    try:
+        _, problem_type, problem_location, metric_value = exception_str.split(" ")
+    except ValueError:
+        return None
+
+    if problem_type == "file-size":
+        return FileSizeProblem(problem_location, metric_value)
+    elif problem_type == "include-count":
+        return IncludeCountProblem(problem_location, metric_value)
+    elif problem_type == "function-size":
+        return FunctionSizeProblem(problem_location, metric_value)
+    else:
+        print "Unknown exception line %s" % exception_str
+        return None
+