Fix regression in pip parse for finding implicit namespace packages. (#504)

* Fix regression in pip parse for finding implicit namespace packages.
Pathlib.Path normalizes path names, which caused lookups of relative
paths using the special '.' directory path to fail to find parents of
standard packages.

* use paths everywhere, add test for cwd case

* refactor for mypy

Co-authored-by: Jonathon Belotti <[email protected]>

Author: hrfuller

python/pip_install/extract_wheels/lib/bazel.py

@@ -184,7 +184,7 @@ def extract_wheel(
     enable_implicit_namespace_pkgs: bool,
     incremental: bool = False,
     incremental_repo_prefix: Optional[str] = None,
-) -> str:
+) -> Optional[str]:
     """Extracts wheel into given directory and creates py_library and filegroup targets.
 
     Args:
@@ -250,5 +250,5 @@ def extract_wheel(
 
     if not incremental:
         os.remove(whl.path)
-
-    return "//%s" % directory
+        return f"//{directory}"
+    return None

python/pip_install/extract_wheels/lib/namespace_pkgs.py

@@ -1,13 +1,13 @@
 """Utility functions to discover python package types"""
 import os
-import pathlib  # supported in >= 3.4
+from pathlib import Path  # supported in >= 3.4
 import textwrap
 from typing import Set, List, Optional
 
 
 def implicit_namespace_packages(
     directory: str, ignored_dirnames: Optional[List[str]] = None
-) -> Set[str]:
+) -> Set[Path]:
     """Discovers namespace packages implemented using the 'native namespace packages' method.
 
     AKA 'implicit namespace packages', which has been supported since Python 3.3.
@@ -20,34 +20,36 @@ def implicit_namespace_packages(
     Returns:
         The set of directories found under root to be packages using the native namespace method.
     """
-    namespace_pkg_dirs: Set[str] = set()
-    standard_pkg_dirs: Set[str] = set()
+    namespace_pkg_dirs: Set[Path] = set()
+    standard_pkg_dirs: Set[Path] = set()
+    directory_path = Path(directory)
+    ignored_dirname_paths: List[Path] = [Path(p) for p in ignored_dirnames or ()]
     # Traverse bottom-up because a directory can be a namespace pkg because its child contains module files.
-    for dirpath, dirnames, filenames in os.walk(directory, topdown=False):
+    for dirpath, dirnames, filenames in map(lambda t: (Path(t[0]), *t[1:]), os.walk(directory_path, topdown=False)):
         if "__init__.py" in filenames:
             standard_pkg_dirs.add(dirpath)
             continue
-        elif ignored_dirnames:
-            is_ignored_dir = dirpath in ignored_dirnames
-            child_of_ignored_dir = any(d in pathlib.Path(dirpath).parents for d in ignored_dirnames)
+        elif ignored_dirname_paths:
+            is_ignored_dir = dirpath in ignored_dirname_paths
+            child_of_ignored_dir = any(d in dirpath.parents for d in ignored_dirname_paths)
             if is_ignored_dir or child_of_ignored_dir:
                 continue
 
         dir_includes_py_modules = _includes_python_modules(filenames)
-        parent_of_namespace_pkg = any(str(pathlib.Path(dirpath, d)) in namespace_pkg_dirs for d in dirnames)
-        parent_of_standard_pkg = any(str(pathlib.Path(dirpath, d)) in standard_pkg_dirs for d in dirnames)
+        parent_of_namespace_pkg = any(Path(dirpath, d) in namespace_pkg_dirs for d in dirnames)
+        parent_of_standard_pkg = any(Path(dirpath, d) in standard_pkg_dirs for d in dirnames)
         parent_of_pkg = parent_of_namespace_pkg or parent_of_standard_pkg
         if (
             (dir_includes_py_modules or parent_of_pkg)
             and
             # The root of the directory should never be an implicit namespace
-            dirpath != directory
+            dirpath != directory_path
         ):
             namespace_pkg_dirs.add(dirpath)
     return namespace_pkg_dirs
 
 
-def add_pkgutil_style_namespace_pkg_init(dir_path: str) -> None:
+def add_pkgutil_style_namespace_pkg_init(dir_path: Path) -> None:
     """Adds 'pkgutil-style namespace packages' init file to the given directory
 
     See: https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
@@ -95,7 +97,7 @@ def _includes_python_modules(files: List[str]) -> bool:
         ".pyd"  # https://docs.python.org/3/faq/windows.html#is-a-pyd-file-the-same-as-a-dll
     }
     return any(
-        pathlib.Path(f).suffix in module_suffixes
+        Path(f).suffix in module_suffixes
         for f
         in files
     )

python/pip_install/extract_wheels/lib/namespace_pkgs_test.py

@@ -1,7 +1,8 @@
+import os
 import pathlib
 import shutil
 import tempfile
-from typing import Optional
+from typing import Optional, Set
 import unittest
 
 from python.pip_install.extract_wheels.lib import namespace_pkgs
@@ -32,6 +33,30 @@ def remove(self) -> None:
 
 
 class TestImplicitNamespacePackages(unittest.TestCase):
+
+    def assertPathsEqual(self, actual: Set[pathlib.Path], expected: Set[str]) -> None:
+        self.assertEqual(actual, {pathlib.Path(p) for p in expected})
+
+    def test_in_current_directory(self) -> None:
+        directory = TempDir()
+        directory.add_file("foo/bar/biz.py")
+        directory.add_file("foo/bee/boo.py")
+        directory.add_file("foo/buu/__init__.py")
+        directory.add_file("foo/buu/bii.py")
+        cwd = os.getcwd()
+        os.chdir(directory.root())
+        expected = {
+            "foo",
+            "foo/bar",
+            "foo/bee",
+        }
+        try:
+            actual = namespace_pkgs.implicit_namespace_packages(".")
+            self.assertPathsEqual(actual, expected)
+        finally:
+            os.chdir(cwd)
+            directory.remove()
+
     def test_finds_correct_namespace_packages(self) -> None:
         directory = TempDir()
         directory.add_file("foo/bar/biz.py")
@@ -45,7 +70,7 @@ def test_finds_correct_namespace_packages(self) -> None:
             directory.root() + "/foo/bee",
         }
         actual = namespace_pkgs.implicit_namespace_packages(directory.root())
-        self.assertEqual(actual, expected)
+        self.assertPathsEqual(actual, expected)
 
     def test_ignores_empty_directories(self) -> None:
         directory = TempDir()
@@ -57,7 +82,7 @@ def test_ignores_empty_directories(self) -> None:
             directory.root() + "/foo/bar",
         }
         actual = namespace_pkgs.implicit_namespace_packages(directory.root())
-        self.assertEqual(actual, expected)
+        self.assertPathsEqual(actual, expected)
 
     def test_empty_case(self) -> None:
         directory = TempDir()
@@ -87,7 +112,7 @@ def test_parent_child_relationship_of_namespace_pkgs(self):
             directory.root() + "/foo/bar/biff",
         }
         actual = namespace_pkgs.implicit_namespace_packages(directory.root())
-        self.assertEqual(actual, expected)
+        self.assertPathsEqual(actual, expected)
 
     def test_parent_child_relationship_of_namespace_and_standard_pkgs(self):
         directory = TempDir()
@@ -99,7 +124,7 @@ def test_parent_child_relationship_of_namespace_and_standard_pkgs(self):
             directory.root() + "/foo/bar",
         }
         actual = namespace_pkgs.implicit_namespace_packages(directory.root())
-        self.assertEqual(actual, expected)
+        self.assertPathsEqual(actual, expected)
 
     def test_parent_child_relationship_of_namespace_and_nested_standard_pkgs(self):
         directory = TempDir()
@@ -115,7 +140,7 @@ def test_parent_child_relationship_of_namespace_and_nested_standard_pkgs(self):
             directory.root() + "/fim",
         }
         actual = namespace_pkgs.implicit_namespace_packages(directory.root())
-        self.assertEqual(actual, expected)
+        self.assertPathsEqual(actual, expected)
 
     def test_recognized_all_nonstandard_module_types(self):
         directory = TempDir()
@@ -132,7 +157,7 @@ def test_recognized_all_nonstandard_module_types(self):
             directory.root() + "/eff/jee",
         }
         actual = namespace_pkgs.implicit_namespace_packages(directory.root())
-        self.assertEqual(actual, expected)
+        self.assertPathsEqual(actual, expected)
 
     def test_skips_ignored_directories(self):
         directory = TempDir()
@@ -147,7 +172,7 @@ def test_skips_ignored_directories(self):
             directory.root(),
             ignored_dirnames=[directory.root() + "/foo/boo"],
         )
-        self.assertEqual(actual, expected)
+        self.assertPathsEqual(actual, expected)
 
 
 if __name__ == "__main__":

python/pip_install/extract_wheels/lib/whl_filegroup_test.py

@@ -34,8 +34,10 @@ def _run(
             enable_implicit_namespace_pkgs=False,
             incremental=incremental,
             incremental_repo_prefix=incremental_repo_prefix
-        )[2:]  # Take off the leading // from the returned label.
+        )
+        # Take off the leading // from the returned label.
         # Assert that the raw wheel ends up in the package.
+        generated_bazel_dir = generated_bazel_dir[2:] if not incremental else self.wheel_dir
         self.assertIn(self.wheel_name, os.listdir(generated_bazel_dir))
         with open("{}/BUILD.bazel".format(generated_bazel_dir)) as build_file:
             build_file_content = build_file.read()

python/pip_install/parse_requirements_to_bzl/__init__.py

@@ -118,6 +118,8 @@ def install_deps():
             )
         )
 
+def coerce_to_bool(option):
+    return str(option).lower() == 'true'
 
 def main() -> None:
     parser = argparse.ArgumentParser(
@@ -132,8 +134,8 @@ def main() -> None:
     )
     parser.add_argument(
         "--quiet",
-        type=bool,
-        action="store",
+        type=coerce_to_bool,
+        default=True,
         required=True,
         help="Whether to print stdout / stderr from child repos.",
     )