On Tue, Feb 9, 2010 at 2:53 PM, Eric Li(李咏竹) <[email protected]> wrote:
> Hi John, > > Thanks for your comments. I had addressed all your comments and please take > another look. > > The actual code patch will be send later after we agreed on all the > comments. > > Thanks. > > 2010/2/9 John Admanski <[email protected]> > > 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. >> > > I think I did that in purpose. Yes, I am bypassing client_job init. > Otherwise, it is over complicated to run a client job on server. Since the > directory layout and assumptions are slightly different. > > The client job need a control file in the constructor, and setup_job does > not need that. > The client job will have state, and setup job does not have that (yet). > > Okay, I just have some concern that if you're using the client_job class by bypassing the __init__ it may make the setup_job code more susceptible to breakage when client_job code gets changed. It would be great if you could provide some sort of unit tests for setup_job, at least ones that test the core assumptions this code is making, so that if someone makes changes to client_job which violate those assumptions then it's much more obvious. > > >>> + 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. >> > I think I am following the CODING_STYLE, maybe I am wrong. Do you have more > specific comment? > > I was mainly referring to the parameter documentation (the @param stuff). > 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. >> > > I will fix this. > >> >>> + 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()? >> > > yes, I will fix this. > >> + 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, >>> >>> >> > > > -- > Eric Li > 李咏竹 > Google Kirkland > > >
_______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
