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

Fix the protect-pip-on-windows logic #10560

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Oct 8, 2021

No idea how we missed this, but this conditional was flipped and, thus, wrong. >.<

Closes #7280
Closes #8247
Closes #8274
Closes #8450
Closes #9395
Closes #9527
Closes #9566
Closes #10014
Closes #10454
Closes #10505
Closes #10848

@pradyunsg pradyunsg added the !release blocker Hold a release until this is resolved label Oct 8, 2021
@pradyunsg pradyunsg added this to the 21.3 milestone Oct 8, 2021
@pradyunsg pradyunsg removed the !release blocker Hold a release until this is resolved label Oct 8, 2021
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

Good catch!

@pradyunsg
Copy link
Member Author

Okay, this doesn't strictly block the release, but we should really fit this in.

@pradyunsg
Copy link
Member Author

pradyunsg commented Oct 8, 2021

Here's is the failure case, as it happens on the current release: https://github.com/pradyunsg/testing-grounds/runs/3840831149

> pip install --upgrade pip --force-reinstall
Collecting pip
  Downloading pip-21.2.4-py3-none-any.whl (1.6 MB)
Installing collected packages: pip
  Attempting uninstall: pip
    Found existing installation: pip 21.2.4
    Uninstalling pip-21.2.4:
      Successfully uninstalled pip-21.2.4
ERROR: Could not install packages due to an OSError: [WinError 5] Access is denied: 'C:\\hostedtoolcache\\windows\\Python\\3.9.7\\x64\\~cripts\\pip.exe'
Consider using the `--user` option or check the permissions.

If someone with a Windows machine can confirm that this branch correctly rejects pip install pip --force-reinstall; that'd be great! (you'll need to install this branch in a virtualenv, and then run that command)

@pfmoore
Copy link
Member

pfmoore commented Oct 8, 2021

If someone with a Windows machine can confirm

Will do, but sorry - what's the correct incantation to install this branch?

@pradyunsg
Copy link
Member Author

python -m pip install https://github.com/pradyunsg/pip/archive/refs/heads/fix-protect-pip-on-windows.zip

@DiddiLeija
Copy link
Member

I will also try it, I have a Windows 10 machine ;)

@pradyunsg
Copy link
Member Author

pradyunsg commented Oct 8, 2021

Womp womp. The checks we have already seem to not work, because... at least on GitHub actions... pip install pip<=21.2.4 has the following as sys.argv[0].

C:\hostedtoolcache\windows\Python\3.9.7\x64\Scripts\pip

And we're checking for endswith pip.exe. :(

@DiddiLeija
Copy link
Member

DiddiLeija commented Oct 8, 2021

I installed pip on a fresh virtualenv and installed your pip branch. Then I tried these commands. This is my output:

  • pip install --upgrade pip --force-reinstall:
(my_venv) C:\Users\Diego Ramirez>pip install pip --force-reinstall
Collecting pip
  Using cached pip-21.2.4-py3-none-any.whl (1.6 MB)
Installing collected packages: pip
  Attempting uninstall: pip
    Found existing installation: pip 21.3.dev0
    Uninstalling pip-21.3.dev0:
      Successfully uninstalled pip-21.3.dev0
ERROR: Could not install packages due to an OSError: [WinError 5] Acceso denegado: 'C:\\Users\\Diego Ramirez\\AppData\\Local\\Temp\\pip-uninstall-m1tyyq18\\pip.exe'
Check the permissions.

Maybe I missed something?

@DiddiLeija
Copy link
Member

Womp womp. The checks we have already seem to not work, because... at least on GitHub actions... pip install pip<=21.2.4 has the following as sys.argv[0].

C:\hostedtoolcache\windows\Python\3.9.7\x64\Scripts\pip

Yup, I can also confirm that :(

@pfmoore
Copy link
Member

pfmoore commented Oct 8, 2021

I get the same result as @DiddiLeija. Looks like satisfied_by is None in my case...

@pradyunsg
Copy link
Member Author

Womp womp.

The answer seems to be that it's no longer pip.exe as sys.argv[0] but rather pip, which... is falling through the check for "is this running pip directly".

@pradyunsg pradyunsg removed this from the 21.3 milestone Oct 9, 2021
@pradyunsg pradyunsg marked this pull request as draft October 9, 2021 13:55
@pradyunsg
Copy link
Member Author

The answer seems to be that it's no longer pip.exe as sys.argv[0] but rather pip, which... is falling through the check for "is this running pip directly".

Well, let's just check for pip then. python -m pip gives us __main__.py as basename(sys.argv[0]).

@pradyunsg pradyunsg marked this pull request as ready for review February 18, 2022 18:29
@pradyunsg pradyunsg added this to the 22.1 milestone Feb 18, 2022
@DiddiLeija
Copy link
Member

Mypy isn't happy now 😅.

@DiddiLeija
Copy link
Member

Before you merge this, I want to check the behavior once again 😉.

@pradyunsg
Copy link
Member Author

pradyunsg commented Feb 18, 2022

Oh yea, totally cool with that!

There's also an automated test, FWIW, so if that fails, we know that this won't work. :)

@DiddiLeija
Copy link
Member

Ok, I tried pip install pip --force-reinstall, and failed with this message:

(test-win-pip) C:\Users\Diego Ramirez>pip install pip --force-reinstall
Collecting pip
  Using cached pip-22.0.3-py3-none-any.whl (2.1 MB)
ERROR: To modify pip, please run the following command:
C:\Users\Diego Ramirez\test-win-pip\Scripts\python.exe -m pip install pip --force-reinstall

I think that is what we wanted to do?

Also, I tried python -m pip install pip --force-reinstall. It works as expected:

(test-win-pip) C:\Users\Diego Ramirez>python -m pip install pip --force-reinstall
Collecting pip
  Using cached pip-22.0.3-py3-none-any.whl (2.1 MB)
Installing collected packages: pip
  Attempting uninstall: pip
    Found existing installation: pip 22.1.dev0
    Uninstalling pip-22.1.dev0:
      Successfully uninstalled pip-22.1.dev0
Successfully installed pip-22.0.3

@pradyunsg
Copy link
Member Author

Awesome!

Copy link
Member

@DiddiLeija DiddiLeija left a comment

Choose a reason for hiding this comment

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

The Windows (group 2) tests failed, it's a really simple thing.

>       assert "python.exe" in result.stderr, str(result)
E       AssertionError: Script result: pip install pip --force-reinstall
E           return code: 1
E         -- stderr: --------------------
E         ERROR: To modify pip, please run the following command:
E         D:\a\pip\pip\.nox\test-3-7\Scripts\python.EXE -m pip install pip --force-reinstall
E         
E         -- stdout: --------------------
E         Collecting pip
E           Downloading pip-22.0.3-py3-none-any.whl (2.1 MB)
E              ---------------------------------------- 2.1/2.1 MB 16.6 MB/s eta 0:00:00
E         
E       assert 'python.exe' in 'ERROR: To modify pip, please run the following command:\nD:\\a\\pip\\pip\\.nox\\test-3-7\\Scripts\\python.EXE -m pip install pip --force-reinstall\n'
E        +  where 'ERROR: To modify pip, please run the following command:\nD:\\a\\pip\\pip\\.nox\\test-3-7\\Scripts\\python.EXE -m pip

Some Windows machines (like the one that ran this test) have python.EXE, not python.exe.

@pradyunsg
Copy link
Member Author

pradyunsg commented Feb 25, 2022

And, now it's #10922 that's necessary for fixing the CI. :)

@pradyunsg pradyunsg added the type: bug A confirmed bug or unintended behavior label Feb 25, 2022
pradyunsg and others added 3 commits February 26, 2022 12:32
The names for the executables does not contain a `.exe` suffix anymore.
@pradyunsg pradyunsg modified the milestones: 22.1, 22.0.4 Feb 26, 2022
@pradyunsg pradyunsg modified the milestones: 22.0.4, 22.1 Feb 26, 2022
Copy link
Member

@DiddiLeija DiddiLeija left a comment

Choose a reason for hiding this comment

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

At last, this works! Thanks @pradyunsg!

@pradyunsg
Copy link
Member Author

Merging this on the basis of a bunch of manual tests by myself and @DiddiLeija! If there's any concerns, please feel free to @ mention me here. :)

@pradyunsg pradyunsg merged commit 25861e5 into pypa:main Mar 18, 2022
@pradyunsg pradyunsg deleted the fix-protect-pip-on-windows branch March 18, 2022 12:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2022
@pypa pypa unlocked this conversation May 27, 2022
@pypa pypa locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.