Merge remote-tracking branch 'origin/issue590'

* origin/issue590:
  issue #590: disable distro test on vanilla
  ci: Ansible 2.8 jobs aren't running against all host types.
  tests: Py3.x fix.
  issue #590: fix test for <2.8 Ansibles.
  tests: Py3.x fix.
  issue #590: actually run Ansible test.
  tests: Py3.x fix.
  master: sysconfig did not exist until 2.7.
  tests: rearrange test modules again, they're used in multiple places
  module_finder_test: mask one more difference between unit2 vs. direct start
  master: fix _is_stdlib_path() failure on Ubuntu.
  issue #590: add dummy package for new test.
  issue #590: add FinderMethod docstrings.
  issue #590: update comment to indicate the hack is permanent
  issue #590: move example modules to module_finder/, fix/add tests
  issue #590: refactor ModuleFinder and teach it a new special case.
  issue #590: Ansible test for module_utils.distro use.
  issue #590: teach importer to handle self-replacing modules

.ci/azure-pipelines.yml

@@ -92,10 +92,8 @@ jobs:
         python.version: '2.7'
         MODE: ansible
         VER: 2.8.0
-        DISTROS: debian
 
       Ansible_280_35:
         python.version: '3.5'
         MODE: ansible
         VER: 2.8.0
-        DISTROS: debian

ansible_mitogen/runner.py

@@ -763,7 +763,16 @@ class NewStyleRunner(ScriptRunner):
             try:
                 mitogen.core.import_module(fullname)
             except ImportError:
-                # TODO: this is a huge hack to work around issue #590.
+                # #590: Ansible 2.8 module_utils.distro is a package that
+                # replaces itself in sys.modules with a non-package during
+                # import. Prior to replacement, it is a real package containing
+                # a '_distro' submodule which is used on 2.x. Given a 2.x
+                # controller and 3.x target, the import hook never needs to run
+                # again before this replacement occurs, and 'distro' is
+                # replaced with a module from the stdlib. In this case as this
+                # loop progresses to the next entry and attempts to preload
+                # 'distro._distro', the import mechanism will fail. So here we
+                # silently ignore any failure for it.
                 if fullname != 'ansible.module_utils.distro._distro':
                     raise
 

docs/changelog.rst

@@ -30,6 +30,12 @@ Enhancements
   <https://docs.ansible.com/ansible/latest/plugins/become.html>`_
   functionality, which will be addressed in a future release.
 
+Fixes
+^^^^^
+
+* `#590 <https://github.com/dw/mitogen/issues/590>`_: the importer can handle
+  modules that replace themselves in :mod:`sys.modules` during import.
+
 
 Thanks!
 ~~~~~~~

mitogen/core.py

@@ -1355,7 +1355,10 @@ class Importer(object):
             exec(code, vars(mod))
         else:
             exec('exec code in vars(mod)')
-        return mod
+
+        # #590: if a module replaces itself in sys.modules during import, below
+        # is necessary. This matches PyImport_ExecCodeModuleEx()
+        return sys.modules.get(fullname, mod)
 
     def get_filename(self, fullname):
         if fullname in self._cache:

mitogen/master.py

@@ -36,6 +36,7 @@ contexts.
 """
 
 import dis
+import errno
 import imp
 import inspect
 import itertools
@@ -45,11 +46,16 @@ import pkgutil
 import re
 import string
 import sys
-import time
 import threading
+import time
 import types
 import zlib
 
+try:
+    import sysconfig
+except ImportError:
+    sysconfig = None
+
 if not hasattr(pkgutil, 'find_loader'):
     # find_loader() was new in >=2.5, but the modern pkgutil.py syntax has
     # been kept intentionally 2.3 compatible so we can reuse it.
@@ -92,10 +98,16 @@ def _stdlib_paths():
         'real_prefix',  # virtualenv: only set inside a virtual environment.
         'base_prefix',  # venv: always set, equal to prefix if outside.
     ]
-    prefixes = (getattr(sys, a) for a in attr_candidates if hasattr(sys, a))
+    prefixes = (getattr(sys, a, None) for a in attr_candidates)
     version = 'python%s.%s' % sys.version_info[0:2]
-    return set(os.path.abspath(os.path.join(p, 'lib', version))
-               for p in prefixes)
+    s = set(os.path.abspath(os.path.join(p, 'lib', version))
+            for p in prefixes if p is not None)
+
+    # When running 'unit2 tests/module_finder_test.py' in a Py2 venv on Ubuntu
+    # 18.10, above is insufficient to catch the real directory.
+    if sysconfig is not None:
+        s.add(sysconfig.get_config_var('DESTLIB'))
+    return s
 
 
 def is_stdlib_name(modname):
@@ -142,6 +154,41 @@ def get_child_modules(path):
     return [to_text(name) for _, name, _ in it]
 
 
+def _looks_like_script(path):
+    """
+    Return :data:`True` if the (possibly extensionless) file at `path`
+    resembles a Python script. For now we simply verify the file contains
+    ASCII text.
+    """
+    try:
+        fp = open(path, 'rb')
+    except IOError:
+        e = sys.exc_info()[1]
+        if e.args[0] == errno.EISDIR:
+            return False
+        raise
+
+    try:
+        sample = fp.read(512).decode('latin-1')
+        return not set(sample).difference(string.printable)
+    finally:
+        fp.close()
+
+
+def _py_filename(path):
+    if not path:
+        return None
+
+    if path[-4:] in ('.pyc', '.pyo'):
+        path = path.rstrip('co')
+
+    if path.endswith('.py'):
+        return path
+
+    if os.path.exists(path) and _looks_like_script(path):
+        return path
+
+
 def _get_core_source():
     """
     Master version of parent.get_core_source().
@@ -368,56 +415,38 @@ class LogForwarder(object):
         return 'LogForwarder(%r)' % (self._router,)
 
 
-class ModuleFinder(object):
+class FinderMethod(object):
     """
-    Given the name of a loaded module, make a best-effort attempt at finding
-    related modules likely needed by a child context requesting the original
-    module.
+    Interface to a method for locating a Python module or package given its
+    name according to the running Python interpreter. You'd think this was a
+    simple task, right? Naive young fellow, welcome to the real world.
     """
-    def __init__(self):
-        #: Import machinery is expensive, keep :py:meth`:get_module_source`
-        #: results around.
-        self._found_cache = {}
-
-        #: Avoid repeated dependency scanning, which is expensive.
-        self._related_cache = {}
-
     def __repr__(self):
-        return 'ModuleFinder()'
+        return '%s()' % (type(self).__name__,)
 
-    def _looks_like_script(self, path):
+    def find(self, fullname):
         """
-        Return :data:`True` if the (possibly extensionless) file at `path`
-        resembles a Python script. For now we simply verify the file contains
-        ASCII text.
+        Accept a canonical module name and return `(path, source, is_pkg)`
+        tuples, where:
+
+        * `path`: Unicode string containing path to source file.
+        * `source`: Bytestring containing source file's content.
+        * `is_pkg`: :data:`True` if `fullname` is a package.
+
+        :returns:
+            :data:`None` if not found, or tuple as described above.
         """
-        fp = open(path, 'rb')
-        try:
-            sample = fp.read(512).decode('latin-1')
-            return not set(sample).difference(string.printable)
-        finally:
-            fp.close()
+        raise NotImplementedError()
 
-    def _py_filename(self, path):
-        if not path:
-            return None
 
-        if path[-4:] in ('.pyc', '.pyo'):
-            path = path.rstrip('co')
-
-        if path.endswith('.py'):
-            return path
-
-        if os.path.exists(path) and self._looks_like_script(path):
-            return path
-
-    def _get_main_module_defective_python_3x(self, fullname):
-        """
-        Recent versions of Python 3.x introduced an incomplete notion of
-        importer specs, and in doing so created permanent asymmetry in the
-        :mod:`pkgutil` interface handling for the `__main__` module. Therefore
-        we must handle `__main__` specially.
-        """
+class DefectivePython3xMainMethod(FinderMethod):
+    """
+    Recent versions of Python 3.x introduced an incomplete notion of
+    importer specs, and in doing so created permanent asymmetry in the
+    :mod:`pkgutil` interface handling for the `__main__` module. Therefore
+    we must handle `__main__` specially.
+    """
+    def find(self, fullname):
         if fullname != '__main__':
             return None
 
@@ -426,7 +455,7 @@ class ModuleFinder(object):
             return None
 
         path = getattr(mod, '__file__', None)
-        if not (os.path.exists(path) and self._looks_like_script(path)):
+        if not (os.path.exists(path) and _looks_like_script(path)):
             return None
 
         fp = open(path, 'rb')
@@ -437,11 +466,13 @@ class ModuleFinder(object):
 
         return path, source, False
 
-    def _get_module_via_pkgutil(self, fullname):
-        """
-        Attempt to fetch source code via pkgutil. In an ideal world, this would
-        be the only required implementation of get_module().
-        """
+
+class PkgutilMethod(FinderMethod):
+    """
+    Attempt to fetch source code via pkgutil. In an ideal world, this would
+    be the only required implementation of get_module().
+    """
+    def find(self, fullname):
         try:
             # Pre-'import spec' this returned None, in Python3.6 it raises
             # ImportError.
@@ -458,7 +489,7 @@ class ModuleFinder(object):
             return
 
         try:
-            path = self._py_filename(loader.get_filename(fullname))
+            path = _py_filename(loader.get_filename(fullname))
             source = loader.get_source(fullname)
             is_pkg = loader.is_package(fullname)
         except (AttributeError, ImportError):
@@ -484,19 +515,27 @@ class ModuleFinder(object):
 
         return path, source, is_pkg
 
-    def _get_module_via_sys_modules(self, fullname):
-        """
-        Attempt to fetch source code via sys.modules. This is specifically to
-        support __main__, but it may catch a few more cases.
-        """
+
+class SysModulesMethod(FinderMethod):
+    """
+    Attempt to fetch source code via sys.modules. This is specifically to
+    support __main__, but it may catch a few more cases.
+    """
+    def find(self, fullname):
         module = sys.modules.get(fullname)
         LOG.debug('_get_module_via_sys_modules(%r) -> %r', fullname, module)
+        if getattr(module, '__name__', None) != fullname:
+            LOG.debug('sys.modules[%r].__name__ does not match %r, assuming '
+                      'this is a hacky module alias and ignoring it',
+                      fullname, fullname)
+            return
+
         if not isinstance(module, types.ModuleType):
             LOG.debug('sys.modules[%r] absent or not a regular module',
                       fullname)
             return
 
-        path = self._py_filename(getattr(module, '__file__', ''))
+        path = _py_filename(getattr(module, '__file__', ''))
         if not path:
             return
 
@@ -517,12 +556,19 @@ class ModuleFinder(object):
 
         return path, source, is_pkg
 
-    def _get_module_via_parent_enumeration(self, fullname):
-        """
-        Attempt to fetch source code by examining the module's (hopefully less
-        insane) parent package. Required for older versions of
-        ansible.compat.six and plumbum.colors.
-        """
+
+class ParentEnumerationMethod(FinderMethod):
+    """
+    Attempt to fetch source code by examining the module's (hopefully less
+    insane) parent package. Required for older versions of
+    ansible.compat.six and plumbum.colors, and Ansible 2.8
+    ansible.module_utils.distro.
+
+    For cases like module_utils.distro, this must handle cases where a package
+    transmuted itself into a totally unrelated module during import and vice
+    versa.
+    """
+    def find(self, fullname):
         if fullname not in sys.modules:
             # Don't attempt this unless a module really exists in sys.modules,
             # else we could return junk.
@@ -531,30 +577,68 @@ class ModuleFinder(object):
         pkgname, _, modname = str_rpartition(to_text(fullname), u'.')
         pkg = sys.modules.get(pkgname)
         if pkg is None or not hasattr(pkg, '__file__'):
+            LOG.debug('%r: %r is not a package or lacks __file__ attribute',
+                      self, pkgname)
             return
 
-        pkg_path = os.path.dirname(pkg.__file__)
+        pkg_path = [os.path.dirname(pkg.__file__)]
         try:
-            fp, path, ext = imp.find_module(modname, [pkg_path])
-            try:
-                path = self._py_filename(path)
-                if not path:
-                    fp.close()
-                    return
-
-                source = fp.read()
-            finally:
-                if fp:
-                    fp.close()
-
-            if isinstance(source, mitogen.core.UnicodeType):
-                # get_source() returns "string" according to PEP-302, which was
-                # reinterpreted for Python 3 to mean a Unicode string.
-                source = source.encode('utf-8')
-            return path, source, False
+            fp, path, (suffix, _, kind) = imp.find_module(modname, pkg_path)
         except ImportError:
             e = sys.exc_info()[1]
-            LOG.debug('imp.find_module(%r, %r) -> %s', modname, [pkg_path], e)
+            LOG.debug('%r: imp.find_module(%r, %r) -> %s',
+                      self, modname, [pkg_path], e)
+            return None
+
+        if kind == imp.PKG_DIRECTORY:
+            return self._found_package(fullname, path)
+        else:
+            return self._found_module(fullname, path, fp)
+
+    def _found_package(self, fullname, path):
+        path = os.path.join(path, '__init__.py')
+        LOG.debug('%r: %r is PKG_DIRECTORY: %r', self, fullname, path)
+        return self._found_module(
+            fullname=fullname,
+            path=path,
+            fp=open(path, 'rb'),
+            is_pkg=True,
+        )
+
+    def _found_module(self, fullname, path, fp, is_pkg=False):
+        try:
+            path = _py_filename(path)
+            if not path:
+                return
+
+            source = fp.read()
+        finally:
+            if fp:
+                fp.close()
+
+        if isinstance(source, mitogen.core.UnicodeType):
+            # get_source() returns "string" according to PEP-302, which was
+            # reinterpreted for Python 3 to mean a Unicode string.
+            source = source.encode('utf-8')
+        return path, source, is_pkg
+
+
+class ModuleFinder(object):
+    """
+    Given the name of a loaded module, make a best-effort attempt at finding
+    related modules likely needed by a child context requesting the original
+    module.
+    """
+    def __init__(self):
+        #: Import machinery is expensive, keep :py:meth`:get_module_source`
+        #: results around.
+        self._found_cache = {}
+
+        #: Avoid repeated dependency scanning, which is expensive.
+        self._related_cache = {}
+
+    def __repr__(self):
+        return 'ModuleFinder()'
 
     def add_source_override(self, fullname, path, source, is_pkg):
         """
@@ -576,10 +660,10 @@ class ModuleFinder(object):
         self._found_cache[fullname] = (path, source, is_pkg)
 
     get_module_methods = [
-        _get_main_module_defective_python_3x,
-        _get_module_via_pkgutil,
-        _get_module_via_sys_modules,
-        _get_module_via_parent_enumeration,
+        DefectivePython3xMainMethod(),
+        PkgutilMethod(),
+        SysModulesMethod(),
+        ParentEnumerationMethod(),
     ]
 
     def get_module_source(self, fullname):
@@ -595,7 +679,7 @@ class ModuleFinder(object):
             return tup
 
         for method in self.get_module_methods:
-            tup = method(self, fullname)
+            tup = method.find(fullname)
             if tup:
                 #LOG.debug('%r returned %r', method, tup)
                 break

tests/ansible/lib/modules/custom_python_uses_distro.py

@@ -0,0 +1,21 @@
+#!/usr/bin/python
+# issue #590: I am an Ansible new-style Python module that tries to use
+# ansible.module_utils.distro.
+
+import ansible
+from ansible.module_utils.basic import AnsibleModule
+
+if ansible.__version__ > '2.8':
+    from ansible.module_utils import distro
+else:
+    distro = None
+
+def main():
+    module = AnsibleModule(argument_spec={})
+    if ansible.__version__ > '2.8':
+        module.exit_json(info=distro.info())
+    else:
+        module.exit_json(info={'id': None})
+
+if __name__ == '__main__':
+    main()

tests/ansible/regression/all.yml

@@ -8,3 +8,4 @@
 - include: issue_154__module_state_leaks.yml
 - include: issue_177__copy_module_failing.yml
 - include: issue_332_ansiblemoduleerror_first_occurrence.yml
+- include: issue_590__sys_modules_crap.yml

tests/ansible/regression/issue_590__sys_modules_crap.yml

@@ -0,0 +1,12 @@
+
+- hosts: test-targets
+  tasks:
+  - meta: end_play
+    when: ansible_version.full < '2.8'
+
+  - custom_python_uses_distro:
+    register: out
+
+  - assert:
+      that:
+      - "'id' in out.info"

tests/data/importer/pkg_like_ansible/module_utils/distro/__init__.py

@@ -0,0 +1,5 @@
+# #590: a package that turns itself into a module.
+I_AM = "the package that was replaced"
+import sys
+from pkg_like_ansible.module_utils.distro import _distro
+sys.modules[__name__] = _distro

tests/data/importer/pkg_like_ansible/module_utils/distro/_distro.py

@@ -0,0 +1 @@
+I_AM = "the module that replaced the package"

tests/data/importer/simple_pkg/imports_replaces_self.py

@@ -0,0 +1,6 @@
+# issue #590: this module imports a module that replaces itself in sys.modules
+# during initialization.
+import simple_pkg.replaces_self
+
+def subtract_one(n):
+    return simple_pkg.replaces_self.subtract_one(n)

tests/data/importer/simple_pkg/replaces_self.py

@@ -0,0 +1,4 @@
+# issue #590: this module replaces itself in sys.modules during initialization.
+import sys
+import simple_pkg.b
+sys.modules[__name__] = simple_pkg.b

tests/importer_test.py

@@ -12,6 +12,7 @@ import mitogen.utils
 from mitogen.core import b
 
 import testlib
+import simple_pkg.imports_replaces_self
 
 
 class ImporterMixin(testlib.RouterMixin):
@@ -214,5 +215,13 @@ class Python24LineCacheTest(testlib.TestCase):
     pass
 
 
+class SelfReplacingModuleTest(testlib.RouterMixin, testlib.TestCase):
+    # issue #590
+    def test_importer_handles_self_replacement(self):
+        c = self.router.local()
+        self.assertEquals(0,
+            c.call(simple_pkg.imports_replaces_self.subtract_one, 1))
+
+
 if __name__ == '__main__':
     unittest2.main()

tests/module_finder_test.py

@@ -5,8 +5,10 @@ import sys
 import unittest2
 
 import mitogen.master
+from mitogen.core import b
 
 import testlib
+from testlib import MODS_DIR
 
 
 class ConstructorTest(testlib.TestCase):
@@ -51,10 +53,10 @@ class IsStdlibNameTest(testlib.TestCase):
 
 
 class GetMainModuleDefectivePython3x(testlib.TestCase):
-    klass = mitogen.master.ModuleFinder
+    klass = mitogen.master.DefectivePython3xMainMethod
 
     def call(self, fullname):
-        return self.klass()._get_main_module_defective_python_3x(fullname)
+        return self.klass().find(fullname)
 
     def test_builtin(self):
         self.assertEquals(None, self.call('sys'))
@@ -77,23 +79,23 @@ class GetMainModuleDefectivePython3x(testlib.TestCase):
         self.assertFalse(is_pkg)
 
 
-class GetModuleViaPkgutilTest(testlib.TestCase):
-    klass = mitogen.master.ModuleFinder
+class PkgutilMethodTest(testlib.TestCase):
+    klass = mitogen.master.PkgutilMethod
 
     def call(self, fullname):
-        return self.klass()._get_module_via_pkgutil(fullname)
+        return self.klass().find(fullname)
 
     def test_empty_source_pkg(self):
         path, src, is_pkg = self.call('module_finder_testmod')
         self.assertEquals(path,
-            testlib.data_path('module_finder_testmod/__init__.py'))
+            os.path.join(MODS_DIR, 'module_finder_testmod/__init__.py'))
         self.assertEquals(mitogen.core.b(''), src)
         self.assertTrue(is_pkg)
 
     def test_empty_source_module(self):
         path, src, is_pkg = self.call('module_finder_testmod.empty_mod')
         self.assertEquals(path,
-            testlib.data_path('module_finder_testmod/empty_mod.py'))
+            os.path.join(MODS_DIR, 'module_finder_testmod/empty_mod.py'))
         self.assertEquals(mitogen.core.b(''), src)
         self.assertFalse(is_pkg)
 
@@ -101,23 +103,29 @@ class GetModuleViaPkgutilTest(testlib.TestCase):
         from module_finder_testmod import regular_mod
         path, src, is_pkg = self.call('module_finder_testmod.regular_mod')
         self.assertEquals(path,
-            testlib.data_path('module_finder_testmod/regular_mod.py'))
+            os.path.join(MODS_DIR, 'module_finder_testmod/regular_mod.py'))
         self.assertEquals(mitogen.core.to_text(src),
                           inspect.getsource(regular_mod))
         self.assertFalse(is_pkg)
 
 
-class GetModuleViaSysModulesTest(testlib.TestCase):
-    klass = mitogen.master.ModuleFinder
+class SysModulesMethodTest(testlib.TestCase):
+    klass = mitogen.master.SysModulesMethod
 
     def call(self, fullname):
-        return self.klass()._get_module_via_sys_modules(fullname)
+        return self.klass().find(fullname)
 
     def test_main(self):
         import __main__
         path, src, is_pkg = self.call('__main__')
         self.assertEquals(path, __main__.__file__)
-        self.assertEquals(src, open(path, 'rb').read())
+
+        # linecache adds a line ending to the final line if one is missing.
+        actual_src = open(path, 'rb').read()
+        if actual_src[-1:] != b('\n'):
+            actual_src += b('\n')
+
+        self.assertEquals(src, actual_src)
         self.assertFalse(is_pkg)
 
     def test_dylib_fails(self):
@@ -133,10 +141,10 @@ class GetModuleViaSysModulesTest(testlib.TestCase):
 
 
 class GetModuleViaParentEnumerationTest(testlib.TestCase):
-    klass = mitogen.master.ModuleFinder
+    klass = mitogen.master.ParentEnumerationMethod
 
     def call(self, fullname):
-        return self.klass()._get_module_via_parent_enumeration(fullname)
+        return self.klass().find(fullname)
 
     def test_main_fails(self):
         import __main__
@@ -157,13 +165,28 @@ class GetModuleViaParentEnumerationTest(testlib.TestCase):
         # plumbum has been eating too many rainbow-colored pills
         import pkg_like_plumbum.colors
         path, src, is_pkg = self.call('pkg_like_plumbum.colors')
-        self.assertEquals(path,
-            testlib.data_path('pkg_like_plumbum/colors.py'))
+        modpath = os.path.join(MODS_DIR, 'pkg_like_plumbum/colors.py')
+        self.assertEquals(path, modpath)
 
-        s = open(testlib.data_path('pkg_like_plumbum/colors.py'), 'rb').read()
-        self.assertEquals(src, s)
+        self.assertEquals(src, open(modpath, 'rb').read())
         self.assertFalse(is_pkg)
 
+    def test_ansible_module_utils_distro_succeeds(self):
+        # #590: a package that turns itself into a module.
+        import pkg_like_ansible.module_utils.distro as d
+        self.assertEquals(d.I_AM, "the module that replaced the package")
+        self.assertEquals(
+            sys.modules['pkg_like_ansible.module_utils.distro'].__name__,
+            'pkg_like_ansible.module_utils.distro._distro'
+        )
+
+        path, src, is_pkg = self.call('pkg_like_ansible.module_utils.distro')
+        modpath = os.path.join(MODS_DIR,
+            'pkg_like_ansible/module_utils/distro/__init__.py')
+        self.assertEquals(path, modpath)
+        self.assertEquals(src, open(modpath, 'rb').read())
+        self.assertEquals(is_pkg, True)
+
 
 class ResolveRelPathTest(testlib.TestCase):
     klass = mitogen.master.ModuleFinder
@@ -235,7 +258,7 @@ class FindRelatedTest(testlib.TestCase):
 
 if sys.version_info > (2, 6):
     class DjangoMixin(object):
-        WEBPROJECT_PATH = testlib.data_path('webproject')
+        WEBPROJECT_PATH = os.path.join(MODS_DIR, 'webproject')
 
         # TODO: rip out Django and replace with a static tree of weird imports
         # that don't depend on .. Django! The hack below is because the version

tests/testlib.py

@@ -41,7 +41,11 @@ except NameError:
 
 LOG = logging.getLogger(__name__)
 DATA_DIR = os.path.join(os.path.dirname(__file__), 'data')
+MODS_DIR = os.path.join(DATA_DIR, 'importer')
+
 sys.path.append(DATA_DIR)
+sys.path.append(MODS_DIR)
+
 
 if mitogen.is_master:
     mitogen.utils.log_to_file()