diff --git a/Makefile.am b/Makefile.am index b40b2e51bc..415c2f2b4e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -161,7 +161,9 @@ EXTRA_DIST+= \ README \ ReleaseNotes \ scripts/maint/checkIncludes.py \ - scripts/maint/checkSpace.pl + scripts/maint/checkSpace.pl \ + scripts/maint/practracker + ## This tells etags how to find mockable function definitions. AM_ETAGSFLAGS=--regex='{c}/MOCK_IMPL([^,]+,\W*\([a-zA-Z0-9_]+\)\W*,/\1/s' @@ -224,7 +226,7 @@ shellcheck: fi; \ fi -check-local: check-spaces check-changes check-includes shellcheck +check-local: check-spaces check-changes check-includes check-best-practices shellcheck need-chutney-path: @if test ! -d "$$CHUTNEY_PATH"; then \ @@ -345,6 +347,11 @@ if USEPYTHON $(PYTHON) $(top_srcdir)/scripts/maint/checkIncludes.py endif +check-best-practices: +if USEPYTHON + $(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py $(top_srcdir) +endif + check-docs: all $(PERL) $(top_builddir)/scripts/maint/checkOptionDocs.pl diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py index 3c22abd444..541c5cbfb3 100755 --- a/scripts/maint/practracker/practracker.py +++ b/scripts/maint/practracker/practracker.py @@ -1,21 +1,21 @@ #!/usr/bin/python3 """ -Tor code best-practices tracker +Best-practices tracker for Tor source code. 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 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 +exceptions file, then log a problem about them. We currently do metrics about file size, function size and number of includes. -TODO: - - How is this tool supposed to be used? How should the exception file work? - How should the UI work? Does it need special exit codes? - - Fix the function_length function so that practracker_tests.py passes. +practracker.py should be run with its second argument pointing to the Tor +top-level source directory like this: + $ python3 ./scripts/maint/practracker/practracker.py . + +The exceptions file is meant to be initialized once with the current state of +the source code and then get saved in the repository for ever after: + $ python3 ./scripts/maint/practracker/practracker.py . > ./scripts/maint/practracker/exceptions.txt """ import os, sys @@ -24,14 +24,8 @@ import metrics import util import problem -# We don't want to run metrics for unittests, automatically-generated C files, -# external libraries or git leftovers. -EXCLUDE_SOURCE_DIRS = ["/src/test/", "/src/trunnel/", "/src/ext/", "/.git/"] - -# Where the Tor source code is -TOR_TOPDIR = "../../../" -# An optional exceptions_file -EXCEPTIONS_FILE = "./exceptions.txt" +# The filename of the exceptions file (it should be placed in the practracker directory) +EXCEPTIONS_FNAME = "./exceptions.txt" # Recommended file size MAX_FILE_SIZE = 3000 # lines @@ -42,8 +36,12 @@ MAX_INCLUDE_COUNT = 50 ####################################################### +# ProblemVault singleton ProblemVault = None +# The Tor source code topdir +TOR_TOPDIR = None + ####################################################### def consider_file_size(fname, f): @@ -114,21 +112,31 @@ def consider_metrics_for_file(fname, f): return found_new_issues def main(): + if (len(sys.argv) != 2): + print("Usage:\n\t$ practracker.py \n\t(e.g. $ practracker.py ~/tor/)") + return + + global TOR_TOPDIR + TOR_TOPDIR = sys.argv[1] + exceptions_file = os.path.join(TOR_TOPDIR, "scripts/maint/practracker", EXCEPTIONS_FNAME) + # 1) Get all the .c files we care about - files_list = util.get_tor_c_files(TOR_TOPDIR, EXCLUDE_SOURCE_DIRS) + files_list = util.get_tor_c_files(TOR_TOPDIR) # 2) Initialize problem vault and load an optional exceptions file so that # we don't warn about the past global ProblemVault - ProblemVault = problem.ProblemVault(EXCEPTIONS_FILE) + ProblemVault = problem.ProblemVault(exceptions_file) # 3) Go through all the files and report problems if they are not exceptions found_new_issues = consider_all_metrics(files_list) - if found_new_issues: - sys.exit(1) - else: - sys.exit(0) + # If new issues were found, try to give out some advice to the developer on how to resolve it. + if (found_new_issues): + new_issues_str = "practracker FAILED as indicated by the problem lines above. Please use the exceptions file ({}) to find any previous state of these problems. If you are unable to fix the underlying best-practices issue right now then you need to either update the relevant exception line or add a new one.".format(exceptions_file) + print(new_issues_str) + + sys.exit(found_new_issues) if __name__ == '__main__': main() diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py index e23d3bbf0e..9f5771678e 100644 --- a/scripts/maint/practracker/problem.py +++ b/scripts/maint/practracker/problem.py @@ -96,6 +96,6 @@ def get_old_problem_from_exception_str(exception_str): elif problem_type == "function-size": return FunctionSizeProblem(problem_location, metric_value) else: - print("Unknown exception line {}".format(exception_str)) +# print("Unknown exception line '{}'".format(exception_str)) return None diff --git a/scripts/maint/practracker/util.py b/scripts/maint/practracker/util.py index 8e9d95ece4..905cb8a5e0 100644 --- a/scripts/maint/practracker/util.py +++ b/scripts/maint/practracker/util.py @@ -1,6 +1,10 @@ import os -def get_tor_c_files(tor_topdir, exclude_dirs): +# We don't want to run metrics for unittests, automatically-generated C files, +# external libraries or git leftovers. +EXCLUDE_SOURCE_DIRS = ["/src/test/", "/src/trunnel/", "/src/ext/", "/.git/"] + +def get_tor_c_files(tor_topdir): """ Return a list with the .c filenames we want to get metrics of. """ @@ -14,7 +18,7 @@ def get_tor_c_files(tor_topdir, exclude_dirs): # Exclude the excluded paths full_path = os.path.join(root,filename) - if any(exclude_dir in full_path for exclude_dir in exclude_dirs): + if any(exclude_dir in full_path for exclude_dir in EXCLUDE_SOURCE_DIRS): continue files_list.append(full_path)