Re-think error swallowing for de-vendored packages

[sk1p]

• Pip version: 9.0.1 (should still apply to pip master)
• Python version: 3.6
• Operating system: Debian Linux

Description:

Currently, in pip._vendor, if DEBUNDLED = True and both the real and the vendored modules don't import, the ImportError is silently swallowed. Now consider the case that the requested module does exist (example: requests) but is missing a dependency (idna). That leads to a situation where an import of pip._vendor.requests raises ImportError: cannot import name 'requests', without any clue about the original error. Example backtrace (when creating a virtualenv):

Installing setuptools, pkg_resources, pip, wheel...
  Complete output from command /tmp/bleh3/bin/python3 - setuptools pkg_resources pip wheel:
  Traceback (most recent call last):
  File "<stdin>", line 7, in <module>
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/__init__.py", line 31, in <module>
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/vcs/mercurial.py", line 9, in <module>
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/download.py", line 40, in <module>
ImportError: cannot import name 'requests'
----------------------------------------
...Installing setuptools, pkg_resources, pip, wheel...done.
Traceback (most recent call last):
  File "/usr/bin/virtualenv", line 11, in <module>
    load_entry_point('virtualenv==15.1.0', 'console_scripts', 'virtualenv')()
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 724, in main
    symlink=options.symlink)
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 992, in create_environment
    download=download,
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 922, in install_wheel
    call_subprocess(cmd, show_stdout=False, extra_env=env, stdin=SCRIPT)
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 817, in call_subprocess
    % (cmd_desc, proc.returncode))
OSError: Command /tmp/bleh3/bin/python3 - setuptools pkg_resources pip wheel failed with error code 1

By explicitly raising the original exception instead of swallowing it, the following error is shown instead:

Installing setuptools, pkg_resources, pip, wheel...
  Complete output from command /tmp/bleh3/bin/python3 - setuptools pkg_resources pip wheel:
  Traceback (most recent call last):
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/_vendor/__init__.py", line 33, in vendored
ModuleNotFoundError: No module named 'pip._vendor.cachecontrol'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 7, in <module>
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/__init__.py", line 28, in <module>
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/exceptions.py", line 6, in <module>
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/_vendor/__init__.py", line 64, in <module>
  File "/usr/share/python-wheels/pip-9.0.1-py2.py3-none-any.whl/pip/_vendor/__init__.py", line 36, in vendored
  File "/tmp/bleh3/share/python-wheels/CacheControl-0.11.7-py2.py3-none-any.whl/cachecontrol/__init__.py", line 9, in <module>
  File "/tmp/bleh3/share/python-wheels/CacheControl-0.11.7-py2.py3-none-any.whl/cachecontrol/wrapper.py", line 1, in <module>
  File "/tmp/bleh3/share/python-wheels/CacheControl-0.11.7-py2.py3-none-any.whl/cachecontrol/adapter.py", line 4, in <module>
  File "/tmp/bleh3/share/python-wheels/requests-2.18.4-py2.py3-none-any.whl/requests/__init__.py", line 98, in <module>
  File "/tmp/bleh3/share/python-wheels/requests-2.18.4-py2.py3-none-any.whl/requests/packages.py", line 7, in <module>
ModuleNotFoundError: No module named 'idna'
----------------------------------------
...Installing setuptools, pkg_resources, pip, wheel...done.
Traceback (most recent call last):
  File "/usr/bin/virtualenv", line 11, in <module>
    load_entry_point('virtualenv==15.1.0', 'console_scripts', 'virtualenv')()
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 724, in main
    symlink=options.symlink)
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 992, in create_environment
    download=download,
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 922, in install_wheel
    call_subprocess(cmd, show_stdout=False, extra_env=env, stdin=SCRIPT)
  File "/usr/lib/python3/dist-packages/virtualenv.py", line 817, in call_subprocess
    % (cmd_desc, proc.returncode))
OSError: Command /tmp/bleh3/bin/python3 - setuptools pkg_resources pip wheel failed with error code 1

See also: debian bugs 896998 and 897121

[pradyunsg]

I won't mind us doing this -- I'd like other downstream redistributors to pitch in here since this likely affects them more than vanilla pip.

[pradyunsg]

Fedora: @hroncok @torsava @stratakis

[pradyunsg]

@sk1p Do you happen to know about Debian side of things for https://github.com/pypa/pip/issues/5346#issuecomment-388046352?

[hroncok]

This should be fine for Fedora.

[stratakis]

That seems a quite reasonable change.

[pradyunsg]

Happy to accept a PR for this. :)

[sk1p]

@pradyunsg sorry, not really.

[pradyunsg]

The change we want to make here is update src/pip/_vendor/__init__.py (here) to something like:

except ImportError:
    # This error used to be silenced in earlier variants of this file, to instead 
    # raise the error when pip actually tries to use the missing module.
    # Based on inputs in #5354, this was changed to explicitly raise the error.
    # Re-raising the exception without modifying it is an intentional choice.
    raise

This would require a .trivial NEWS entry (https://pip.pypa.io/en/latest/development/#adding-a-news-entry) in the PR.

[pradyunsg]

This issue is a good starting point for anyone who wants to help out with pip's development -- it's simple and the process of fixing this should be a good introduction to pip's development workflow. See the discussion above to understand what the desired fix is.

[gutsytechster]

@pradyunsg, I'd like to take up the issue. :)

[pradyunsg]

@gutsytechster Go ahead!

[rohitsanj]

took this one home, it was a trivial fix

[gutsytechster]

@rohitsanj please inform earlier if you already have started working over the issue. It would be inconvenient for two people working on the same issue without knowing.

Although, this was not a major fix, this kind of conflict may arise across other issues. So just let others know that you are already working over the issue.

Thanks. :)

[McSinyx]

@pradyunsg, could you please elaborate on why re-raising the error is a better idea comparing to

    def vendored(modulename):
        # (explanation here)
        __import__(modulename, globals(), locals(), level=0)
        vendored_name = "{0}.{1}".format(__name__, modulename)
        sys.modules[vendored_name] = sys.modules[modulename]
        base, head = vendored_name.rsplit(".", 1)
        setattr(sys.modules[base], head, sys.modules[modulename])

which AFAIK gives the same behavior.

[deveshks]

Also the previous PR https://github.com/pypa/pip/pull/7790 linked to this issue has been closed without merging, so if we still want to re-raise the error, I can open a new one.

Also If I understand correctly @McSinyx , you want to propagate all errors coming from __import__ directly instead, of catching only ImportError and raising it to propagate it further, which @pradyunsg suggested.

[McSinyx]

catching only ImportError and raising it to propagate it further

I don't get the further part here, i.e.

$ cat e.py
import foobar
$ python e.py
Traceback (most recent call last):
  File "e.py", line 1, in <module>
    import foobar
ImportError: No module named foobar
$ cat r.py 
try:
    import foobar
except ImportError:
    raise
$ python r.py 
Traceback (most recent call last):
  File "r.py", line 2, in <module>
    import foobar
ImportError: No module named foobar

the behaviors are exactly the same, unless I'm missing something here. AFAIK reraising an exception is usually used when there is some check beyond the type of the exception, e.g.

try:
    import foobar
except ImportError as e:
    if 'foobar' in str(e): raise

[deveshks]

@McSinyx I was actually referring to propagating the error up the function call stack.

I was saying that if __import__ doesn't raise any other Exception apart from ImportError, both the behaviours isareexactly the same, but if there is other exception which __import__ throws, then the behaviours will be different.

[McSinyx]

if there is other exception which import throws, then the behaviours will be different.

I don't think so:

>>> def foo():
...     try: bar()
...     except IndexError: raise
... 
>>> def bar(): 1/0
... 
>>> foo()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in foo
  File "<stdin>", line 1, in bar
ZeroDivisionError: division by zero
>>> def bar(): [][0]
... 
>>> foo()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in foo
  File "<stdin>", line 1, in bar
IndexError: list index out of range

I can't find any reference describing the exact behavior other than this, but raise seem to exactly reraise the exception as if the try clause doesn't exist.

[deveshks]

Aah you are correct @McSinyx . Appreciate the detailed explanation :)

[NeilBotelho]

@pradyunsg Hi can I take this up? It looks like the last person who was working on it closed his pull request

[pradyunsg]

could you please elaborate on why re-raising the error is a better idea

To provide the historical context of these changes. Most of this file exists as a result of discussions between a lot of groups, so actively providing context in-place is more important than reducing the lines of code/indentation IMO. :)

@NeilBotelho Go ahead! The required change is described here: https://github.com/pypa/pip/issues/5354#issuecomment-581107760

[deveshks]

This issue is fixed by https://github.com/pypa/pip/pull/8391 and can be closed.

[sk1p]

Thanks!

[felixonmars]

This change seems to make optional dependencies of the devendored library hard dependencies of pip, in devendered installation:

Traceback (most recent call last):
  File "pip_sphinxext.py", line 11, in <module>
    from pip._internal.cli import cmdoptions
  File "/build/python-pip/src/pip-20.2/src/pip/_internal/cli/cmdoptions.py", line 23, in <module>
    from pip._internal.cli.progress_bars import BAR_TYPES
  File "/build/python-pip/src/pip-20.2/src/pip/_internal/cli/progress_bars.py", line 7, in <module>
    from pip._vendor import six
  File "/build/python-pip/src/pip-20.2/src/pip/_vendor/__init__.py", line 83, in <module>
    vendored("requests.packages.urllib3.contrib.ntlmpool")
  File "/build/python-pip/src/pip-20.2/src/pip/_vendor/__init__.py", line 33, in vendored
    __import__(modulename, globals(), locals(), level=0)
  File "/usr/lib/python3.8/site-packages/urllib3/contrib/ntlmpool.py", line 9, in <module>
    from ntlm import ntlm
ModuleNotFoundError: No module named 'ntlm'

Is this intended?