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). + 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? > 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
