-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Revert #7969 and fix VCS stdout/stderr capture #9331
Conversation
Wondering why these tests fail so early. They work on my machine™️. |
proc.stdin.close() | ||
if not stdout_only: | ||
assert proc.stdin | ||
proc.stdin.close() |
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.
Why not
if proc.stdin:
proc.stdin.close()
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.
@uranusjr I did something a little bit different in the last version. I don't quite understand why we need to pass a PIPE to stdin while we don't use it, but I don't dare to change it, and I decided to keep passing one in both modes.
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.
I think the intent is to disallow all stdin from subprocesses. Without it, the subprocess’s stdin would be inherited from pip’s, i.e. subprocesses are able to show prompts etc.
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.
More fun is that it waits in CI for prompts, instead dof outright failing as well.
d227635
to
de11340
Compare
Confirmed this solution appears to fix #8876 |
1923ee8
to
8dff97c
Compare
@pradyunsg how do you want to proceed here ? The python 2 cleanup is creating conflicts but this PR has to go in 20.3.4. BTW wasn't there a bot warning about merge conflicts ? |
I broke the bot, and then my laptop so, now, I can't fix the bot. :) |
@sbidoul If you (1) rebase this on top of 20.3.3, (2) create a merge commit against the current master branch, fixing conflicts and (3) force-push that. Then, it'll be possible to have an "easy backport" of this PR -- since it's based off 20.3.3 and hence won't be difficult to cherry-pick for release, and would also be a part of 21.0, since, well, it'll be merged into |
8dff97c
to
b3d348d
Compare
@pradyunsg ok, I did that and created the merge commit by merging the master branch into my branch. |
I assign this to 21.0, as the backport to 20.3.4 is tracked via #8876 |
Fixes #8876
Supersedes #9234 and #9327
Reverts #7969
Better fix for #7545 and #7968
Reopens #7841 (which pip can't fix as it is git output) and #7711 (which is not entirely trivial and can be handled in another PR)