Check stderr first before stdout on VCS Install #9234
Conversation
When working with large amounts of data, Git reports on stderr instead of stdout. For some reason, on Git for Windows (I have not been able to reproduce this on Linux), this can cause the subprocess to completely stall while asking for a return from the stdout. In the context of a `pip install git+https://`, this results in the Clone step freezing, without providing any errors or context about what's happening (or what has gone wrong). This fix circumvents that by first check stderr for output, and then checking stdout (if none is found).
|
Can this be done simpler with |
uranusjr
added
C: vcs
I'm not sure if that would address the Git issue, as it may effectively resolve to the same thing as the current solution. I'll try it though, as it would definitely make for a cleaner change. |
When working with large amounts of data, Git reports on stderr instead of stdout. For some reason, on Git for Windows (I have not been able to reproduce this on Linux), this can cause the subprocess to completely stall while asking for a return from the stdout. In the context of a `pip install git+https://`, this results in the Clone step freezing, without providing any errors or context about what's happening (or what has gone wrong).
Pipe stderr to stdout
That was a good suggestion. It works on my Windows environment with a Git install. Updated pull request with the change. |
|
FYI - automated checks are failing, but that's happening when I pull down master and run it on my local as well. |
|
Hmm, reading the code again, |
|
@uranusjr I have to admit that I don't have enough experience with subprocess to know whether or not this is a good idea. If we reverted to my first suggestion, and used |
|
So in the original implementation, only contents from stdout would be added to But reading the original implementation yet again, stderr was piped but never read in any way before being closed. So I’m going to assume it’s actually a bug, and this reflects the original author’s intention. We can always fix it if we’re wrong 🙂 |
Co-authored-by: Tzu-ping Chung <[email protected]>
Oh gotcha. So even if we capture details that we don't intend to from |
| @@ -121,7 +121,7 @@ def call_subprocess( | |||
| # Convert HiddenText objects to the underlying str. | |||
| reveal_command_args(cmd), | |||
| stdout=subprocess.PIPE, | |||
| stderr=subprocess.PIPE, | |||
| stderr=subprocess.STDOUT, | |||
There was a problem hiding this comment.
I'm not sure. This bug was introduced in #7969 which was intended to not merge stdout and stderr, because some VCS command log warnings on stderr and we don't want to capture them in the command output. So I'd say we should leave stderr alone to be printed on the console for the user to see.
Actually I was wondering this week why I was seeing pip failing on git exit codes while not showing the error details. That is probably it.
There was a problem hiding this comment.
@Mikuana could you check if simply removing this stderr=subprocess.STDOUT, line works ?
There was a problem hiding this comment.
@sbidoul that worked as well. I've updated this PR to remove that line instead of piping it to stdout.
There was a problem hiding this comment.
Unfortunately tests are red. It's because some calls (one actually, via get_repository_root) need to capture stderr.
There was a problem hiding this comment.
What would your recommendation be? Should we modify the tests, or should we trust the test and instead redirect the stderr to stdout as I had it previously?
Sorry if that seems like a silly question.
There was a problem hiding this comment.
Redirecting stderr to stdout won't work because that would reopen #7545 and #7968 where the vcs logs warnings which get mixed with the stdout we want to extract and parse.
If we let stderr go to the console, this will create unwanted noise on the console (which is why the tests fail), and bypass the pip logging and verbosity control mechanisms.
So what can we do? Not a silly question indeed.
I see two approach.
1/ The easy one is to use Popen.communicate() which has a safe (multithreaded) mechanism to capture stderr and stdout separately. There are two logging-related drawbacks to this: a) in debug mode it would not display the process output until it has terminated b) stdout and stderr could only be showed one after the other instead of the natural line order produced by the subprocess.
2/ The hard one is to reimplement a variant of communicate to both debug log and capture (a kind of tee)...
There was a problem hiding this comment.
So I went for approach 1/ in #9327
This feature may have been the root cause in the introduction of hanging git installs in 20.2.
Remove stderr subprocess
|
Closing in favor of #9327 |
When working with large amounts of data, Git reports on stderr instead of
stdout. For some reason, on Git for Windows (I have not been able to reproduce
this on Linux), this can cause the subprocess to completely stall while asking
for a return from the stdout. In the context of a
pip install git+https://,this results in the Clone step freezing, without providing any errors or
context about what's happening (or what has gone wrong).
This fix circumvents that by first check stderr for output, and then checking
stdout (if none is found).