-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
f171ebc
to
89dfcfe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Okay, this doesn't strictly block the release, but we should really fit this in. |
Here's is the failure case, as it happens on the current release: https://github.com/pradyunsg/testing-grounds/runs/3840831149
If someone with a Windows machine can confirm that this branch correctly rejects |
89dfcfe
to
4be3224
Compare
Will do, but sorry - what's the correct incantation to install this branch? |
|
I will also try it, I have a Windows 10 machine ;) |
Womp womp. The checks we have already seem to not work, because... at least on GitHub actions...
And we're checking for endswith |
I installed
Maybe I missed something? |
Yup, I can also confirm that :( |
I get the same result as @DiddiLeija. Looks like |
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". |
e0999ca
to
0696daf
Compare
Well, let's just check for pip then. |
Mypy isn't happy now 😅. |
0696daf
to
912319d
Compare
Before you merge this, I want to check the behavior once again 😉. |
Oh yea, totally cool with that! There's also an automated test, FWIW, so if that fails, we know that this won't work. :) |
Ok, I tried
I think that is what we wanted to do? Also, I tried
|
Awesome! |
There was a problem hiding this 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
.
912319d
to
d84fd07
Compare
And, now it's #10922 that's necessary for fixing the CI. :) |
The names for the executables does not contain a `.exe` suffix anymore.
d84fd07
to
afda756
Compare
There was a problem hiding this 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!
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. :) |
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