Issue #15343: Handle importlib.machinery.FileFinder instances in pkgutil.walk_packages (et al)

5 files changed, 106 insertions, 19 deletions

diff --git a/Doc/library/pkgutil.rst b/Doc/library/pkgutil.rst
index bcd5d91..22d44eb 100644
--- a/Doc/library/pkgutil.rst
+++ b/Doc/library/pkgutil.rst
@@ -81,7 +81,7 @@ support.
 
    .. versionchanged:: 3.3
       Updated to be based directly on :mod:`importlib` rather than relying
-      on a package internal PEP 302 import emulation.
+      on the package internal PEP 302 import emulation.
 
 
 .. function:: get_importer(path_item)
@@ -96,7 +96,7 @@ support.
 
    .. versionchanged:: 3.3
       Updated to be based directly on :mod:`importlib` rather than relying
-      on a package internal PEP 302 import emulation.
+      on the package internal PEP 302 import emulation.
 
 
 .. function:: get_loader(module_or_name)
@@ -115,7 +115,7 @@ support.
 
    .. versionchanged:: 3.3
       Updated to be based directly on :mod:`importlib` rather than relying
-      on a package internal PEP 302 import emulation.
+      on the package internal PEP 302 import emulation.
 
 
 .. function:: iter_importers(fullname='')
@@ -133,12 +133,12 @@ support.
 
    .. versionchanged:: 3.3
       Updated to be based directly on :mod:`importlib` rather than relying
-      on a package internal PEP 302 import emulation.
+      on the package internal PEP 302 import emulation.
 
 
 .. function:: iter_modules(path=None, prefix='')
 
-   Yields ``(module_loader, name, ispkg)`` for all submodules on *path*, or, if
+   Yields ``(module_finder, name, ispkg)`` for all submodules on *path*, or, if
    path is ``None``, all top-level modules on ``sys.path``.
 
    *path* should be either ``None`` or a list of paths to look for modules in.
@@ -146,19 +146,19 @@ support.
    *prefix* is a string to output on the front of every module name on output.
 
    .. note::
-      Only works with a :term:`finder` which defines an ``iter_modules()``
-      method, which is non-standard but implemented by classes defined in this
-      module.
+      Only works for a :term:`finder` which defines an ``iter_modules()``
+      method. This interface is non-standard, so the module also provides
+      implementations for :class:`importlib.machinery.FileFinder` and
+      :class:`zipimport.zipimporter`.
 
    .. versionchanged:: 3.3
-      As of Python 3.3, the import system provides finders by default, but they
-      do not include the non-standard ``iter_modules()`` method required by this
-      function.
+      Updated to be based directly on :mod:`importlib` rather than relying
+      on the package internal PEP 302 import emulation.
 
 
 .. function:: walk_packages(path=None, prefix='', onerror=None)
 
-   Yields ``(module_loader, name, ispkg)`` for all modules recursively on
+   Yields ``(module_finder, name, ispkg)`` for all modules recursively on
    *path*, or, if path is ``None``, all accessible modules.
 
    *path* should be either ``None`` or a list of paths to look for modules in.
@@ -184,13 +184,14 @@ support.
       walk_packages(ctypes.__path__, ctypes.__name__ + '.')
 
    .. note::
-      Only works for a :term:`finder` which define an ``iter_modules()`` method,
-      which is non-standard but implemented by classes defined in this module.
+      Only works for a :term:`finder` which defines an ``iter_modules()``
+      method. This interface is non-standard, so the module also provides
+      implementations for :class:`importlib.machinery.FileFinder` and
+      :class:`zipimport.zipimporter`.
 
    .. versionchanged:: 3.3
-      As of Python 3.3, the import system provides finders by default, but they
-      do not include the non-standard ``iter_modules()`` method required by this
-      function.
+      Updated to be based directly on :mod:`importlib` rather than relying
+      on the package internal PEP 302 import emulation.
 
 
 .. function:: get_data(package, resource)
diff --git a/Lib/pkgutil.py b/Lib/pkgutil.py
index 487c8cd..8407b6d 100644
--- a/Lib/pkgutil.py
+++ b/Lib/pkgutil.py
@@ -157,6 +157,49 @@ def iter_importer_modules(importer, prefix=''):
 
 iter_importer_modules = simplegeneric(iter_importer_modules)
 
+# Implement a file walker for the normal importlib path hook
+def _iter_file_finder_modules(importer, prefix=''):
+    if importer.path is None or not os.path.isdir(importer.path):
+        return
+
+    yielded = {}
+    import inspect
+    try:
+        filenames = os.listdir(importer.path)
+    except OSError:
+        # ignore unreadable directories like import does
+        filenames = []
+    filenames.sort()  # handle packages before same-named modules
+
+    for fn in filenames:
+        modname = inspect.getmodulename(fn)
+        if modname=='__init__' or modname in yielded:
+            continue
+
+        path = os.path.join(importer.path, fn)
+        ispkg = False
+
+        if not modname and os.path.isdir(path) and '.' not in fn:
+            modname = fn
+            try:
+                dircontents = os.listdir(path)
+            except OSError:
+                # ignore unreadable directories like import does
+                dircontents = []
+            for fn in dircontents:
+                subname = inspect.getmodulename(fn)
+                if subname=='__init__':
+                    ispkg = True
+                    break
+            else:
+                continue    # not a package
+
+        if modname and '.' not in modname:
+            yielded[modname] = 1
+            yield prefix + modname, ispkg
+
+iter_importer_modules.register(
+    importlib.machinery.FileFinder, _iter_file_finder_modules)
 
 class ImpImporter:
     """PEP 302 Importer that wraps Python's "classic" import algorithm
diff --git a/Lib/test/test_pkgutil.py b/Lib/test/test_pkgutil.py
index 51f5dee..73c6869 100644
--- a/Lib/test/test_pkgutil.py
+++ b/Lib/test/test_pkgutil.py
@@ -9,7 +9,11 @@ import tempfile
 import shutil
 import zipfile
 
-
+# Note: pkgutil.walk_packages is currently tested in test_runpy. This is
+# a hack to get a major issue resolved for 3.3b2. Longer term, it should
+# be moved back here, perhaps by factoring out the helper code for
+# creating interesting package layouts to a separate module.
+# Issue #15348 declares this is indeed a dodgy hack ;)
 
 class PkgutilTests(unittest.TestCase):
 
diff --git a/Lib/test/test_runpy.py b/Lib/test/test_runpy.py
index c39e281..abb7dd9 100644
--- a/Lib/test/test_runpy.py
+++ b/Lib/test/test_runpy.py
@@ -13,6 +13,7 @@ from test.support import (
 from test.script_helper import (
     make_pkg, make_script, make_zip_pkg, make_zip_script, temp_dir)
 
+
 import runpy
 from runpy import _run_code, _run_module_code, run_module, run_path
 # Note: This module can't safely test _run_module_as_main as it
@@ -148,7 +149,7 @@ class ExecutionLayerTestCase(unittest.TestCase, CodeExecutionMixin):
                                     mod_package)
         self.check_code_execution(create_ns, expected_ns)
 
-
+# TODO: Use self.addCleanup to get rid of a lot of try-finally blocks
 class RunModuleTestCase(unittest.TestCase, CodeExecutionMixin):
     """Unit tests for runpy.run_module"""
 
@@ -413,6 +414,40 @@ from ..uncle.cousin import nephew
         finally:
             self._del_pkg(pkg_dir, depth, mod_name)
 
+    def test_pkgutil_walk_packages(self):
+        # This is a dodgy hack to use the test_runpy infrastructure to test
+        # issue #15343. Issue #15348 declares this is indeed a dodgy hack ;)
+        import pkgutil
+        max_depth = 4
+        base_name = "__runpy_pkg__"
+        package_suffixes = ["uncle", "uncle.cousin"]
+        module_suffixes = ["uncle.cousin.nephew", base_name + ".sibling"]
+        expected_packages = set()
+        expected_modules = set()
+        for depth in range(1, max_depth):
+            pkg_name = ".".join([base_name] * depth)
+            expected_packages.add(pkg_name)
+            for name in package_suffixes:
+                expected_packages.add(pkg_name + "." + name)
+            for name in module_suffixes:
+                expected_modules.add(pkg_name + "." + name)
+        pkg_name = ".".join([base_name] * max_depth)
+        expected_packages.add(pkg_name)
+        expected_modules.add(pkg_name + ".runpy_test")
+        pkg_dir, mod_fname, mod_name = (
+               self._make_pkg("", max_depth))
+        self.addCleanup(self._del_pkg, pkg_dir, max_depth, mod_name)
+        for depth in range(2, max_depth+1):
+            self._add_relative_modules(pkg_dir, "", depth)
+        for finder, mod_name, ispkg in pkgutil.walk_packages([pkg_dir]):
+            self.assertIsInstance(finder,
+                                  importlib.machinery.FileFinder)
+            if ispkg:
+                expected_packages.remove(mod_name)
+            else:
+                expected_modules.remove(mod_name)
+        self.assertEqual(len(expected_packages), 0, expected_packages)
+        self.assertEqual(len(expected_modules), 0, expected_modules)
 
 class RunPathTestCase(unittest.TestCase, CodeExecutionMixin):
     """Unit tests for runpy.run_path"""
diff --git a/Misc/NEWS b/Misc/NEWS
index d67afcd..5b1845a 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -38,6 +38,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #15343: pkgutil now includes an iter_importer_modules implementation
+  for importlib.machinery.FileFinder (similar to the way it already handled
+  zipimport.zipimporter)
+
 - Issue #15314: runpy now sets __main__.__loader__ correctly
 
 - Issue #15357: The import emulation in pkgutil is now deprecated. pkgutil