Refactor call_subprocess() to not raise InstallationError

[uranusjr]

What's the problem this feature will solve?

As mentioned in #7593 (comment), call_subprocess() currently raises InstallationError, but that function is used in a lot of places, many of them are not really related to installing things.

Its design is also limiting if the caller wants to get information other than merged stdout/stderr output.

Describe the solution you'd like

Introduce a wrapper call_subprocess_for_install() for existing usages that work the same as the current call_subprocess, and change most of the current call_subprocess invocations to use that instead. No changes are made except the name, since there are far too many invocations relying on the implicit InstallationError for flow control.

call_subprocess_for_install() would delegate most of its work to a refactored call_subprocess(), which would return a composite object, similar to subprocess.CompletedProcess introduced in Python 3.5, to hold information of the subprocess result. Callers expecting the subprocess to not return 0 can call this function instead, and check the return code and stderr without needing to catch InstallationError.

Alternative Solutions

• Do nothing, and continue to catch InstallationError. Not ideal, but I am actually OK with that TBH.
• Keep call_subprocess calls in tack, but raise something else instead. This would affect all usages that rely on the implicit InstallationError. This feels risky for me since there are a lot of indirect usages, and it would be a pain to hunt down all of them and wrap the call in a raise InstallationError.
• Accept call_subprocess is only for installing things, and invent something else under other contexts. This is also a good solution IMO, and since most call_subprocess usages are in the installation context, it’s not nearly as difficult to hunt them down an invoke subprocess.Popen instead. But it would still be a good idea IMO to change the name of call_subprocess, and if that’s the case, why not make its internal useful for other situations.

[pradyunsg]

Looking at the usage of call_subprocess, the areas it is used in are:

$ grep "call_subprocess(" -r ./src/pip/_internal
./src/pip/_internal/utils/subprocess.py:def call_subprocess(
./src/pip/_internal/utils/subprocess.py:            call_subprocess(
./src/pip/_internal/operations/install/editable_legacy.py:            call_subprocess(
./src/pip/_internal/operations/build/wheel_legacy.py:            output = call_subprocess(
./src/pip/_internal/operations/build/metadata_legacy.py:        call_subprocess(
./src/pip/_internal/vcs/versioncontrol.py:    # Iterable of environment variable names to pass to call_subprocess().
./src/pip/_internal/vcs/versioncontrol.py:            return call_subprocess(cmd, show_stdout, cwd,
./src/pip/_internal/vcs/subversion.py:        # call_subprocess(), pip must pass --force-interactive to ensure
./src/pip/_internal/wheel_builder.py:        call_subprocess(clean_args, cwd=req.source_dir)
./src/pip/_internal/build_env.py:            call_subprocess(args, spinner=spinner)

The sections which are calling this that aren't "install" stuff, are in "build" and "vcs" -- both of which are potentially reachable in a pip install run.

I feel like the best approach here is to add a SubprocessError, that directly inherits from PipError, and change/wrap all exceptions raised through call_subprocess with it. (i.e. alternative 2) The main reason for this is that these aren't a lot of these call sites, so it should be viable to identify where errors from these call sites would get caught, and augment them to also handle SubprocessError.

If we want to tackle this incrementally, we could start by adding a SubprocessError that inherits from InstallationError. Then, once we've augmented all the call-site-catch-points, we can change the base class for the newly introduced exception.

[uranusjr]

If we always catch SubprocessError at the call site (and re-raise InstallationError when appropriate), the end result wouldn’t be to dissimilar to the call_subprocess() + call_subprocess_for_install() proposal, but inlining the latter instead of making a separate function. The downside though is we still don’t have a way to get separate stdout and stderr channels, which would be useful for solving problems like #7545 in the long term.

[swchoi727]

i can try taking a stab at this

[uranusjr]

@swchoi727 Go ahead!

[swchoi727]

@uranusjr , created a PR here - #7776