Try using ensurepip instead of get-pip #665

joerick · 2021-05-08T16:56:20Z

Eliminating get-pip.py removes a vendored dependency.

cibuildwheel/windows.py

joerick · 2021-05-09T09:35:18Z

Hmm. It was working, but the 3.8.10 update broke it on mac! I wonder what changed...

YannickJadoul · 2021-05-09T12:57:02Z

Cross-linking to #210. Great that it does now seem to be working! :-)

joerick · 2021-05-10T19:37:39Z

This is ready to go I think. To explain what's going on here, we need to ensure that pip is available on PATH as pip, because user options like BEFORE_BUILD will likely use that. But distributions differ in that respect, some have pip available, some only as pip3, some only have pip in site-packages for use as python -m pip.

So, to ensure that pip is available, you run python -m ensurepip --default-pip. But! If pip is installed, ensurepip does nothing, so it doesn't act on --default-pip. So the only way to get --default-pip to work is to make sure that ensurepip is actually installing pip! So that's the why behind the little dance we're doing here.

This seems okay, but alternative ideas welcome!

henryiii · 2021-05-11T14:11:19Z

cibuildwheel/macos.py

+        try:
+            call(["python", "-m", "pip", "--version"], env=env, cwd="/tmp")
+        except subprocess.CalledProcessError:
+            pip_is_installed = False
+        else:
+            pip_is_installed = True


This feels slightly overkill with an exception. How about adding check: bool=True to call, then passing check=check through to subprocess.run, and return the result? Then this could be:

pip_is_installed = call(["python", "-m", "pip", "--version"], env=env, cwd="/tmp", check=False).returncode == 0

(same thing for Windows)

haha, yes that was my first instinct, too. You can see it here:
ef91635

However, I ended up (#665 (comment)) changing that to the exception form because

adding the check argument to call() is awkward, because it looks a lot like the env and cwd arguments, that passthrough to subprocess.run, but there's a subtle difference - a different default.

adding the return type CompletedProcess to call() is also awkward, because it's only useful when check=False.

it muddles call()'s purpose, it's no longer 'do this' it's now 'do this/try this and tell me what happened'

So I thought the exception was neater overall.

Our entire wrapper is a bit bothersome - the important thing it does is add logging. It also happens to modify the interface just a tiny bit (different name, check=True). This is heavily because it was a shortcut for check_call, but we've standardized on run(check=True), which is the recommended method to use for 3.5+ code. So what about renaming the wrapper to utils.run, just passing **kwargs, and adding check=True for most of the uses? I'd rather make it look like something people are used to (subprocess.run), rather than save a few chars.

Now if it was dramatically better, ala plumbum, then it would be worth learning something new. But not for just a small interface change.

Could be updated later, of course, doesn't have to be in this PR.

Points 2 and 3 are just inherited from subprocess.run. The interface might not be ideal, but it's in the stdlib.

Would using:

call(["python", "-m", "ensurepip"], env=env, cwd="/tmp") call( ["python", "-m", "pip", "install", "--force-reinstall", "pip", *dependency_constraint_flags], env=env, cwd="/tmp", )

solve the issue of detecting if pip is installed in the first place ?

i.e. use --force-reinstall instead of --upgrade when installing the pinned version

Would that ensure the defaultpip script was there? I think so?

Given that an actual update would end-up in the place where we want to be and starts by removing the existing installation of pip, I would guess so. Let's try this. @joerick, I'll push a commit, please do revert it if you don't like what you're seeing :-)

I agree- if that works, I'm interested. Let's see.

for more information, see https://pre-commit.ci

henryiii

I'm happy if we can get rid of this internal pip copy! :)

joerick · 2021-05-14T13:55:34Z

Cool, that worked! I just want to check out test suite timings though, it occurs to me that this might be reinstalling pip on every test now, which could be adding some time that we could do without...

henryiii · 2021-05-15T02:12:25Z

Weren't we doing that before? The pinned pip was never the bundled pip.

joerick · 2021-05-15T15:55:50Z

Weren't we doing that before? The pinned pip was never the bundled pip.

True, but when running our integration tests, each test reuses the same environment, so once the pinned pip is installed, it doesn't get reinstalled on each test.

	master	`a116c2e` (uninstall)	`7cde8fb` (--force-reinstall)
GHA Linux	19m	24m	22m
GHA Mac	28m	20m	22m
GHA Windows	35m	36m	40m
Azure Mac	22m	18m	21m
Azure Windows 3.6	35m	37m	47m
Azure Windows 3.8	35m	47m	61m

There's quite a lot of variance in these figures, so I'm not totally sure, but it does look like an increase on windows... let me try adding back the check for installation_dir/bin/pip before doing the reinstall, maybe we'll see a decrease.

joerick · 2021-05-15T22:16:18Z

updated table with the last commit-

	master	`a116c2e` (uninstall)	`7cde8fb` (--force-reinstall)	`39302c7` (--force-reinstall with existence check)
GHA Linux	19m	24m	22m	23m
GHA Mac	28m	20m	22m	22m
GHA Windows	35m	36m	40m	36m
Azure Mac	22m	18m	21m	14m
Azure Windows 3.6	35m	37m	47m	34m
Azure Windows 3.8	35m	47m	61m	36m

joerick · 2021-05-15T22:18:37Z

Looks like it's a bit faster with a check around the --force-reinstall step. @mayeut does this look okay to you still?

mayeut · 2021-05-15T22:35:57Z

That looks promising this way.
I'd use the "if not installed" to set a flag variable to --force-reinstall with the default being --upgrade.
It would get rid of 1 call to pip install and avoid code duplication for all args but this one.
What do you think ?

joerick · 2021-05-16T07:32:20Z

Really nice idea 👍

mayeut · 2021-05-16T09:18:29Z

cibuildwheel/macos.py

+    # ensure pip is installed
+    call(["python", "-m", "ensurepip"], env=env, cwd="/tmp")
+
+    # upgrade to the version matching our constraints
+    # if necessary, reinstall it to ensure that it's installed as 'pip'
+    requires_reinstall = not (installation_bin_path / "pip").exists()


maybe move python -m ensurepip below requires_reinstall to get rid of this call as well ?

requires_reinstall = not (installation_bin_path / "pip").exists() if requires_reinstall: # ensure pip is installed call(["python", "-m", "ensurepip"], env=env, cwd="/tmp")

hmm. yeah, I guess it would be a little faster? I thought 'ensurepip' was a quick noop when it's installed, but it still takes around a second on my machine. I'll try your suggestion.

joerick · 2021-05-16T11:06:56Z

GHA having issues - https://www.githubstatus.com/

mayeut · 2021-05-16T19:30:21Z

updated table with the last commit-

	master	`a116c2e` (uninstall)	`7cde8fb` (--force-reinstall)	`39302c7` (--force-reinstall with existence check)	`fd94e77` (conditional ensurepip)
GHA Linux	19m	24m	22m	23m	24m
GHA Mac	28m	20m	22m	22m	21m
GHA Windows	35m	36m	40m	36m	32m
Azure Mac	22m	18m	21m	14m	14m
Azure Windows 3.6	35m	37m	47m	34m	41m
Azure Windows 3.8	35m	47m	61m	36m	34m

joerick · 2021-05-18T09:13:14Z

updated table with the last commits (no code changes)-

	master	`a116c2e` (uninstall)	`7cde8fb` (--force-reinstall)	`39302c7` (--force-reinstall with existence check)	`fd94e77` (conditional ensurepip)	`3a61a5a`	`9b48a41`
GHA Linux	19m	24m	22m	23m	24m	20m	20m
GHA Mac	28m	20m	22m	22m	21m	17m	26m
GHA Windows	35m	36m	40m	36m	32m	30m	29m
Azure Mac	22m	18m	21m	14m	14m	14m	16m
Azure Windows 3.6	35m	37m	47m	34m	41m	37m	45m
Azure Windows 3.8	35m	47m	61m	36m	34m	29m	46m

In summary, there might be too much variance in the Azure Windows figures to conclude anything, but the GHA WIndows ones seem fairly stable. Conversely the Mac figures are more stable on Azure!

Either way, this seems good to go 👍

joerick · 2021-05-18T12:01:07Z

thanks for spotting that license note henry!

joerick added 5 commits May 8, 2021 17:55

Remove get-pip and use ensurepip instead

62672c8

Try to force pip to install itself on PATH

add1a64

--yes

6007cdd

Run in the correct venv

7ffa9bb

Try a different method to find out if pip is installed

9034e09

joerick mentioned this pull request May 8, 2021

Plan for 2.0 #657

Closed

12 tasks

joerick added 3 commits May 8, 2021 19:20

Use the call() function

ef91635

Fix some typing issues

084e50b

Don't require pip!

0838df4

joerick commented May 9, 2021

View reviewed changes

cibuildwheel/windows.py Outdated Show resolved Hide resolved

joerick added 2 commits May 9, 2021 10:20

Tidy up

f5046a6

Merge remote-tracking branch 'origin/master' into ensurepip-2

b0f94c3

joerick added 2 commits May 9, 2021 10:42

Use the same mechanism on mac for ensuring default-pip

b5f8002

Fix paths

a116c2e

joerick marked this pull request as ready for review May 10, 2021 19:29

henryiii reviewed May 11, 2021

View reviewed changes

mayeut and others added 3 commits May 13, 2021 20:32

Force install pip

ee5dfc6

Force reinstall pip

36ef6de

[pre-commit.ci] auto fixes from pre-commit.com hooks

7cde8fb

for more information, see https://pre-commit.ci

henryiii mentioned this pull request May 13, 2021

refactor: use run-like syntax #672

Closed

henryiii approved these changes May 13, 2021

View reviewed changes

Try adding the existence check before reinstalling

39302c7

Apply @mayeut's clean up suggestion

9a07f00

mayeut reviewed May 16, 2021

View reviewed changes

Only call ensurepip if bin/pip wasn't found

fd94e77

joerick and others added 2 commits May 17, 2021 08:31

Empty commit to trigger another build

3a61a5a

docs: remove get-pip from LICENCE

9b48a41

joerick merged commit a167567 into master May 18, 2021

henryiii deleted the ensurepip-2 branch May 18, 2021 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try using ensurepip instead of get-pip #665

Try using ensurepip instead of get-pip #665

joerick commented May 8, 2021 •

edited

Loading

joerick commented May 9, 2021

YannickJadoul commented May 9, 2021

joerick commented May 10, 2021

henryiii May 11, 2021 •

edited

Loading

henryiii May 11, 2021

joerick May 13, 2021

henryiii May 13, 2021 •

edited

Loading

henryiii May 13, 2021

mayeut May 13, 2021 •

edited by henryiii

Loading

henryiii May 13, 2021

mayeut May 13, 2021

joerick May 13, 2021

henryiii left a comment

joerick commented May 14, 2021

henryiii commented May 15, 2021

joerick commented May 15, 2021 •

edited

Loading

joerick commented May 15, 2021

joerick commented May 15, 2021

mayeut commented May 15, 2021 •

edited

Loading

joerick commented May 16, 2021

mayeut May 16, 2021

joerick May 16, 2021

joerick commented May 16, 2021

mayeut commented May 16, 2021

joerick commented May 18, 2021 •

edited

Loading

joerick commented May 18, 2021

Try using ensurepip instead of get-pip #665

Try using ensurepip instead of get-pip #665

Conversation

joerick commented May 8, 2021 • edited Loading

joerick commented May 9, 2021

YannickJadoul commented May 9, 2021

joerick commented May 10, 2021

henryiii May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayeut May 13, 2021 • edited by henryiii Loading

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

henryiii left a comment

Choose a reason for hiding this comment

joerick commented May 14, 2021

henryiii commented May 15, 2021

joerick commented May 15, 2021 • edited Loading

joerick commented May 15, 2021

joerick commented May 15, 2021

mayeut commented May 15, 2021 • edited Loading

joerick commented May 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joerick commented May 16, 2021

mayeut commented May 16, 2021

joerick commented May 18, 2021 • edited Loading

joerick commented May 18, 2021

joerick commented May 8, 2021 •

edited

Loading

henryiii May 11, 2021 •

edited

Loading

henryiii May 13, 2021 •

edited

Loading

mayeut May 13, 2021 •

edited by henryiii

Loading

joerick commented May 15, 2021 •

edited

Loading

mayeut commented May 15, 2021 •

edited

Loading

joerick commented May 18, 2021 •

edited

Loading