Comments on the patch inline...
On Tue, Feb 9, 2010 at 12:58 PM, John Admanski <[email protected]> wrote:
>
> Index: client/bin/setup_job.py
> ===================================================================
> --- client/bin/setup_job.py (revision 4228)
> +++ client/bin/setup_job.py (working copy)
> @@ -2,67 +2,134 @@
> #
> # Eric Li <[email protected]>
>
> -import logging, os, pickle, shutil, sys
> +import logging, os, pickle, re, sys
> +
> +from autotest_lib.client.bin import client_logging_config
> +from autotest_lib.client.bin import job as client_job
> from autotest_lib.client.bin import utils
> +from autotest_lib.client.common_lib import base_job
> +from autotest_lib.client.common_lib import error
> +from autotest_lib.client.common_lib import logging_manager
> +from autotest_lib.client.common_lib import packages
>
> -def initialize(client_job):
> - cwd = os.getcwd()
> - os.chdir(client_job.autodir)
> - os.system('tools/make_clean')
> - os.chdir(cwd)
>
> - sys.path.insert(0, client_job.bindir)
> +class setup_job(client_job.job):
> + """
> + setup_job is a job which runs client test setup() method at server side.
>
> - os.environ['AUTODIR'] = client_job.autodir
> - os.environ['AUTODIRBIN'] = client_job.bindir
> - os.environ['PYTHONPATH'] = client_job.bindir
> + This job is used to pre-setup client tests when development toolchain is
> not
> + available at client.
> + """
>
> + def __init__(self, options):
> + base_job.base_job.__init__(self, options=options)
>
> This __init__ call should be using super(), not an explicit base_job call;
or are you actually trying to explicitly bypass the client_job __init__? If
so, that's pretty fragile; I'm worried about code that wants to inherit from
the client job class but not use it's initialization, it seems very
susceptible to breakage when changes are made to the client job class.
>
> + logging_manager.configure_logging(
> + client_logging_config.ClientLoggingConfig(),
> + results_dir=self.resultdir,
> + verbose=options.verbose)
> + self._cleanup_results_dir()
> + self.pkgmgr = packages.PackageManager(
> + self.autodir, run_function_dargs={'timeout':3600})
>
> -# This function was inspired from runtest() on client/common_lib/test.py.
> -# Same logic to instantiate a client test object.
> -def setup_test(testname, client_job):
> - logging.info('setup %s.' % testname)
>
> +def load_all_client_tests(options):
> + """
> + Load and instantiate all client tests.
> +
> + This function is inspired from runtest() on client/common_lib/test.py.
> + """
> +
>
> These comments (and any other ones in here) really should adhere to the
style in CODING_STYLE.
> local_namespace = locals().copy()
> global_namespace = globals().copy()
>
> - outputdir = os.path.join(client_job.resultdir, testname)
> - try:
> - os.makedirs(outputdir)
> - except OSError, oe:
> - print oe
> + all_tests = []
> + for test_base_dir in ['tests', 'site_tests']:
> + testdir = os.path.join(os.environ['AUTODIR'], test_base_dir)
> + for test_name in os.listdir(testdir):
> + job = setup_job(options=options)
> + testbindir = os.path.join(testdir, test_name)
> + local_namespace['testbindir'] = testbindir
>
> - local_namespace['job'] = client_job
> - local_namespace['outputdir'] = outputdir
> + outputdir = os.path.join(job.resultdir, test_name)
> + try:
> + os.makedirs(outputdir)
> + except OSError:
> + pass
>
> - # if the test is local, it can be found in either testdir or
> site_testdir.
> - # tests in site_testdir override tests defined in testdir.
> - testdir = os.path.join(client_job.autodir, 'site_tests')
> - testbindir = os.path.join(testdir, testname)
> - if not os.path.exists(testbindir):
> - testdir = os.path.join(client_job.autodir, 'tests')
> - testbindir = os.path.join(testdir, testname)
> - local_namespace['testbindir'] = testbindir
> - sys.path.insert(0, testbindir)
> + local_namespace['job'] = job
> + local_namespace['outputdir'] = outputdir
>
> - try:
> - exec("import %s" % testname, local_namespace, global_namespace)
> - exec("auto_test = %s.%s(job, testbindir, outputdir)" %
> - (testname, testname), local_namespace, global_namespace)
> - finally:
> - sys.path.pop(0) # pop up testbindir
> + try:
> + sys.path.insert(0, testbindir)
> + try:
> + exec("import %s" % test_name, local_namespace,
> + global_namespace)
> + exec("auto_test = %s.%s(job, testbindir, outputdir)" %
> + (test_name, test_name), local_namespace,
> + global_namespace)
> + client_test = global_namespace['auto_test']
> + all_tests.append(client_test)
> + except ImportError:
> + # this test has only control file but no python.
> + pass
> + finally:
> + sys.path.pop(0) # pop up testbindir
>
> A finally block should not be trying to revert changes from inside the try
block, assuming they were successful. Either the insert needs to be pulled
outside of the try, or the finally needs to check if insert actually ran. In
this case I suspect you just want to do the former.
>
> + return all_tests
>
> - pwd = os.getcwd()
> - os.chdir(outputdir)
>
> - try:
> - auto_test = global_namespace['auto_test']
> - auto_test.setup()
> +def setup_tests(options):
> + assert options.setup, 'Specify setup tests on the command line.'
>
> - # touch .version file under src to prevent further setup on client
> host.
> - # see client/common_lib/utils.py update_version()
> - versionfile = os.path.join(auto_test.srcdir, '.version')
> - pickle.dump(auto_test.version, open(versionfile, 'w'))
> - finally:
> - os.chdir(pwd)
> - shutil.rmtree(auto_test.tmpdir, ignore_errors=True)
> + requested_tests = options.setup.split(',')
> + candidates = load_all_client_tests(options)
> +
> + rex = re.compile(r'ALL', re.IGNORECASE)
> + if rex.search(options.setup):
>
> Using a regular expression here just to handle mixed case seems overkill,
why not just do 'all' in options.setup.lower()?
> + need_to_setup = candidates
> + else:
> + need_to_setup = []
> + for candidate in candidates:
> + if candidate.__class__.__name__ in requested_tests:
> + need_to_setup.append(candidate)
> +
> + if need_to_setup:
> + cwd = os.getcwd()
> + os.chdir(need_to_setup[0].job.clientdir)
> + os.system('tools/make_clean')
> + os.chdir(cwd)
> +
> + failed_tests = []
> + for setup_test in need_to_setup:
> + test_name = setup_test.__class__.__name__
> + try:
> + outputdir = os.path.join(setup_test.job.resultdir, test_name)
> + try:
> + os.makedirs(outputdir)
> + os.chdir(outputdir)
> + except OSError:
> + pass
> + logging.info('setup %s.' % test_name)
> + setup_test.setup()
> + # Touch .version file under src to prevent further setup on
> client
> + # host. See client/common_lib/utils.py update_version()
> + if os.path.exists(setup_test.srcdir):
> + versionfile = os.path.join(setup_test.srcdir, '.version')
> + pickle.dump(setup_test.version, open(versionfile, 'w'))
> + except Exception, err:
> + logging.error(err)
> + failed_tests.append(test_name)
> +
> + logging.info('############################# SUMMARY '
> + '#############################')
> +
> + # Print out tests that failed
> + if failed_tests:
> + logging.info('Finished setup -- The following tests failed')
> + for failed_test in failed_tests:
> + logging.info(failed_test)
> + else:
> + logging.info('Finished setup -- All tests built successfully')
> + logging.info('######################### END SUMMARY '
> + '##############################')
> + if failed_tests:
> + raise error.AutoservError('Finished setup with errors.')
> Index: client/bin/autotest
> ===================================================================
> --- client/bin/autotest (revision 4228)
> +++ client/bin/autotest (working copy)
> @@ -2,8 +2,7 @@
> #
> # autotest <control file> - run the autotest control file specified.
> #
> -import os, sys, shutil
> -import common
> +import os, sys
> from optparse import OptionParser
> from autotest_lib.client.bin import job
> from autotest_lib.client.common_lib import global_config
> @@ -41,22 +40,29 @@
> parser.add_option("-l", "--external_logging", dest="log",
> action="store_true",
> default=False, help="enable external logging")
>
> -parser.add_option('--verbose', action='store_true',
> +parser.add_option('--verbose', dest='verbose', action='store_true',
> help='Include DEBUG messages in console output')
>
> +parser.add_option('--quiet', dest='verbose', action='store_false',
> + help='Not include DEBUG messages in console output')
> +
> parser.add_option('--hostname', dest='hostname', type='string',
> default=None, action='store',
> help='Take this as the hostname of this machine '
> '(given by autoserv)')
>
> +parser.add_option('--setup', dest='setup', type='string',
> + default=None, action='store',
> + help='a comma seperated list of client tests to prebuild
> on '
> + 'the server. Use all to prebuild all of them.')
>
> This is a really generic name for something that's actually quite specific.
I think something like --client-test-setup would be better. I know that
something more specific is going to be verbose, but I don't like the idea of
using a really generic name like "setup" for running a very specific kind of
setup.
>
> def usage():
> parser.print_help()
> sys.exit(1)
>
> options, args = parser.parse_args()
>
> -# Check for a control file.
> -if len(args) != 1:
> +# Check for a control file if not in prebuild mode.
> +if len(args) != 1 and options.setup is None:
> usage()
>
> drop_caches = global_config.global_config.get_config_value('CLIENT',
> @@ -64,5 +70,14 @@
> type=bool,
> default=True)
>
> +if options.setup:
> + from autotest_lib.client.bin import setup_job
> + exit_code = 0
> + try:
> + setup_job.setup_tests(options)
> + except:
> + exit_code = 1
> + sys.exit(exit_code)
> +
> # JOB: run the specified job control file.
> job.runjob(os.path.realpath(args[0]), drop_caches, options)
> Index: client/common_lib/test.py
> ===================================================================
> --- client/common_lib/test.py (revision 4228)
> +++ client/common_lib/test.py (working copy)
> @@ -16,7 +16,7 @@
> # src eg. tests/<test>/src
> # tmpdir eg. tmp/<tempname>_<testname.tag>
>
> -import fcntl, os, re, sys, shutil, tarfile, tempfile, time, traceback
> +import fcntl, getpass, os, re, sys, shutil, tarfile, tempfile, time,
> traceback
> import warnings, logging, glob, resource
>
> from autotest_lib.client.common_lib import error
> @@ -39,7 +39,8 @@
> os.mkdir(self.profdir)
> self.debugdir = os.path.join(self.outputdir, 'debug')
> os.mkdir(self.debugdir)
> - self.configure_crash_handler()
> + if getpass.getuser() == 'root':
> + self.configure_crash_handler()
> self.bindir = bindir
> self.srcdir = os.path.join(self.bindir, 'src')
> self.tmpdir = tempfile.mkdtemp("_" + self.tagged_testname,
>
>
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
