bpo-42856: Add --with-wheel-pkg-dir=PATH configure option #24210

vstinner · 2021-01-13T14:43:23Z

Add --with-wheel-pkg-dir=PATH option to the ./configure script. If
specified, the ensurepip module looks for setuptools and pip wheel
packages in this directory: if both are present, these wheel packages
are used instead of ensurepip bundled wheel packages.

Some Linux distribution packaging policies recommand against bundling
dependencies. For example, Fedora installs wheel packages in the
/usr/share/python-wheels/ directory and don't install the
ensurepip._bundled package.

ensurepip: Remove unused runpy import.

https://bugs.python.org/issue42856

vstinner · 2021-01-13T14:47:53Z

This PR is my second attempt which is way less than intrusive.

Changes since my first attempt PR #24154:

Only use the wheel package directory if setuptools and pip wheel packages are found there. Otherwise, use existing code.
Revert most changes to leave most of existing code unchanged: common case when --with-wheel-pkg-dir is not used.
Don't attempt to compare version numbers anymore. Only use sort(filename) for the case which should not happen: whne the wheel package directory contains multiple files for pip or setuptools. It prevents to have to bother with complex versions numbers like 20.3b1 (pip-20.3b1-py2.py3-none-any.whl filename).

vstinner · 2021-01-13T17:30:29Z

I created a PR on the Fedora python3.10 package to test the integration of my patch:
https://src.fedoraproject.org/rpms/python3.10/pull-request/22

vstinner · 2021-01-14T07:46:29Z

@xavfernandez @pradyunsg @pablogsal @kkonopko @hroncok @nanjekyejoannah: Hi, does anyone of you want to review this PR? See https://bugs.python.org/issue42856 for the rationale. (I looked at who modified Lib/ensurepip/ last months.)

vstinner · 2021-01-14T08:57:46Z

I fixed test_ensurepip when ensurepip._bundled is missing (on Fedora).

xavfernandez

I did not review the non-python files.

The python part seems ok but now feels somewhat complicated.

I wonder if we should not drop _SETUPTOOLS_VERSION, _PIP_VERSION & _PROJECTS variables in favor of using importlib.resources.files on ensurepip._bundled to automatically discover the bundled versions (similarly to what is now done with _WHEEL_PKG_DIR).

This would ease the next pip/setuptools versions bump: all that would be necessary would be to replace the wheels in Lib/ensurepip/_bundled/ without specifying the expected versions.

xavfernandez · 2021-01-14T09:11:29Z

Lib/test/test_ensurepip.py

+    def test_version(self):
+        # Test version()
+        with tempfile.TemporaryDirectory() as tmpdir:
+            self.touch(tmpdir, "pip-1.2.3b1-py2.py3-none-any.whl")


Any reason for not using pathlib ?

Suggested change

self.touch(tmpdir, "pip-1.2.3b1-py2.py3-none-any.whl")

pathlib.Path(tmpdir, "pip-1.2.3b1-py2.py3-none-any.whl").touch()

I prefer to avoid testing pathlib in test_ensurepip :-) I like low-level code :-D

vstinner · 2021-01-14T10:41:17Z

@xavfernandez:

I wonder if we should not drop _SETUPTOOLS_VERSION, _PIP_VERSION & _PROJECTS variables in favor of using importlib.resources.files on ensurepip._bundled to automatically discover the bundled versions (similarly to what is now done with _WHEEL_PKG_DIR).

Sure, that's doable, but can it be done in a following PR? I tried to reduce changes in my PR to focus on the new feature (and reduce any risk of regression).

My first PR attempt scanned ensurepip/_bundled/ to discover the version and Python tag of bundled setuptools and pip. It reused the os.listdir() code, but importlib.resources.files sounds like a better idea for bundled wheel packages.

xavfernandez · 2021-01-14T13:54:09Z

but can it be done in a following PR ?

certainly :)

vstinner · 2021-01-20T14:33:19Z

I tested manually 3 cases using test_ensurepip.

I added logs to see which packages are used with log.patch file:

diff --git a/Lib/ensurepip/__init__.py b/Lib/ensurepip/__init__.py
index 8b000f93db..292b47002a 100644
--- a/Lib/ensurepip/__init__.py
+++ b/Lib/ensurepip/__init__.py
@@ -162,10 +162,12 @@ def _bootstrap(*, root=None, upgrade=False, user=False,
                 from ensurepip import _bundled
                 wheel_name = package.wheel_name
                 whl = resources.read_binary(_bundled, wheel_name)
+                print("WHEEL: read_binary")
             else:
                 with open(package.filename, "rb") as fp:
                     whl = fp.read()
                 wheel_name = os.path.basename(package.filename)
+                print("WHEEL: copy file", package.filename)
 
             filename = os.path.join(tmpdir, wheel_name)
             with open(filename, "wb") as fp:

Case 1: only bundled

git clean -fdx
git checkout .
git apply ~/log.patch
./configure --with-pydebug CFLAGS="-O0" && make && ./python -m test -v test_ensurepip

=> OK: BUNDLED is used, test_ensurepip pass

Case 2: only wheel dir

git clean -fdx
git checkout .
git apply ~/log.patch
rm -rf Lib/ensurepip/_bundled/
./configure --with-pydebug CFLAGS="-O0" --with-wheel-pkg-dir=/usr/share/python-wheels && make && ./python -m test -v test_ensurepip

=> OK: WHEEL DIR is used, test_ensurepip pass

Case 3: both, wheel dir + bundled

git clean -fdx
git checkout .
git apply ~/log.patch
./configure --with-pydebug CFLAGS="-O0" --with-wheel-pkg-dir=/usr/share/python-wheels && make && ./python -m test -v test_ensurepip

=> OK: WHEEL DIR is used, test_ensurepip pass

hroncok

(I haven't check the configure changes, because I don't understand autotools.)

Lib/ensurepip/__init__.py

hroncok · 2021-01-20T14:33:43Z

Lib/ensurepip/__init__.py

+            packages = dir_packages
+    _PACKAGES = packages
+    return packages
+_PACKAGES = None


Why not move this to other constants?

It's not a constant but a cache for _get_packages(). I prefer to keep it close to the function definition.

Your call. The missing empty line looks weird thou.

hroncok · 2021-01-20T14:33:43Z

Lib/ensurepip/__init__.py

-            )
-            with open(os.path.join(tmpdir, wheel_name), "wb") as fp:
+        for name, package in _get_packages().items():
+            if package.wheel_name:


This is confusing. What about:

Suggested change

if package.wheel_name:

# bundled wheels have names:

if package.wheel_name:

I added comments inside, is it better?

Lib/ensurepip/__init__.py

Lib/test/test_ensurepip.py

hroncok

Awesome, thanks!

Add --with-wheel-pkg-dir=PATH option to the ./configure script. If specified, the ensurepip module looks for setuptools and pip wheel packages in this directory: if both are present, these wheel packages are used instead of ensurepip bundled wheel packages. Some Linux distribution packaging policies recommend against bundling dependencies. For example, Fedora installs wheel packages in the /usr/share/python-wheels/ directory and don't install the ensurepip._bundled package. ensurepip: Remove unused runpy import.

vstinner · 2021-01-20T15:21:06Z

Oops, there is spelling mistake: recommand => recommend. I updated my PR :-)

vstinner · 2021-01-20T16:07:40Z

Thanks for the reviews @xavfernandez and @hroncok! It's now merged.

the-knights-who-say-ni added the CLA signed label Jan 13, 2021

bedevere-bot added the awaiting core review label Jan 13, 2021

vstinner force-pushed the vstinner:ensurepip_dir2 branch from 5f75b90 to 8a97d1e Jan 13, 2021

vstinner mentioned this pull request Jan 13, 2021

[WIP] bpo-42856: Add --with-wheel-pkg-dir=PATH configure option #24151

Closed

vstinner force-pushed the vstinner:ensurepip_dir2 branch from 8a97d1e to 70e6f44 Jan 14, 2021

xavfernandez reviewed Jan 14, 2021

View changes

hroncok reviewed Jan 20, 2021

View changes

hroncok approved these changes Jan 20, 2021

View changes

vstinner force-pushed the vstinner:ensurepip_dir2 branch from 37f4bac to ee03e54 Jan 20, 2021

bedevere-bot removed the awaiting core review label Jan 20, 2021

vstinner deleted the vstinner:ensurepip_dir2 branch Jan 20, 2021

python / cpython

bpo-42856: Add --with-wheel-pkg-dir=PATH configure option #24210

bpo-42856: Add --with-wheel-pkg-dir=PATH configure option #24210

vstinner commented Jan 13, 2021 •

edited by bedevere-bot

vstinner commented Jan 13, 2021

vstinner commented Jan 13, 2021

vstinner commented Jan 14, 2021

vstinner commented Jan 14, 2021

xavfernandez left a comment

This comment has been minimized.

This comment has been minimized.

vstinner commented Jan 14, 2021

xavfernandez commented Jan 14, 2021

vstinner commented Jan 20, 2021

hroncok left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hroncok left a comment

vstinner commented Jan 20, 2021

vstinner commented Jan 20, 2021

	self.touch(tmpdir, "pip-1.2.3b1-py2.py3-none-any.whl")
	pathlib.Path(tmpdir, "pip-1.2.3b1-py2.py3-none-any.whl").touch()

	if package.wheel_name:
	# bundled wheels have names:
	if package.wheel_name:

python / cpython

Sponsor python/cpython

bpo-42856: Add --with-wheel-pkg-dir=PATH configure option #24210

bpo-42856: Add --with-wheel-pkg-dir=PATH configure option #24210

Conversation

vstinner commented Jan 13, 2021 • edited by bedevere-bot

vstinner commented Jan 13, 2021

vstinner commented Jan 13, 2021

vstinner commented Jan 14, 2021

vstinner commented Jan 14, 2021

xavfernandez left a comment

This comment has been minimized.

xavfernandez Jan 14, 2021 Contributor

This comment has been minimized.

vstinner Jan 14, 2021 Author Member

vstinner commented Jan 14, 2021

xavfernandez commented Jan 14, 2021

vstinner commented Jan 20, 2021

hroncok left a comment

This comment has been minimized.

hroncok Jan 20, 2021 Contributor

This comment has been minimized.

vstinner Jan 20, 2021 Author Member

This comment has been minimized.

hroncok Jan 20, 2021 Contributor

This comment has been minimized.

hroncok Jan 20, 2021 Contributor

This comment has been minimized.

vstinner Jan 20, 2021 Author Member

This comment has been minimized.

hroncok Jan 20, 2021 Contributor

hroncok left a comment

vstinner commented Jan 20, 2021

vstinner commented Jan 20, 2021

vstinner commented Jan 13, 2021 •

edited by bedevere-bot

xavfernandez Jan 14, 2021
Contributor

vstinner Jan 14, 2021
Author Member

hroncok Jan 20, 2021
Contributor

vstinner Jan 20, 2021
Author Member

hroncok Jan 20, 2021
Contributor

hroncok Jan 20, 2021
Contributor

vstinner Jan 20, 2021
Author Member

hroncok Jan 20, 2021
Contributor