Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing modules to packages_distributions #432

Merged
merged 4 commits into from Mar 18, 2023
Merged

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Mar 10, 2023

This now considers:

  • Modules that don't have the .py prefix (eg. native modules, .pyc-only modules)
  • Namespace packages without any .py files inside

Which I think should be all possible modules, right?

Fix #428

@FFY00 FFY00 requested a review from jaraco March 10, 2023 01:06
@FFY00 FFY00 force-pushed the gh-428 branch 2 times, most recently from e456fcf to 8d9bc40 Compare March 10, 2023 02:17
@FFY00
Copy link
Member Author

FFY00 commented Mar 10, 2023

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.

@jaraco jaraco merged commit 66e8e9b into python:main Mar 18, 2023
@jherland
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

ucodery commented Mar 25, 2023

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
$ 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
$ 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']}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incomplete packages_distributions
4 participants