Here's what I had in mind. -- John On Mon, Oct 5, 2009 at 11:20 AM, John Admanski <jadmanski@xxxxxxxxxx> wrote: > > After some investigation and irc chat, it has become clear that this misbehaving chunk of code in _autotest_logging_handle_error is in fact an error; the patch that was applied in rev 3649 in subversion added a chunk of the code from setup to the end of _autotest_logging_handle_error, which doesn't work very well since it references a variable local to setup. > The simpler solution would be to just delete that chunk of code. > > -- John > > On Mon, Oct 5, 2009 at 10:40 AM, Lucas Meneghel Rodrigues <lmr@xxxxxxxxxx> wrote: >> >> Currently client/setup_modules.py has an undefined variable error >> that was spot by Amos Kong <akong@xxxxxxxxxx>, and a simple fix >> was proposed. (see http://patchwork.test.kernel.org/patch/1315/) >> >> It seems to me that in a previous refactor of setup_modules, some >> code was moved but not all references were properly fixed. >> >> Amos's fix, while fine, introduces global variables to accomplish >> the goal and didn't fix another undefined reference that was lying >> around. As in general it's advisable to avoid resorting to global >> namespacing, I propose another fix: Encapsulate the library >> namespace setup into a class, and make setup() just a delegate >> of the setup() method inside that class. This way it's possible >> to encapsulate code and use class attributes to avoid using >> global variables. >> >> Since I was there I modified the docstrings of that file to comply >> with the docstring guideline. >> >> John, please let me know what you think about this. >> >> Complexity: Medium (Code refactor, all public API preserved) >> Risk: High (Core code executed by the client). Change was tested though :) >> >> Signed-off-by: Lucas Meneghel Rodrigues <lmr@xxxxxxxxxx> >> --- >> client/setup_modules.py | 306 ++++++++++++++++++++++++++++------------------ >> 1 files changed, 186 insertions(+), 120 deletions(-) >> >> diff --git a/client/setup_modules.py b/client/setup_modules.py >> index dc255c4..188114c 100644 >> --- a/client/setup_modules.py >> +++ b/client/setup_modules.py >> @@ -1,4 +1,9 @@ >> -__author__ = "jadmanski@xxxxxxxxxx (John Admanski)" >> +""" >> +Utility library used for autotest library namespace configuration. >> + >> +@copyright Google 2007-2009 >> +@author John Admanski <jadmanski@xxxxxxxxxx> >> +""" >> >> import os, sys >> >> @@ -12,134 +17,195 @@ check_version.check_python_version() >> >> import new, glob, traceback >> >> +class namespace_config(object): >> + """ >> + Class to perform namespace configuration for autotest. >> >> -def _create_module(name): >> - """Create a single top-level module""" >> - module = new.module(name) >> - sys.modules[name] = module >> - return module >> - >> - >> -def _create_module_and_parents(name): >> - """Create a module, and all the necessary parents""" >> - parts = name.split(".") >> - # first create the top-level module >> - parent = _create_module(parts[0]) >> - created_parts = [parts[0]] >> - parts.pop(0) >> - # now, create any remaining child modules >> - while parts: >> - child_name = parts.pop(0) >> - module = new.module(child_name) >> - setattr(parent, child_name, module) >> - created_parts.append(child_name) >> - sys.modules[".".join(created_parts)] = module >> - parent = module >> - >> - >> -def _import_children_into_module(parent_module_name, path): >> - """Import all the packages on a path into a parent module""" >> - # find all the packages at 'path' >> - names = [] >> - for filename in os.listdir(path): >> - full_name = os.path.join(path, filename) >> - if not os.path.isdir(full_name): >> - continue # skip files >> - if "." in filename: >> - continue # if "." is in the name it's not a valid package name >> - if not os.access(full_name, os.R_OK | os.X_OK): >> - continue # need read + exec access to make a dir importable >> - if "__init__.py" in os.listdir(full_name): >> - names.append(filename) >> - # import all the packages and insert them into 'parent_module' >> - sys.path.insert(0, path) >> - for name in names: >> - module = __import__(name) >> - # add the package to the parent >> - parent_module = sys.modules[parent_module_name] >> - setattr(parent_module, name, module) >> - full_name = parent_module_name + "." + name >> - sys.modules[full_name] = module >> - # restore the system path >> - sys.path.pop(0) >> + This is what makes possible to use statements like >> + from autotest_lib.client.bin import package >> + """ >> + def __init__(self, base_path, root_module_name=""): >> + """ >> + Perform all the necessary setup so that all the packages at >> + 'base_path' can be imported via "import root_module_name.package". >> + If root_module_name is empty, then all the packages at base_path >> + are inserted as top-level packages. >> + >> + Also, setup all the common.* aliases for modules in the common >> + library. >> + >> + The setup must be different if you are running on an Autotest server >> + or on a test machine that just has the client directories installed. >> + >> + @param base_path: Path used to compute all relative paths needed for >> + the setup. >> + @param root_module_name: Top level name of the 'virtual' library >> + namespace. >> + """ >> + # Hack... Any better ideas? >> + if (root_module_name == 'autotest_lib.client' and >> + os.path.exists(os.path.join(os.path.dirname(__file__), >> + '..', 'server'))): >> + root_module_name = 'autotest_lib' >> + base_path = os.path.abspath(os.path.join(base_path, '..')) >> + self.root_module_name = root_module_name >> + self.base_path = base_path >> + >> + >> + def _create_module(self, name): >> + """ >> + Create a single top-level module. >> + >> + @param name: Name of the module you wish to create. >> + """ >> + module = new.module(name) >> + sys.modules[name] = module >> + return module >> + >> + >> + def _create_module_and_parents(self, name): >> + """ >> + Create a module, and all the necessary parents. >> + >> + @param name: Name of the module you wish to create. >> + """ >> + parts = name.split(".") >> + # first create the top-level module >> + parent = self._create_module(parts[0]) >> + created_parts = [parts[0]] >> + parts.pop(0) >> + # now, create any remaining child modules >> + while parts: >> + child_name = parts.pop(0) >> + module = new.module(child_name) >> + setattr(parent, child_name, module) >> + created_parts.append(child_name) >> + sys.modules[".".join(created_parts)] = module >> + parent = module >> + >> + >> + def _import_children_into_module(self, parent_module_name, path): >> + """ >> + Import all the packages on a path into a parent module. >> + >> + @param parent_module_name: Name of the parent module you want to use >> + @param path: Path that contains a number of python packages (libs) >> + """ >> + # find all the packages at 'path' >> + names = [] >> + for filename in os.listdir(path): >> + full_name = os.path.join(path, filename) >> + if not os.path.isdir(full_name): >> + continue # skip files >> + if "." in filename: >> + continue # if "." is in the name it's not a valid package name >> + if not os.access(full_name, os.R_OK | os.X_OK): >> + continue # need read + exec access to make a dir importable >> + if "__init__.py" in os.listdir(full_name): >> + names.append(filename) >> + # import all the packages and insert them into 'parent_module' >> + sys.path.insert(0, path) >> + for name in names: >> + module = __import__(name) >> + # add the package to the parent >> + parent_module = sys.modules[parent_module_name] >> + setattr(parent_module, name, module) >> + full_name = parent_module_name + "." + name >> + sys.modules[full_name] = module >> + # restore the system path >> + sys.path.pop(0) >> + >> + >> + def _autotest_logging_handle_error(self, record): >> + """ >> + Method to monkey patch into logging.Handler to replace handleError. >> + >> + @param record: Logging record that is going to be treated. >> + """ >> + # The same as the default logging.Handler.handleError but also prints >> + # out the original record causing the error so there is -some- idea >> + # about which call caused the logging error. >> + import logging >> + if logging.raiseExceptions: >> + # Avoid recursion as the below output can end up back in here when >> + # something has *seriously* gone wrong in autotest. >> + logging.raiseExceptions = 0 >> + sys.stderr.write('Exception occurred formatting message: ' >> + '%r using args %r\n' % (record.msg, record.args)) >> + traceback.print_stack() >> + sys.stderr.write('Future logging formatting exceptions disabled.\n') >> + >> + if self.root_module_name == 'autotest_lib': >> + # Allow locally installed third party packages to be found >> + # before any that are installed on the system itself when not. >> + # running as a client. >> + # This is primarily for the benefit of frontend and tko so that they >> + # may use libraries other than those available as system packages. >> + sys.path.insert(0, os.path.join(self.base_path, "site-packages")) >> + >> + >> + def _monkeypatch_logging_handle_error(self): >> + """ >> + Hack out logging.py to introduce a custom logging error handling >> + function. >> + """ >> + # Hack out logging.py* >> + logging_py = os.path.join(os.path.dirname(__file__), "common_lib", >> + "logging.py*") >> + if glob.glob(logging_py): >> + os.system("rm -f %s" % logging_py) >> + >> + # Monkey patch our own handleError into the logging module's >> + # StreamHandler. A nicer way of doing this -might- be to have our own >> + # logging module define an autotest Logger instance that added our own >> + # Handler subclass with this handleError method in it. But that would >> + # mean modifying tons of code. >> + import logging >> + assert callable(logging.Handler.handleError) >> + logging.Handler.handleError = self._autotest_logging_handle_error >> + >> + >> + def setup(self): >> + """ >> + Effectively creates the autotest_lib namespace. >> + """ >> + # Some internal functions might need to reference those variables. >> + # It doesn't occur to me how to refactor the API in this file in a clean >> + # way to avoid >> + self._create_module_and_parents(self.root_module_name) >> + self._import_children_into_module(self.root_module_name, self.base_path) >> + >> + if self.root_module_name == 'autotest_lib': >> + # Allow locally installed third party packages to be found >> + # before any that are installed on the system itself when not. >> + # running as a client. >> + # This is primarily for the benefit of frontend and tko so that they >> + # may use libraries other than those available as system packages. >> + sys.path.insert(0, os.path.join(self.base_path, "site-packages")) >> + >> + self._monkeypatch_logging_handle_error() >> >> >> def import_module(module, from_where): >> - """Equivalent to 'from from_where import module' >> - Returns the corresponding module""" >> + """ >> + Equivalent to 'from from_where import module' >> + >> + @param module: Module intended to be loaded >> + @param from_where: Top level module where we want to import the module >> + @return: loaded module >> + """ >> from_module = __import__(from_where, globals(), locals(), [module]) >> return getattr(from_module, module) >> >> >> -def _autotest_logging_handle_error(self, record): >> - """Method to monkey patch into logging.Handler to replace handleError.""" >> - # The same as the default logging.Handler.handleError but also prints >> - # out the original record causing the error so there is -some- idea >> - # about which call caused the logging error. >> - import logging >> - if logging.raiseExceptions: >> - # Avoid recursion as the below output can end up back in here when >> - # something has *seriously* gone wrong in autotest. >> - logging.raiseExceptions = 0 >> - sys.stderr.write('Exception occurred formatting message: ' >> - '%r using args %r\n' % (record.msg, record.args)) >> - traceback.print_stack() >> - sys.stderr.write('Future logging formatting exceptions disabled.\n') >> - >> - if root_module_name == 'autotest_lib': >> - # Allow locally installed third party packages to be found >> - # before any that are installed on the system itself when not. >> - # running as a client. >> - # This is primarily for the benefit of frontend and tko so that they >> - # may use libraries other than those available as system packages. >> - sys.path.insert(0, os.path.join(base_path, "site-packages")) >> - >> - >> -def _monkeypatch_logging_handle_error(): >> - # Hack out logging.py* >> - logging_py = os.path.join(os.path.dirname(__file__), "common_lib", >> - "logging.py*") >> - if glob.glob(logging_py): >> - os.system("rm -f %s" % logging_py) >> - >> - # Monkey patch our own handleError into the logging module's StreamHandler. >> - # A nicer way of doing this -might- be to have our own logging module define >> - # an autotest Logger instance that added our own Handler subclass with this >> - # handleError method in it. But that would mean modifying tons of code. >> - import logging >> - assert callable(logging.Handler.handleError) >> - logging.Handler.handleError = _autotest_logging_handle_error >> - >> - >> def setup(base_path, root_module_name=""): >> """ >> - Perform all the necessary setup so that all the packages at >> - 'base_path' can be imported via "import root_module_name.package". >> - If root_module_name is empty, then all the packages at base_path >> - are inserted as top-level packages. >> - >> - Also, setup all the common.* aliases for modules in the common >> - library. >> + Configures the autotest_lib namespace for use on a given autotest >> + entry point >> >> - The setup must be different if you are running on an Autotest server >> - or on a test machine that just has the client directories installed. >> + @param base_path: Path used to compute all relative paths needed for the >> + setup. >> + @param root_module_name: Top level name of the 'virtual' library namespace. >> """ >> - # Hack... Any better ideas? >> - if (root_module_name == 'autotest_lib.client' and >> - os.path.exists(os.path.join(os.path.dirname(__file__), >> - '..', 'server'))): >> - root_module_name = 'autotest_lib' >> - base_path = os.path.abspath(os.path.join(base_path, '..')) >> - >> - _create_module_and_parents(root_module_name) >> - _import_children_into_module(root_module_name, base_path) >> - >> - if root_module_name == 'autotest_lib': >> - # Allow locally installed third party packages to be found >> - # before any that are installed on the system itself when not. >> - # running as a client. >> - # This is primarily for the benefit of frontend and tko so that they >> - # may use libraries other than those available as system packages. >> - sys.path.insert(0, os.path.join(base_path, "site-packages")) >> - >> - _monkeypatch_logging_handle_error() >> + configurator = namespace_config(base_path, root_module_name) >> + configurator.setup() >> -- >> 1.6.2.5 >> >
Attachment:
patch
Description: Binary data
