[lit] Move argument parsing/validation to separate file

[serge-sans-paille]

LGTM otherwise

llvm/utils/lit/lit/main.py

237

nitpicking: PEP8 recommends `sort(key=by_mtime, reverse=True)`

[yln]

Ensure refactored code follows Python style guide.

llvm/utils/lit/lit/main.py

237

Fixed. Thanks! :)

[yln]

@serge-sans-paille: I am assuming that you are again fine with this, but I would more comfortable landing this with the official green check mark. ;)

[thakis]

Looks like the --threads option disappeared in the move, which angers the libc++ testsuite.

[stevewan]

Hi @yln,

The disappeared --threads is causing LIT failures in our internal build bots. Can you please take a look? Thanks!

Steven

[yln]

Added back the --threads option alias in b858895c859712e7c0f4e1c5e0436bd20cbe0c34.
Apologies for the churn.

llvm/utils/lit/lit/cl_arguments.py

+import argparse
+import os
+import shlex
+import sys
+
+import lit.util
+
+def parse_args():
+    parser = argparse.ArgumentParser()
+    parser.add_argument('test_paths',
+            nargs='*',
+            help='Files or paths to include in the test suite')
+
+    parser.add_argument("--version",
+            dest="show_version",
+            help="Show version and exit",
+            action="store_true",
+            default=False)
+    parser.add_argument("-j", "--workers",
+            dest="numWorkers",
+            metavar="N",
+            help="Number of workers used for testing",
+            type=int,
+            default=None)
+    parser.add_argument("--config-prefix",
+            dest="configPrefix",
+            metavar="NAME",
+            help="Prefix for 'lit' config files",
+            action="store",
+            default=None)
+    parser.add_argument("-D", "--param",
+            dest="userParameters",
+            metavar="NAME=VAL",
+            help="Add 'NAME' = 'VAL' to the user defined parameters",
+            type=str,
+            action="append",
+            default=[])
+
+    format_group = parser.add_argument_group("Output Format")
+    # FIXME: I find these names very confusing, although I like the
+    # functionality.
+    format_group.add_argument("-q", "--quiet",
+            help="Suppress no error output",
+            action="store_true",
+            default=False)
+    format_group.add_argument("-s", "--succinct",
+            help="Reduce amount of output",
+            action="store_true",
+            default=False)
+    format_group.add_argument("-v", "--verbose",
+            dest="showOutput",
+            help="Show test output for failures",
+            action="store_true",
+            default=False)
+    format_group.add_argument("-vv", "--echo-all-commands",
+            dest="echoAllCommands",
+            action="store_true",
+            default=False,
+            help="Echo all commands as they are executed to stdout. In case of "
+                 "failure, last command shown will be the failing one.")
+    format_group.add_argument("-a", "--show-all",
+            dest="showAllOutput",
+            help="Display all commandlines and output",
+            action="store_true",
+            default=False)
+    format_group.add_argument("-o", "--output",
+            dest="output_path",
+            help="Write test results to the provided path",
+            action="store",
+            metavar="PATH")
+    format_group.add_argument("--no-progress-bar",
+            dest="useProgressBar",
+            help="Do not use curses based progress bar",
+            action="store_false",
+            default=True)
+    format_group.add_argument("--show-unsupported",
+            help="Show unsupported tests",
+            action="store_true",
+            default=False)
+    format_group.add_argument("--show-xfail",
+            help="Show tests that were expected to fail",
+            action="store_true",
+            default=False)
+
+    execution_group = parser.add_argument_group("Test Execution")
+    execution_group.add_argument("--path",
+            help="Additional paths to add to testing environment",
+            action="append",
+            type=str,
+            default=[])
+    execution_group.add_argument("--vg",
+            dest="useValgrind",
+            help="Run tests under valgrind",
+            action="store_true",
+            default=False)
+    execution_group.add_argument("--vg-leak",
+            dest="valgrindLeakCheck",
+            help="Check for memory leaks under valgrind",
+            action="store_true",
+            default=False)
+    execution_group.add_argument("--vg-arg",
+            dest="valgrindArgs",
+            metavar="ARG",
+            help="Specify an extra argument for valgrind",
+            type=str,
+            action="append",
+            default=[])
+    execution_group.add_argument("--time-tests",
+            dest="timeTests",
+            help="Track elapsed wall time for each test",
+            action="store_true",
+            default=False)
+    execution_group.add_argument("--no-execute",
+            dest="noExecute",
+            help="Don't execute any tests (assume PASS)",
+            action="store_true",
+            default=False)
+    execution_group.add_argument("--xunit-xml-output",
+            dest="xunit_output_file",
+            help="Write XUnit-compatible XML test reports to the specified file",
+            default=None)
+    execution_group.add_argument("--timeout",
+            dest="maxIndividualTestTime",
+            help="Maximum time to spend running a single test (in seconds). "
+                 "0 means no time limit. [Default: 0]",
+            type=int,
+            default=None)
+    execution_group.add_argument("--max-failures",
+            dest="maxFailures",
+            help="Stop execution after the given number of failures.",
+            action="store",
+            type=int,
+            default=None)
+
+    selection_group = parser.add_argument_group("Test Selection")
+    selection_group.add_argument("--max-tests",
+            dest="maxTests",
+            metavar="N",
+            help="Maximum number of tests to run",
+            action="store",
+            type=int,
+            default=None)
+    selection_group.add_argument("--max-time",
+            dest="maxTime",
+            metavar="N",
+            help="Maximum time to spend testing (in seconds)",
+            action="store",
+            type=float,
+            default=None)
+    selection_group.add_argument("--shuffle",
+            help="Run tests in random order",
+            action="store_true",
+            default=False)
+    selection_group.add_argument("-i", "--incremental",
+            help="Run modified and failing tests first (updates mtimes)",
+            action="store_true",
+            default=False)
+    selection_group.add_argument("--filter",
+            metavar="REGEX",
+            help="Only run tests with paths matching the given regular expression",
+            action="store",
+            default=os.environ.get("LIT_FILTER"))
+    selection_group.add_argument("--num-shards", dest="numShards", metavar="M",
+            help="Split testsuite into M pieces and only run one",
+            action="store",
+            type=int,
+            default=os.environ.get("LIT_NUM_SHARDS"))
+    selection_group.add_argument("--run-shard",
+            dest="runShard",
+            metavar="N",
+            help="Run shard #N of the testsuite",
+            action="store",
+            type=int,
+            default=os.environ.get("LIT_RUN_SHARD"))
+
+    debug_group = parser.add_argument_group("Debug and Experimental Options")
+    debug_group.add_argument("--debug",
+            help="Enable debugging (for 'lit' development)",
+            action="store_true",
+            default=False)
+    debug_group.add_argument("--show-suites",
+            dest="showSuites",
+            help="Show discovered test suites",
+            action="store_true",
+            default=False)
+    debug_group.add_argument("--show-tests",
+            dest="showTests",
+            help="Show all discovered tests",
+            action="store_true",
+            default=False)
+
+    opts = parser.parse_args(sys.argv[1:] +
+                             shlex.split(os.environ.get("LIT_OPTS", "")))
+
+    # Validate options
+    if not opts.test_paths:
+        parser.error('No inputs specified')
+
+    if opts.numWorkers is None:
+        opts.numWorkers = lit.util.detectCPUs()
+    elif opts.numWorkers <= 0:
+        parser.error("Option '--workers' or '-j' requires positive integer")
+
+    if opts.maxFailures is not None and opts.maxFailures <= 0:
+        parser.error("Option '--max-failures' requires positive integer")
+
+    if opts.echoAllCommands:
+        opts.showOutput = True
+
+    if (opts.numShards is not None) or (opts.runShard is not None):
+        if (opts.numShards is None) or (opts.runShard is None):
+            parser.error("--num-shards and --run-shard must be used together")
+        if opts.numShards <= 0:
+            parser.error("--num-shards must be positive")
+        if (opts.runShard < 1) or (opts.runShard > opts.numShards):
+            parser.error("--run-shard must be between 1 and --num-shards (inclusive)")
+
+    return opts

llvm/utils/lit/lit/main.py

 #!/usr/bin/env python
 
 """
 lit - LLVM Integrated Tester.
 
 See lit.pod for more information.
 """
 
 from __future__ import absolute_import
 import os
 import platform
 import random
 import re
-import shlex
 import sys
 import time
-import argparse
 import tempfile
 import shutil
 from xml.sax.saxutils import quoteattr
 
+import lit.cl_arguments
 import lit.discovery
 import lit.display
 import lit.LitConfig
 import lit.run
 import lit.Test
 import lit.util
 
 def write_test_results(run, lit_config, testing_time, output_path):
finally:
         f.close()
 
 def update_incremental_cache(test):
     if not test.result.code.isFailure:
         return
     fname = test.getFilePath()
     os.utime(fname, None)
 
-def sort_by_incremental_cache(run):
-    def sortIndex(test):
-        fname = test.getFilePath()
-        try:
-            return -os.path.getmtime(fname)
-        except:
-            return 0
-    run.tests.sort(key = lambda t: sortIndex(t))
+def by_mtime(test):
+        return os.path.getmtime(fname)
 
 def main(builtinParameters = {}):
     # Create a temp directory inside the normal temp directory so that we can
     # try to avoid temporary test file leaks. The user can avoid this behavior
     # by setting LIT_PRESERVES_TMP in the environment, so they can easily use
     # their own temp directory to monitor temporary file leaks or handle them at
     # the buildbot level.
     lit_tmp = None
finally:
         if lit_tmp:
             try:
                 shutil.rmtree(lit_tmp)
             except:
                 # FIXME: Re-try after timeout on Windows.
                 pass
 
 def main_with_tmp(builtinParameters):
-    parser = argparse.ArgumentParser()
-    parser.add_argument('test_paths',
-                        nargs='*',
-                        help='Files or paths to include in the test suite')
-
-    parser.add_argument("--version", dest="show_version",
-                      help="Show version and exit",
-                      action="store_true", default=False)
-    parser.add_argument("-j", "--threads", "--workers", dest="numWorkers", metavar="N",
-                      help="Number of workers used for testing",
-                      type=int, default=None)
-    parser.add_argument("--config-prefix", dest="configPrefix",
-                      metavar="NAME", help="Prefix for 'lit' config files",
-                      action="store", default=None)
-    parser.add_argument("-D", "--param", dest="userParameters",
-                      metavar="NAME=VAL",
-                      help="Add 'NAME' = 'VAL' to the user defined parameters",
-                      type=str, action="append", default=[])
-
-    format_group = parser.add_argument_group("Output Format")
-    # FIXME: I find these names very confusing, although I like the
-    # functionality.
-    format_group.add_argument("-q", "--quiet",
-                     help="Suppress no error output",
-                     action="store_true", default=False)
-    format_group.add_argument("-s", "--succinct",
-                     help="Reduce amount of output",
-                     action="store_true", default=False)
-    format_group.add_argument("-v", "--verbose", dest="showOutput",
-                     help="Show test output for failures",
-                     action="store_true", default=False)
-    format_group.add_argument("-vv", "--echo-all-commands",
-                     dest="echoAllCommands",
-                     action="store_true", default=False,
-                     help="Echo all commands as they are executed to stdout.\
-                     In case of failure, last command shown will be the\
-                     failing one.")
-    format_group.add_argument("-a", "--show-all", dest="showAllOutput",
-                     help="Display all commandlines and output",
-                     action="store_true", default=False)
-    format_group.add_argument("-o", "--output", dest="output_path",
-                     help="Write test results to the provided path",
-                     action="store", metavar="PATH")
-    format_group.add_argument("--no-progress-bar", dest="useProgressBar",
-                     help="Do not use curses based progress bar",
-                     action="store_false", default=True)
-    format_group.add_argument("--show-unsupported",
-                     help="Show unsupported tests",
-                     action="store_true", default=False)
-    format_group.add_argument("--show-xfail",
-                     help="Show tests that were expected to fail",
-                     action="store_true", default=False)
-
-    execution_group = parser.add_argument_group("Test Execution")
-    execution_group.add_argument("--path",
-                     help="Additional paths to add to testing environment",
-                     action="append", type=str, default=[])
-    execution_group.add_argument("--vg", dest="useValgrind",
-                     help="Run tests under valgrind",
-                     action="store_true", default=False)
-    execution_group.add_argument("--vg-leak", dest="valgrindLeakCheck",
-                     help="Check for memory leaks under valgrind",
-                     action="store_true", default=False)
-    execution_group.add_argument("--vg-arg", dest="valgrindArgs", metavar="ARG",
-                     help="Specify an extra argument for valgrind",
-                     type=str, action="append", default=[])
-    execution_group.add_argument("--time-tests", dest="timeTests",
-                     help="Track elapsed wall time for each test",
-                     action="store_true", default=False)
-    execution_group.add_argument("--no-execute", dest="noExecute",
-                     help="Don't execute any tests (assume PASS)",
-                     action="store_true", default=False)
-    execution_group.add_argument("--xunit-xml-output", dest="xunit_output_file",
-                      help=("Write XUnit-compatible XML test reports to the"
-                            " specified file"), default=None)
-    execution_group.add_argument("--timeout", dest="maxIndividualTestTime",
-                     help="Maximum time to spend running a single test (in seconds)."
-                     "0 means no time limit. [Default: 0]",
-                    type=int, default=None)
-    execution_group.add_argument("--max-failures", dest="maxFailures",
-                     help="Stop execution after the given number of failures.",
-                     action="store", type=int, default=None)
-
-    selection_group = parser.add_argument_group("Test Selection")
-    selection_group.add_argument("--max-tests", dest="maxTests", metavar="N",
-                     help="Maximum number of tests to run",
-                     action="store", type=int, default=None)
-    selection_group.add_argument("--max-time", dest="maxTime", metavar="N",
-                     help="Maximum time to spend testing (in seconds)",
-                     action="store", type=float, default=None)
-    selection_group.add_argument("--shuffle",
-                     help="Run tests in random order",
-                     action="store_true", default=False)
-    selection_group.add_argument("-i", "--incremental",
-                     help="Run modified and failing tests first (updates "
-                     "mtimes)",
-                     action="store_true", default=False)
-    selection_group.add_argument("--filter", metavar="REGEX",
-                     help=("Only run tests with paths matching the given "
-                           "regular expression"),
-                     action="store",
-                     default=os.environ.get("LIT_FILTER"))
-    selection_group.add_argument("--num-shards", dest="numShards", metavar="M",
-                     help="Split testsuite into M pieces and only run one",
-                     action="store", type=int,
-                     default=os.environ.get("LIT_NUM_SHARDS"))
-    selection_group.add_argument("--run-shard", dest="runShard", metavar="N",
-                     help="Run shard #N of the testsuite",
-                     action="store", type=int,
-                     default=os.environ.get("LIT_RUN_SHARD"))
-
-    debug_group = parser.add_argument_group("Debug and Experimental Options")
-    debug_group.add_argument("--debug",
-                      help="Enable debugging (for 'lit' development)",
-                      action="store_true", default=False)
-    debug_group.add_argument("--show-suites", dest="showSuites",
-                      help="Show discovered test suites",
-                      action="store_true", default=False)
-    debug_group.add_argument("--show-tests", dest="showTests",
-                      help="Show all discovered tests",
-                      action="store_true", default=False)
-
-    opts = parser.parse_args(sys.argv[1:] +
-                             shlex.split(os.environ.get("LIT_OPTS", "")))
-    args = opts.test_paths
+    opts = lit.cl_arguments.parse_args()
 
     if opts.show_version:
         print("lit %s" % (lit.__version__,))
         return
 
-    if not args:
-        parser.error('No inputs specified')
-
-    if opts.numWorkers is None:
-        opts.numWorkers = lit.util.detectCPUs()
-    elif opts.numWorkers <= 0:
-        parser.error("Option '--workers' or '-j' requires positive integer")
-
-    if opts.maxFailures is not None and opts.maxFailures <= 0:
-        parser.error("Option '--max-failures' requires positive integer")
-
-    if opts.echoAllCommands:
-        opts.showOutput = True
-
-    inputs = args
-
     # Create the user defined parameters.
     userParams = dict(builtinParameters)
     for entry in opts.userParameters:
         if '=' not in entry:
             name,val = entry,''
         else:
             name,val = entry.split('=', 1)
         userParams[name] = val
litConfig = lit.LitConfig.LitConfig(
         config_prefix = opts.configPrefix,
         maxIndividualTestTime = maxIndividualTestTime,
         maxFailures = opts.maxFailures,
         parallelism_groups = {},
         echo_all_commands = opts.echoAllCommands)
 
     # Perform test discovery.
     run = lit.run.Run(litConfig,
-                      lit.discovery.find_tests_for_inputs(litConfig, inputs))
+                      lit.discovery.find_tests_for_inputs(litConfig, opts.test_paths))
 
     # After test discovery the configuration might have changed
     # the maxIndividualTestTime. If we explicitly set this on the
     # command line then override what was set in the test configuration
     if opts.maxIndividualTestTime is not None:
         if opts.maxIndividualTestTime != litConfig.maxIndividualTestTime:
             litConfig.note(('The test suite configuration requested an individual'
                 ' test timeout of {0} seconds but a timeout of {1} seconds was'
if opts.filter:
                     opts.filter))
         run.tests = [result_test for result_test in run.tests
                      if rex.search(result_test.getFullName())]
 
     # Then select the order.
     if opts.shuffle:
         random.shuffle(run.tests)
     elif opts.incremental:
-        sort_by_incremental_cache(run)
+        run.tests.sort(key=by_mtime, reverse=True)
     else:
         run.tests.sort(key = lambda t: (not t.isEarlyTest(), t.getFullName()))
 
     # Then optionally restrict our attention to a shard of the tests.
     if (opts.numShards is not None) or (opts.runShard is not None):
-        if (opts.numShards is None) or (opts.runShard is None):
-            parser.error("--num-shards and --run-shard must be used together")
-        if opts.numShards <= 0:
-            parser.error("--num-shards must be positive")
-        if (opts.runShard < 1) or (opts.runShard > opts.numShards):
-            parser.error("--run-shard must be between 1 and --num-shards (inclusive)")
         num_tests = len(run.tests)
         # Note: user views tests and shard numbers counting from 1.
         test_ixs = range(opts.runShard - 1, num_tests, opts.numShards)
         run.tests = [run.tests[i] for i in test_ixs]
         # Generate a preview of the first few test indices in the shard
         # to accompany the arithmetic expression, for clarity.
         preview_len = 3
         ix_preview = ", ".join([str(i+1) for i in test_ixs[:preview_len]])