Skip to content

pkg_resources: Flip import conditionals around pkgutil.ImpImporter and importlib.machinery.FileFinder#3685

Merged
abravalheri merged 3 commits into
pypa:mainfrom
abravalheri:issue-3631
Jan 20, 2023
Merged

pkg_resources: Flip import conditionals around pkgutil.ImpImporter and importlib.machinery.FileFinder#3685
abravalheri merged 3 commits into
pypa:mainfrom
abravalheri:issue-3631

Conversation

@abravalheri

@abravalheri abravalheri commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

Summary of changes

  • In Python 3.3+ importlib.machinery.FileFinder is always present, therefore we no longer need to keep the conditional
  • In Python 3.12+ pkgutil.ImpImporter is removed, so we need a conditional to avoid using it directly.

(Potentially) closes #3631

Notes

  • pkg_resources uses pkgutil.get_importer to find out which importer is responsible for importing an specific file.
    • I don't know if there is a situation in Python >= 3.7, < 3.12 where this function can return an instance of pkgutil.ImpImporter.
    • In the case pkgutil.get_importer never returns pkgutil.ImpImporter, I believe we could simply remove pkgutil.ImpImporter from the namespace handler and finder registries in pkg_resources (but don't quote me on that 😅).
  • Issue [BUG] setuptools is using deprecated and slated to be removed APIs #3631 also indicates that the find_module will be removed in 3.12.
    • pkg_resources use this function just as a contingency in the case find_spec is not defined.
      • This might be particularly important for zipimporter in Python >=3.7, < 3.10
    • For Python >= 3.12, find_spec should always be defined, and therefore I believe the code path is not triggered.
  • Both pkg_resources.find_on_path and pkg_resources.file_ns_handler ignore the importer argument.
    • If needed we can also play with that.

Pull Request Checklist

@abravalheri abravalheri requested a review from jaraco November 15, 2022 17:41
@abravalheri abravalheri marked this pull request as ready for review November 15, 2022 17:41
@jaraco jaraco force-pushed the main branch 3 times, most recently from a85759c to 93ce5a0 Compare December 16, 2022 18:21
@kdeldycke

Copy link
Copy Markdown
  • This might be particularly important for zipimporter in Python >=3.7, < 3.10

Can confirm the issue on Python 3.12-dev. I stumble upon it running some pip code which depends on a vendored pkg_resources module: pypa/pip#11688

@abravalheri

Copy link
Copy Markdown
Contributor Author

Hi @warsaw, do you think the changes here are enough to fix the problem described in #3631?

@warsaw

warsaw commented Jan 12, 2023

Copy link
Copy Markdown
Contributor

@abravalheri Just from a visual inspection of the diff, it looks pretty good. Thanks. I'll try to find some time to actually build the branch and plumb it through to see if any other problems arise.

@warsaw

warsaw commented Jan 14, 2023

Copy link
Copy Markdown
Contributor

Running tox on this PR branch on macOS 13.1, I get a bunch of failures:

================================================ short test summary info =================================================
SKIPPED [1] pkg_resources/tests/test_pkg_resources.py:379: Testing case-insensitive filesystems.
SKIPPED [3] pkg_resources/tests/test_pkg_resources.py:396: Testing systems using backslashes as path separators.
SKIPPED [1] setuptools/tests/test_develop.py:64: TODO: needs a fixture to cause 'develop' to be invoked without mutating environment.
SKIPPED [1] setuptools/tests/test_easy_install.py:266: Test can only be run on Linux
SKIPPED [1] setuptools/tests/test_install_scripts.py:50: Windows only
SKIPPED [1] setuptools/tests/test_install_scripts.py:78: Windows only
SKIPPED [1] setuptools/tests/test_msvc14.py:16: These tests are only for win32
SKIPPED [1] setuptools/tests/test_msvc14.py:34: These tests are only for win32
SKIPPED [1] setuptools/tests/test_msvc14.py:52: These tests are only for win32
SKIPPED [1] setuptools/tests/test_msvc14.py:68: These tests are only for win32
SKIPPED [1] setuptools/tests/test_sdist.py:469: pytest-dev/pytest-xdist#843
SKIPPED [1] setuptools/tests/test_sdist.py:331: pytest-dev/pytest-xdist#843
SKIPPED [1] setuptools/tests/test_editable_install.py:935: compilers may fail without correct setup
SKIPPED [1] setuptools/tests/test_windows_wrappers.py:80: Windows only
SKIPPED [1] setuptools/tests/test_windows_wrappers.py:121: Windows only
SKIPPED [1] setuptools/tests/test_windows_wrappers.py:182: Windows only
SKIPPED [8] conftest.py:51: skipping integration tests
XFAIL setuptools/tests/test_build_py.py::test_excluded_subpackages - reason: #3260
XFAIL setuptools/tests/test_dist.py::test_read_metadata[Metadata Version 1.2: Project-Url-attrs5] - Issue #1578: project_urls not read
XFAIL setuptools/tests/test_dist.py::test_read_metadata[Metadata Version 2.1: Provides Extra-attrs9] - provides_extras not read
XFAIL setuptools/tests/test_egg_info.py::TestEggInfo::test_requires[extras_require_with_marker_in_setup_cfg]
XFAIL setuptools/tests/test_integration.py::test_virtualenvwrapper
XFAIL setuptools/tests/test_sdist.py::TestSdistTest::test_sdist_with_utf8_encoded_filename - System does not support latin-1 filenames
XFAIL setuptools/tests/test_sdist.py::TestSdistTest::test_read_manifest_skips_non_utf8_filenames - System does not support latin-1 filenames
XFAIL setuptools/tests/config/test_apply_pyprojecttoml.py::test_utf8_maintainer_in_metadata[international-email] - CPython's `email.headerregistry.Address` only supports RFC 5322, as of Nov 10, 2022 and latest Python 3.11.0
XFAIL setuptools/tests/test_integration.py::test_python_novaclient
XFAIL setuptools/tests/test_virtualenv.py::test_pip_upgrade_from_source[pip<20] - pypa/pip#6599
XPASS setuptools/tests/test_archive_util.py::test_unicode_files #710 and #712
XPASS setuptools/tests/test_virtualenv.py::test_pip_upgrade_from_source[https://github.com/pypa/pip/archive/main.zip] #2975
===================== 4 failed, 1246 passed, 26 skipped, 10 xfailed, 2 xpassed in 192.81s (0:03:12) ======================
python: exit 1 (194.51 seconds) /Users/barry/projects/setuptools> pytest pid=30101
.pkg: _exit> python /usr/local/Cellar/tox/4.2.8/libexec/lib/python3.11/site-packages/pyproject_api/_backend.py True setuptools.build_meta
  python: FAIL code 1 (249.11=setup[54.60]+cmd[194.51] seconds)
  evaluation failed :( (249.59 seconds)

I also don't know how to create a whl locally that I could then drop into my branch on CPython to test my imp removal changes.

@pradyunsg

Copy link
Copy Markdown
Member
pip wheel https://github.com/abravalheri/setuptools/archive/refs/heads/issue-3631.zip --no-deps

@jaraco jaraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this change, although it leaves it to users to encounter it in Python 3.12. Presumably, they've been encountering deprecation warnings previously, so it's probably safe to remove support for it, but I'm also fine with just being cautious.

This issue reminds me that we really need to remove setuptools' dependence on pkg_resources so that the whole pkg_resources can be deprecated and removed.

@abravalheri

Copy link
Copy Markdown
Contributor Author

Hi @jaraco thank you very much for having a look. I don't get exactly what you mean by it leaves it to users to encounter it in Python 3.12. Although pkg_resources is still left behind, this PR should fix the specific error that was reported (while still maintaining backward compatibility in older Python versions), right?

Later we can keep working for the removal of pkg_resources.

@abravalheri

Copy link
Copy Markdown
Contributor Author

Thank you very much @warsaw for checking the PR. I just rebased the PR so I am hopping some of the errors you are seeing got solved.

I am not exactly sure which tests are failing in your setup (maybe the relevant parts of the test logs got removed out accidentally?). With the rebase, I can successfully running the following:

docker run --rm -it python:3.12-rc-bullseye /bin/bash

git clone https://github.com/abravalheri/setuptools -b issue-3631 /tmp/setuptools
cd /tmp/setuptools
python -m venv .venv
.venv/bin/python -m pip install -U pip tox
.venv/bin/tox -- -p no:cov -p no:perf -p no:flake8
# ...
# ====================================== 1115 passed, 29 skipped, 10 xfailed, 2 xpassed, 1 warning in 67.06s (0:01:07) =======================================
# .pkg: _exit> python /tmp/setuptools/.venv/lib/python3.12/site-packages/pyproject_api/_backend.py True setuptools.build_meta
#  python: OK (88.86=setup[21.35]+cmd[67.51] seconds)
#  congratulations :) (88.95 seconds)

Here I am disabling the following plugins:

  • pytest-cov because of the noise
  • pytest-perf because it can be a bit problematic (sometimes it can get in trouble to find the baseline for comparison when using a clone from a fork)
  • pytest-flake8 which seems to have some problems with importlib.metadata in this version of Python 3.12...

Of course this may not be very representative because the container corresponds to an old snapshot for Python 3.12 development. Also, in macOS things may be very different (I am afraid I don't have access to this system to test).

If the tests for pkg_resources are solved by this PR, I think we can close the issue. If there are any other problems for the setuptools tests we can create some new tickets...

@abravalheri abravalheri merged commit e03f883 into pypa:main Jan 20, 2023
@abravalheri abravalheri deleted the issue-3631 branch January 20, 2023 11:43
diazona added a commit to diazona/setuptools-pyproject-migration that referenced this pull request Dec 13, 2023
Older versions of setuptools fail to import distutils on Python 3.12,
presumably due to the removal of distutils from the standard library.
This bug was fixed in pypa/setuptools#3685, and
setuptools 66.1 was the first version released that includes the fix.
(I'm not sure exactly how that PR fixed the bug, but it doesn't matter.)

Because setuptools 66.0 understandably does not declare itself to be
incompatible with Python 3.12, I'm recording that constraint here.
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.

[BUG] setuptools is using deprecated and slated to be removed APIs

5 participants