target: Cleanup and lazily initialize modules

Cleanup the module loading code and enable lazy initialization of modules:

* Target.modules is now a read-only property, that is a list of strings
  (always, not either strings or dict with mod name as key and params dict as
  value).
  Target._modules dict stores parameters for each module that was asked
  for.

* Target.__init__() now makes thorough validation of the modules list it
  is given:
    * Specifying the same module mulitple time is only allowed if they
      are specified with the same parameters. If a module is specified
      both with and without parameters, the parameters take precedence
      and the conflict is resolved.
    * Only one module of each "kind" can be present in the list.

* Module subclasses gained a class attribute "attr_name" that computes
  their "attribute name", i.e. the name under which they are expected to
  be lookedup on a Target instance.

* Modules are now automatically registered by simple virtue of
  inheriting from Module and defining a name, wherever the source
  resides. They do not have to be located in devlib.modules anymore.
  This allows 3rd party module providers to very easily add new ones.

* Modules are accessible as Target attribute as:
    * Their "kind" if they specified one
    * Their "name" (always)

    This allows the consumer to either rely on a generic API (via the
    "kind") or to expect a specific module (via the "name").

* Accessing a module on Target will lazily load it even if was not
  selected using Target(modules=...):
    * If the module parameters were specified in Target(modules=...) or
      via platform modules, they will be applied automatically.
    * Otherwise, no parameter is passed.
    * If no module can be found with that name, the list of
      Target.modules will be searched for a module matching the given
      kind. The first one to be found will be used.

* Modules specified in Target(modules=...) are still loaded eagerly when
  their stage is reached just like it used to. We could easily make
  those lazily loaded though if we wanted.

* Specifying Target(modules={'foo': None}) will make the "foo" module
  unloadable. This can be used to prevent lazy loading a specific
  module.

devlib/module/__init__.py

@@ -15,16 +15,30 @@
 import logging
 from inspect import isclass
 
-from past.builtins import basestring
+from devlib.exception import TargetStableError
-
-from devlib.utils.misc import walk_modules
 from devlib.utils.types import identifier
+from devlib.utils.misc import walk_modules
+
+_module_registry = {}
+
+def register_module(mod):
+    if not issubclass(mod, Module):
+        raise ValueError('A module must subclass devlib.Module')
+
+    if mod.name is None:
+        raise ValueError('A module must define a name')
+
+    try:
+        existing = _module_registry[mod.name]
+    except KeyError:
+        pass
+    else:
+        if existing is not mod:
+            raise ValueError(f'Module "{mod.name}" already exists')
+    _module_registry[mod.name] = mod
 
 
-__module_cache = {}
+class Module:
-
-
-class Module(object):
 
     name = None
     kind = None
@@ -48,22 +62,48 @@ class Module(object):
 
     @classmethod
     def install(cls, target, **params):
-        if cls.kind is not None:
+        attr_name = cls.attr_name
-            attr_name = identifier(cls.kind)
+        installed = target._installed_modules
+
+        try:
+            mod = installed[attr_name]
+        except KeyError:
+            mod = cls(target, **params)
+            mod.logger.debug(f'Installing module {cls.name}')
+
+            if mod.probe(target):
+                for name in (
+                    attr_name,
+                    identifier(cls.name),
+                    identifier(cls.kind) if cls.kind else None,
+                ):
+                    if name is not None:
+                        installed[name] = mod
+
+                target._modules[cls.name] = params
+                return mod
             else:
-            attr_name = identifier(cls.name)
+                raise TargetStableError(f'Module "{cls.name}" is not supported by the target')
-        if hasattr(target, attr_name):
+        else:
-            existing_module = getattr(target, attr_name)
+            raise ValueError(
-            existing_name = getattr(existing_module, 'name', str(existing_module))
+                f'Attempting to install module "{cls.name}" but a module is already installed as attribute "{attr_name}": {mod}'
-            message = 'Attempting to install module "{}" which already exists (new: {}, existing: {})'
+            )
-            raise ValueError(message.format(attr_name, cls.name, existing_name))
-        setattr(target, attr_name, cls(target, **params))
 
     def __init__(self, target):
         self.target = target
         self.logger = logging.getLogger(self.name)
 
 
+    def __init_subclass__(cls, *args, **kwargs):
+        super().__init_subclass__(*args, **kwargs)
+
+        attr_name = cls.kind or cls.name
+        cls.attr_name = identifier(attr_name) if attr_name else None
+
+        if cls.name is not None:
+            register_module(cls)
+
+
 class HardRestModule(Module):
 
     kind = 'hard_reset'
@@ -96,32 +136,25 @@ class FlashModule(Module):
 
 
 def get_module(mod):
-    if not __module_cache:
+    def from_registry(mod):
-        __load_cache()
-
-    if isinstance(mod, basestring):
         try:
-            return __module_cache[mod]
+            return _module_registry[mod]
         except KeyError:
             raise ValueError('Module "{}" does not exist'.format(mod))
+
+    if isinstance(mod, str):
+        try:
+            return from_registry(mod)
+        except ValueError:
+            # If the lookup failed, we may have simply not imported Modules
+            # from the devlib.module package. The former module loading
+            # implementation was also pre-importing modules, so we need to
+            # replicate that behavior since users are currently not expected to
+            # have imported the module prior to trying to use it.
+            walk_modules('devlib.module')
+            return from_registry(mod)
+
     elif issubclass(mod, Module):
         return mod
     else:
         raise ValueError('Not a valid module: {}'.format(mod))
-
-
-def register_module(mod):
-    if not issubclass(mod, Module):
-        raise ValueError('A module must subclass devlib.Module')
-    if mod.name is None:
-        raise ValueError('A module must define a name')
-    if mod.name in __module_cache:
-        raise ValueError('Module {} already exists'.format(mod.name))
-    __module_cache[mod.name] = mod
-
-
-def __load_cache():
-    for module in walk_modules('devlib.module'):
-        for obj in vars(module).values():
-            if isclass(obj) and issubclass(obj, Module) and obj.name:
-                register_module(obj)

devlib/target.py

@@ -21,6 +21,7 @@ import functools
 import gzip
 import glob
 import os
+from operator import itemgetter
 import re
 import time
 import logging
@@ -48,7 +49,7 @@ from enum import Enum
 from concurrent.futures import ThreadPoolExecutor
 
 from devlib.host import LocalConnection, PACKAGE_BIN_DIRECTORY
-from devlib.module import get_module
+from devlib.module import get_module, Module
 from devlib.platform import Platform
 from devlib.exception import (DevlibTransientError, TargetStableError,
                               TargetNotRespondingError, TimeoutError,
@@ -338,7 +339,6 @@ class Target(object):
         self.connection_settings['platform'] = self.platform
         self.working_directory = working_directory
         self.executables_directory = executables_directory
-        self.modules = modules or []
         self.load_default_modules = load_default_modules
         self.shell_prompt = bytes_regex(shell_prompt)
         self.conn_cls = conn_cls
@@ -352,12 +352,73 @@ class Target(object):
         self._max_async = max_async
         self.busybox = None
 
-        if load_default_modules:
+        def normalize_mod_spec(spec):
-            module_lists = [self.default_modules]
+            if isinstance(spec, str):
+                return (spec, {})
             else:
-            module_lists = []
+                [(name, params)] = spec.items()
-        module_lists += [self.modules, self.platform.modules]
+                return (name, params)
-        self.modules = merge_lists(*module_lists, duplicates='first')
+
+        modules = sorted(
+            map(
+                normalize_mod_spec,
+                itertools.chain(
+                    self.default_modules if load_default_modules else [],
+                    modules or [],
+                    self.platform.modules or [],
+                )
+            ),
+            key=itemgetter(0),
+        )
+
+        # Ensure that we did not ask for the same module but different
+        # configurations. Empty configurations are ignored, so any
+        # user-provided conf will win against an empty conf.
+        def elect(name, specs):
+            specs = list(specs)
+
+            confs = set(
+                tuple(sorted(params.items()))
+                for _, params in specs
+                if params
+            )
+            if len(confs) > 1:
+                raise ValueError(f'Attempted to load the module "{name}" with multiple different configuration')
+            else:
+                if any(
+                    params is None
+                    for _, params in specs
+                ):
+                    params = None
+                else:
+                    params = dict(confs.pop()) if confs else {}
+
+                return (name, params)
+
+        modules = dict(itertools.starmap(
+            elect,
+            itertools.groupby(modules, key=itemgetter(0))
+        ))
+
+        def get_kind(name):
+            return get_module(name).kind or ''
+
+        def kind_conflict(kind, names):
+            if kind:
+                raise ValueError(f'Cannot enable multiple modules sharing the same kind "{kind}": {sorted(names)}')
+
+        list(itertools.starmap(
+            kind_conflict,
+            itertools.groupby(
+                sorted(
+                    modules.keys(),
+                    key=get_kind
+                ),
+                key=get_kind
+            )
+        ))
+        self._modules = modules
+
         self._update_modules('early')
         if connect:
             self.connect(max_async=max_async)
@@ -409,8 +470,6 @@ class Target(object):
         self._detect_max_async(max_async or self._max_async)
         self.platform.update_from_target(self)
         self._update_modules('connected')
-        if self.platform.big_core and self.load_default_modules:
-            self._install_module(get_module('bl'))
 
     def _detect_max_async(self, max_async):
         self.logger.debug('Detecting max number of async commands ...')
@@ -1290,7 +1349,13 @@ fi
         return self._installed_binaries.get(name, name)
 
     def has(self, modname):
-        return hasattr(self, identifier(modname))
+        modname = identifier(modname)
+        try:
+            self._get_module(modname, log=False)
+        except Exception:
+            return False
+        else:
+            return True
 
     @asyn.asyncf
     async def lsmod(self):
@@ -1442,14 +1507,11 @@ fi
     def install_module(self, mod, **params):
         mod = get_module(mod)
         if mod.stage == 'early':
-            msg = 'Module {} cannot be installed after device setup has already occoured.'
+            raise TargetStableError(
-            raise TargetStableError(msg)
+                f'Module "{mod.name}" cannot be installed after device setup has already occoured'
-
+            )
-        if mod.probe(self):
-            self._install_module(mod, **params)
         else:
-            msg = 'Module {} is not supported by the target'.format(mod.name)
+            return self._install_module(mod, params)
-            raise TargetStableError(msg)
 
     # internal methods
 
@@ -1500,39 +1562,82 @@ fi
                 extracted = dest
         return extracted
 
-    def _update_modules(self, stage):
+    def _install_module(self, mod, params, log=True):
-        for mod_name in copy.copy(self.modules):
+        mod = get_module(mod)
-            if isinstance(mod_name, dict):
-                mod_name, params = list(mod_name.items())[0]
-            else:
-                params = {}
-            mod = get_module(mod_name)
-            if not mod.stage == stage:
-                continue
-            if mod.probe(self):
-                self._install_module(mod, **params)
-            else:
-                msg = 'Module {} is not supported by the target'.format(mod.name)
-                self.modules.remove(mod_name)
-                if self.load_default_modules:
-                    self.logger.debug(msg)
-                else:
-                    self.logger.warning(msg)
-
-    def _install_module(self, mod, **params):
         name = mod.name
-        if name not in self._installed_modules:
+        if params is None or self._modules.get(name, {}) is None:
-            self.logger.debug('Installing module {}'.format(name))
+            raise TargetStableError(f'Could not load module "{name}" as it has been explicilty disabled')
-            try:
-                mod.install(self, **params)
-            except Exception as e:
-                self.logger.error('Module "{}" failed to install on target: {}'.format(name, e))
-                raise
-            self._installed_modules[name] = mod
-            if name not in self.modules:
-                self.modules.append(name)
         else:
-            self.logger.debug('Module {} is already installed.'.format(name))
+            try:
+                return mod.install(self, **params)
+            except Exception as e:
+                if log:
+                    self.logger.error(f'Module "{name}" failed to install on target: {e}')
+                raise
+
+    @property
+    def modules(self):
+        return sorted(self._modules.keys())
+
+    def _update_modules(self, stage):
+        to_install = [
+            (mod, params)
+            for mod, params in (
+                (get_module(name), params)
+                for name, params in self._modules.items()
+            )
+            if mod.stage == stage
+        ]
+        for mod, params in to_install:
+            try:
+                self._install_module(mod, params)
+            except Exception as e:
+                mod_name = mod.name
+                self.logger.warning(f'Module {mod.name} is not supported by the target: {e}')
+
+    def _get_module(self, modname, log=True):
+        try:
+            return self._installed_modules[modname]
+        except KeyError:
+            params = {}
+            try:
+                mod = get_module(modname)
+            # We might try to access e.g. "boot" attribute, which is ambiguous
+            # since there are multiple modules with the "boot" kind. In that
+            # case, we look into the list of modules enabled by the user and
+            # get the first "boot" module we find.
+            except ValueError:
+                for _mod, _params in self._modules.items():
+                    try:
+                        _mod = get_module(_mod)
+                    except ValueError:
+                        pass
+                    else:
+                        if _mod.attr_name == modname:
+                            mod = _mod
+                            params = _params
+                            break
+                else:
+                    raise AttributeError(
+                        f"'{self.__class__.__name__}' object has no attribute '{modname}'"
+                    )
+            else:
+                params = self._modules.get(mod.name, {})
+
+            self._install_module(mod, params, log=log)
+            return self.__getattr__(modname)
+
+    def __getattr__(self, attr):
+        # When unpickled, objects will have an empty dict so fail early
+        if attr.startswith('__') and attr.endswith('__'):
+            raise AttributeError(attr)
+
+        try:
+            return self._get_module(attr)
+        except Exception as e:
+            # Raising AttributeError is important otherwise hasattr() will not
+            # work as expected
+            raise AttributeError(str(e))
 
     def _resolve_paths(self):
         raise NotImplementedError()