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.

Sign up for GitHub

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

Check stderr first before stdout on VCS Install #9234

Closed
wants to merge 6 commits into from
Closed

Check stderr first before stdout on VCS Install #9234

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
66a4f9c
Check stderr first before stdout
Mikuana Dec 6, 2020
1d86daa
Pipe stderr to stdout
Mikuana Dec 8, 2020
56397ea
Merge pull request #1 from Mikuana/large-git-install-bugfix-alt
Mikuana Dec 8, 2020
f8aefd1
Update news/8876.bugfix.rst
Mikuana Dec 10, 2020
d6b06a5
Remove stderr subprocess capture in VCS install
Mikuana Dec 17, 2020
cf436e6
Merge pull request #2 from Mikuana/large-git-install-bugfix-rm-stderr
Mikuana Dec 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/8876.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Detect both stdout and stderr on VCS install VCS install to avoid process freeze with Git.
2 changes: 1 addition & 1 deletion src/pip/_internal/vcs/versioncontrol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@Mikuana could you check if simply removing this stderr=subprocess.STDOUT, line works ?

Copy link
Author

Choose a reason for hiding this comment

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

@sbidoul that worked as well. I've updated this PR to remove that line instead of piping it to stdout.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately tests are red. It's because some calls (one actually, via get_repository_root) need to capture stderr.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)...

Copy link
Member

Choose a reason for hiding this comment

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

So I went for approach 1/ in #9327

cwd=cwd
)
if proc.stdin:
Expand Down