python / importlib_metadata

Library to access metadata for Python packages
https://importlib-metadata.readthedocs.io
Apache License 2.0
123 stars 80 forks source link

Add missing modules to packages_distributions #432

Closed FFY00 closed 1 year ago

FFY00 commented 1 year ago

This now considers:

Which I think should be all possible modules, right?

Fix #428

FFY00 commented 1 year ago

Edge case I found: empty namespace packages, which are still importable, can't ever be reported with the current abstractions. I don't think this is a big issue though, https://github.com/pypa/installer also does not support creating empty directories, so people trying to ship packages containing such thing will likely get trouble trying to install them in the first place.

jherland commented 1 year ago

I wonder if this adds the metadata directories into the output of packages_distributions() as import names. After this was merged, I'm seeing entries like these returned from packages_distributions():

 'black-23.1.0.dist-info': ['black'],
 'filelock-3.10.0.dist-info': ['filelock'],
 'iniconfig-2.0.0.dist-info': ['iniconfig'],
 'more_itertools-9.1.0.dist-info': ['more-itertools'],
 'typing_extensions-4.5.0.dist-info': ['typing_extensions'],

IMHO, these should never occur here, as they are not importable modules.

jherland commented 1 year ago

Confirmed by applying this diff to the test added in this PR; the important stuff is in the last 4 lines, but I first needed to change the existing dummy module names into valid import names (== valid Python identifiers):

diff --git a/tests/test_main.py b/tests/test_main.py
index 1636779..d448e6c 100644
--- a/tests/test_main.py
+++ b/tests/test_main.py
@@ -336,10 +336,10 @@ class PackagesDistributionsTest(
                         Version: 1.0.0
                     """,
                     'RECORD': ''.join(
-                        f'{i}-top-level{suffix},,\n'
-                        f'{i}-in-namespace/mod{suffix},,\n'
-                        f'{i}-in-package/__init__.py,,\n'
-                        f'{i}-in-package/mod{suffix},,\n'
+                        f'top_level_{i}{suffix},,\n'
+                        f'in_namespace_{i}/mod{suffix},,\n'
+                        f'in_package_{i}/__init__.py,,\n'
+                        f'in_package_{i}/mod{suffix},,\n'
                         for i, suffix in enumerate(suffixes)
                     ),
                 },
@@ -350,6 +350,11 @@ class PackagesDistributionsTest(
         distributions = packages_distributions()

         for i in range(len(suffixes)):
-            assert distributions[f'{i}-top-level'] == ['all_distributions']
-            assert distributions[f'{i}-in-namespace'] == ['all_distributions']
-            assert distributions[f'{i}-in-package'] == ['all_distributions']
+            assert distributions[f'top_level_{i}'] == ['all_distributions']
+            assert distributions[f'in_namespace_{i}'] == ['all_distributions']
+            assert distributions[f'in_package_{i}'] == ['all_distributions']
+
+        # All keys returned from packages_distributions() should be valid import
+        # names, which means that they must _at least_ be valid identifiers:
+        for import_name in distributions.keys():
+            assert import_name.isidentifier(), import_name
jherland commented 1 year ago

I wonder if the final if f.suffix == ".py" clause in the set comprehension that was lost in this PR should instead be replaced with something like if f.suffix in importlib.machinery.all_suffixes()? Otherwise it seems we accept any first-level directories that are part of .files paths, even those that do not contain any packages/modules.

ucodery commented 1 year ago

I came here to say the same thing as @jherland. The introduction of package.version.dist-info has caused problems with one of my apps that started finding "extra" import targets. It's not only metadata dirs, this change also adds .. as a key in some cases, presumably for any distribution that ships an executable and has files like ../../../bin/tool associated with it.

To reproduce:

  1. create a new venv
  2. pip install importlib_metadata==6.1 wheel
  3. $ python -m pip list
    Package            Version
    ------------------ -------
    importlib-metadata 6.1.0
    pip                21.2.3
    setuptools         57.4.0
    wheel              0.40.0
    zipp               3.15.0
  4. $ python -c "import importlib_metadata;print(importlib_metadata.packages_distributions())"
    {'_distutils_hack': ['setuptools'], 'pkg_resources': ['setuptools'], 'setuptools': ['setuptools'], 'zipp': ['zipp'], 'wheel-0.40.0.dist-info': ['wheel'], 'wheel': ['wheel'], '..': ['wheel'], 'pip': ['pip'], 'importlib_metadata': ['importlib-metadata']}