<div dir="ltr">Oops, I looked at that and I broke something in V2. I have V3 patches that address that issue I'm sending now.</div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Mar 8, 2013 at 9:06 AM, Jordan Justen <<a href="mailto:jljusten@gmail.com" target="_blank">jljusten@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Mar 5, 2013 at 4:47 PM, Dylan Baker <<a href="mailto:baker.dylan.c@gmail.com">baker.dylan.c@gmail.com</a>> wrote: > This change gets us cleaner, more readable argument parsing code. > Another advantage of this approach is that it automatically generates > help menus, which means options will not be implemented without a help > entry (like --resume) > > V2: - Does not remove deprecated options, only marks them as deprecated > > Signed-off-by: Dylan Baker <<a href="mailto:baker.dylan.c@gmail.com">baker.dylan.c@gmail.com</a>> > --- > piglit-print-commands.py | 98 +++++++------------ > piglit-run.py | 249 +++++++++++++++++++++++------------------------ > 2 files changed, 159 insertions(+), 188 deletions(-) > > diff --git a/piglit-print-commands.py b/piglit-print-commands.py > index 3479ef1..9dd2290 100755 > --- a/piglit-print-commands.py > +++ b/piglit-print-commands.py > @@ -22,7 +22,7 @@ > # DEALINGS IN THE SOFTWARE. > > > -from getopt import getopt, GetoptError > +import argparse > import os.path as path > import re > import sys, os > @@ -38,77 +38,51 @@ from framework.gleantest import GleanTest > ############################################################################# > ##### Main program > ############################################################################# > -def usage(): > - USAGE = """\ > -Usage: %(progName)s [options] [profile.tests] > - > -Prints a list of all the tests and how to run them. Ex: > - piglit test name ::: /path/to/piglit/bin/program <args> > - glean test name ::: PIGLIT_TEST='...' /path/to/piglit/bin/glean -v -v -v ... > - > -Options: > - -h, --help Show this message > - -t regexp, --include-tests=regexp Run only matching tests (can be used more > - than once) > - --tests=regexp Run only matching tests (can be used more > - than once) DEPRICATED use --include-tests instead > - -x regexp, --exclude-tests=regexp Exclude matching tests (can be used > - more than once) > -Example: > - %(progName)s tests/all.tests > - %(progName)s -t basic tests/all.tests > - Print tests whose path contains the word 'basic' > - > - %(progName)s -t ^glean/ -t tex tests/all.tests > - Print tests that are in the 'glean' group or whose path contains > - the substring 'tex' > -""" > - print USAGE % {'progName': sys.argv[0]} > - sys.exit(1) > > def main(): > - env = core.Environment() > + parser = argparse.ArgumentParser(sys.argv) > + > + parser.add_argument("-t", "--include-tests", > + default = [], > + action = "append", > + metavar = "<regex>", > + help = "Run only matching tests (can be used more than once)") > + parser.add_argument("--tests", > + default = [], > + action = "append", > + metavar = "<regex>", > + help = "Run only matching tests (can be used more than once)" \ > + "Deprecated") > + parser.add_argument("-x", "--exclude-tests", > + default = [], > + action = "append", > + metavar = "<regex>", > + help = "Exclude matching tests (can be used more than once)") > + parser.add_argument("testProfile", > + metavar = "<Path to testfile>", > + help = "Path to results folder") > + > + args = parser.parse_args() > > - try: > - option_list = [ > - "help", > - "tests=", > - "include-tests=", > - "exclude-tests=", > - ] > - options, args = getopt(sys.argv[1:], "ht:x:", option_list) > - except GetoptError: > - usage() > - > - OptionName = '' > - test_filter = [] > - exclude_filter = [] > - > - for name, value in options: > - if name in ('-h', '--help'): > - usage() > - elif name in ('-t', '--include-tests'): > - test_filter.append(value) > - env.filter.append(re.compile(value)) > - elif name in ('--tests'): > - test_filter.append(value) > - env.filter.append(re.compile(value)) > - print "Warning: Option --tets is deprecated, " \ > - "use --include-tests instead" > - elif name in ('-x', '--exclude-tests'): > - exclude_filter.append(value) > - env.exclude_filter.append(re.compile(value)) > + env = core.Environment() > > - if len(args) != 1: > - usage() > + # If --tests is called warn that it is deprecated > + if args.tests is no []: > + print "Warning: Option --tests is deprecated. Use --include-tests" > > - profileFilename = args[0] > + # Append includes and excludes to env > + for each in args.include_tests: > + env.filter.append(re.compile(each)) > + for each in args.tests: > + env.filter.append(re.compile(each)) > + for each in args.exclude_tests: > + env.exclude_filter.append(re.compile(each)) > > # Change to the piglit's path > piglit_dir = path.dirname(path.realpath(sys.argv[0])) > os.chdir(piglit_dir) > > - profile = core.loadTestProfile(profileFilename) > + profile = core.loadTestProfile(args.testFile) > > def getCommand(test): > command = '' > diff --git a/piglit-run.py b/piglit-run.py > index 599981d..1a50651 100755 > --- a/piglit-run.py > +++ b/piglit-run.py > @@ -22,7 +22,7 @@ > # DEALINGS IN THE SOFTWARE. > > > -from getopt import getopt, GetoptError > +import argparse > import os.path as path > import re > import sys, os > @@ -37,123 +37,112 @@ from framework.threads import synchronized_self > ############################################################################# > ##### Main program > ############################################################################# > -def usage(): > - USAGE = """\ > -Usage: %(progName)s [options] [profile.tests] [results] > - %(progName)s [options] -r [results] > - > -Options: > - -h, --help Show this message > - -d, --dry-run Do not execute the tests > - -t regexp, --include-tests=regexp Run only matching tests (can be used more > - than once) > - --tests=regexp Run only matching tests (can be used more > - than once) DEPRICATED: use --include-tests instead > - -x regexp, --exclude-tests=regexp Excludey matching tests (can be used > - more than once) > - -n name, --name=name Name of the testrun > - -c bool, --concurrent= Enable/disable concurrent test runs. Valid > - option values are: 0, 1, on, off. (default: on) > - DEPRICATED: use --no-concurrency to turn > - concurrent test runs off > - --no-concurrency Disables concurrent test runs > - --valgrind Run tests in valgrind's memcheck. > - -p platform, --platform=platform Name of the piglit platform to use. > - --resume Resume and interupted test run > - > -Example: > - %(progName)s tests/all.tests results/all > - Run all tests, store the results in the directory results/all > - > - %(progName)s -t basic tests/all.tests results/all > - Run all tests whose path contains the word 'basic' > - > - %(progName)s -t ^glean/ -t tex tests/all.tests results/all > - Run all tests that are in the 'glean' group or whose path contains > - the substring 'tex' > - > - %(progName)s -r -x bad-test results/all > - Resume an interrupted test run whose results are stored in the > - directory results/all, skipping bad-test. > -""" > - print USAGE % {'progName': sys.argv[0]} > - sys.exit(1) > > def main(): > env = core.Environment() > > - try: > - option_list = [ > - "help", > - "dry-run", > - "resume", > - "valgrind", > - "include-tests=", > - "tests=", > - "name=", > - "exclude-tests=", > - "no-concurrency", > - "concurrent=", > - "platform=", > - ] > - options, args = getopt(sys.argv[1:], "hdrt:n:x:c:p:", option_list) > - except GetoptError: > - usage() > - > - OptionName = '' > - OptionResume = False > - test_filter = [] > - exclude_filter = [] > - platform = None > - > - for name, value in options: > - if name in ('-h', '--help'): > - usage() > - elif name in ('-d', '--dry-run'): > - env.execute = False > - elif name in ('-r', '--resume'): > - OptionResume = True > - elif name in ('--valgrind'): > - env.valgrind = True > - elif name in ('-t', '--include-tests'): > - test_filter.append(value) > - env.filter.append(re.compile(value)) > - elif name in ('--tests'): > - test_filter.append(value) > - env.filter.append(re.compile(value)) > - print "Warning: Option --tests is deprecated, " \ > - "use --include-tests instead" > - elif name in ('-x', '--exclude-tests'): > - exclude_filter.append(value) > - env.exclude_filter.append(re.compile(value)) > - elif name in ('-n', '--name'): > - OptionName = value > - elif name in ('--no-concurrency'): > + parser = argparse.ArgumentParser(sys.argv) > + > + > + # Either require that a name for the test is passed or that > + # resume is requested > + excGroup1 = parser.add_mutually_exclusive_group() > + excGroup1.add_argument("-n", "--name", > + metavar = "<test name>", > + default = None, > + help = "Name of this test run") > + excGroup1.add_argument("-r", "--resume", > + action = "store_true", > + help = "Resume an interupted test run") > + > + parser.add_argument("-d", "--dry-run", > + action = "store_true", > + help = "Do not execute the tests") > + parser.add_argument("-t", "--include-tests", > + default = [], > + action = "append", > + metavar = "<regex>", > + help = "Run only matching tests (can be used more than once)") > + parser.add_argument("--tests", > + default = [], > + action = "append", > + metavar = "<regex>", > + help = "Run only matching tests (can be used more than once) " \ > + "DEPRECATED: use --include-tests instead") > + parser.add_argument("-x", "--exclude-tests", > + default = [], > + action = "append", > + metavar = "<regex>", > + help = "Exclude matching tests (can be used more than once)") > + > + # The new option going forward should be --no-concurrency, but to > + # maintain backwards compatability the --c, --concurrent option should > + # also be maintained. This code allows only one of the two options to be > + # supplied, or it throws an error > + excGroup2 = parser.add_mutually_exclusive_group() > + excGroup2.add_argument("--no-concurrency", > + action = "store_true", > + help = "Disable concurrent test runs") > + excGroup2.add_argument("-c", "--concurrent", > + action = "store", > + metavar = "<boolean>", > + choices = ["1", "0", "on", "off"], > + help = "Deprecated: Turn concrrent runs on or off") > + > + parser.add_argument("-p", "--platform", > + choices = ["glx", "x11_egl", "wayland", "gbm"], > + help = "Name of windows system passed to waffle") > + parser.add_argument("--valgrind", > + action = "store_true", > + help = "Run tests in valgrind's memcheck") > + parser.add_argument("testProfile", > + metavar = "<Path to test profile>", > + help = "Path to testfile to run") > + parser.add_argument("resultsPath", > + metavar = "<Results Path>", > + help = "Path to results folder") > + > + args = parser.parse_args() > + > + > + # Set the platform to pass to waffle > + if args.platform is not None: > + os.environ['PIGLIT_PLATFORM'] = args.platform > + > + # Set dry-run > + if args.dry_run is True: > + env.execute = False > + > + # Set valgrind > + if args.valgrind is True: > + env.valgrind = True > + > + # Turn concurency off if requested > + # Deprecated > + if args.concurrent is not None: > + if (args.concurrent == '1' or args.concurrent == 'on'): > + env.concurrent = True > + print "Warning: Option -c, --concurrent is deprecated, " \ > + "concurrent test runs are on by default" > + elif (args.concurrent == '0' or args.concurrent == 'off'): > env.concurrent = False > - elif name in ('-c', '--concurrent'): > - if value in ('1', 'on'): > - env.concurrent = True > - print "Warning: Option -c, --concurrent is deprecated, " \ > - "concurrent test runs are on by default" > - elif value in ('0', 'off'): > - env.concurrent = False > - print "Warning: Option -c, --concurrent is deprecated, " \ > - "use --no-concurrency for non-concurrent test runs" > - else: > - usage() > - elif name in ('-p, --platform'): > - platform = value > - > - if platform is not None: > - os.environ['PIGLIT_PLATFORM'] = platform > - > - if OptionResume: > - if test_filter or OptionName: > - print "-r is not compatible with -t or -n." > - usage() > - if len(args) != 1: > - usage() > - resultsDir = path.realpath(args[0]) > + print "Warning: Option -c, --concurrent is deprecated, " \ > + "use --no-concurrent for non-concurrent test runs" > + # Ne need for else, since argparse restricts the arguments allowed > + > + # Not deprecated > + elif args.no_concurrency is True: > + env.concurrent = False > + > + # If the deprecated tests option was passed print a warning > + if args.tests is not []: > + print "Warning: Option --tests is deprecated, use " \ > + "--include-tests instead" </div></div>I got the warning message when using -t. I think we may want 'if len(args.tests) > 0' instead. Series Reviewed-by: Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>> I'll commit the series with this change later today if no further comments come in. Thanks, -Jordan <div class="HOEnZb"><div class="h5"> > + > + # If resume is requested attempt to load the results file > + # in the specified path > + if args.resume is True: > + resultsDir = path.realpath(args.resultsPath) > > # Load settings from the old results JSON > old_results = core.loadTestResults(resultsDir) > @@ -164,14 +153,21 @@ def main(): > for value in old_results.options['exclude_filter']: > exclude_filter.append(value) > env.exclude_filter.append(re.compile(value)) > - else: > - if len(args) != 2: > - usage() > > - profileFilename = args[0] > - resultsDir = path.realpath(args[1]) > - > - # Change to the piglit's path > + # Otherwise parse additional settings from the command line > + else: > + profileFilename = args.testProfile > + resultsDir = args.resultsPath > + > + # Set the excluded and included tests regex > + for each in args.include_tests: > + env.filter.append(re.compile(each)) > + for each in args.tests: > + env.filter.append(re.compile(each)) > + for each in args.exclude_tests: > + env.exclude_filter.append(re.compile(each)) > + > + # Change working directory to the root of the piglit directory > piglit_dir = path.dirname(path.realpath(sys.argv[0])) > os.chdir(piglit_dir) > > @@ -180,10 +176,10 @@ def main(): > results = core.TestrunResult() > > # Set <a href="http://results.name" target="_blank">results.name</a> > - if OptionName is '': > - <a href="http://results.name" target="_blank">results.name</a> = path.basename(resultsDir) > + if <a href="http://args.name" target="_blank">args.name</a> is not None: > + <a href="http://results.name" target="_blank">results.name</a> = <a href="http://args.name" target="_blank">args.name</a> > else: > - <a href="http://results.name" target="_blank">results.name</a> = OptionName > + <a href="http://results.name" target="_blank">results.name</a> = path.basename(resultsDir) > > # Begin json. > result_filepath = os.path.join(resultsDir, 'main') > @@ -196,9 +192,9 @@ def main(): > json_writer.open_dict() > json_writer.write_dict_item('profile', profileFilename) > json_writer.write_dict_key('filter') > - result_file.write(json.dumps(test_filter)) > + result_file.write(json.dumps(args.include_tests)) > json_writer.write_dict_key('exclude_filter') > - result_file.write(json.dumps(exclude_filter)) > + result_file.write(json.dumps(args.exclude_tests)) > json_writer.close_dict() > > json_writer.write_dict_item('name', <a href="http://results.name" target="_blank">results.name</a>) > @@ -212,7 +208,7 @@ def main(): > # If resuming an interrupted test run, re-write all of the existing > # results since we clobbered the results file. Also, exclude them > # from being run again. > - if OptionResume: > + if args.resume is True: > for (key, value) in old_results.tests.items(): > if os.path.sep != '/': > key = key.replace(os.path.sep, '/', -1) > @@ -236,5 +232,6 @@ def main(): > print 'Thank you for running Piglit!' > print 'Results have been written to ' + result_filepath > > + > if __name__ == "__main__": > main() > -- > 1.8.1.4 > </div></div><div class="HOEnZb"><div class="h5">> _______________________________________________ > Piglit mailing list > <a href="mailto:Piglit@lists.freedesktop.org">Piglit@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/piglit" target="_blank">http://lists.freedesktop.org/mailman/listinfo/piglit</a> </div></div></blockquote></div> </div>