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-14T08:24:08Z

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:26:31Z

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:27:06Z

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.

…4210) 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.

…4210) 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. backported to 3.8 by Michał Górny

…4210) 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. backported to 3.9 by Michał Górny

…4210) 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. backported to 3.8 by Michał Górny

…4210) 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. backported to 3.9 by Michał Górny

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 mentioned this pull request Jan 13, 2021

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

Closed

xavfernandez reviewed Jan 14, 2021

View reviewed changes

hroncok reviewed Jan 20, 2021

View reviewed changes

hroncok approved these changes Jan 20, 2021

View reviewed changes

vstinner merged commit 75e59a9 into python:master Jan 20, 2021

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

vstinner deleted the ensurepip_dir2 branch January 20, 2021 16:07

wimglenn mentioned this pull request May 16, 2021

bpo-37449: ensurepip uses importlib.resources.files() traversable APIs #22659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Loading

vstinner commented Jan 13, 2021

vstinner commented Jan 13, 2021

vstinner commented Jan 14, 2021

vstinner commented Jan 14, 2021

xavfernandez left a comment

xavfernandez Jan 14, 2021

vstinner Jan 14, 2021

vstinner commented Jan 14, 2021

xavfernandez commented Jan 14, 2021

vstinner commented Jan 20, 2021

hroncok left a comment

hroncok Jan 20, 2021

vstinner Jan 20, 2021

hroncok Jan 20, 2021

hroncok Jan 20, 2021

vstinner Jan 20, 2021

hroncok Jan 20, 2021

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:

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 Loading

vstinner commented Jan 13, 2021

vstinner commented Jan 13, 2021

vstinner commented Jan 14, 2021

vstinner commented Jan 14, 2021

xavfernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Jan 14, 2021

xavfernandez commented Jan 14, 2021

vstinner commented Jan 20, 2021

hroncok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hroncok left a comment

Choose a reason for hiding this comment

vstinner commented Jan 20, 2021

vstinner commented Jan 20, 2021

vstinner commented Jan 13, 2021 •

edited by bedevere-bot

Loading