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

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

Merged
merged 3 commits into from
Jan 20, 2023

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented Nov 15, 2022

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 marked this pull request as ready for review November 15, 2022 17:41
@kdeldycke
Copy link

  • 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
Contributor Author

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

@warsaw
Copy link
Contributor

warsaw commented Jan 12, 2023

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

warsaw commented Jan 14, 2023

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
Member

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

Copy link
Member

@jaraco jaraco left a comment

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