Re-think error swallowing for de-vendored packages #5354
Comments
|
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. |
|
Fedora: @hroncok @torsava @stratakis |
|
@sk1p Do you happen to know about Debian side of things for #5346 (comment)? |
|
This should be fine for Fedora. |
|
That seems a quite reasonable change. |
|
Happy to accept a PR for this. :) |
pradyunsg
added
type: enhancement
|
@pradyunsg sorry, not really. |
|
The change we want to make here is update 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.
raiseThis would require a |
pradyunsg
added
good first issue
|
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. |
|
@pradyunsg, I'd like to take up the issue. :) |
|
@gutsytechster Go ahead! |
|
took this one home, it was a trivial fix |
|
@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. :) |
|
@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. |
|
Also the previous PR #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 |
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 foobarthe 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 |
|
@McSinyx I was actually referring to propagating the error up the function call stack. I was saying that if |
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 rangeI can't find any reference describing the exact behavior other than this, but |
|
Aah you are correct @McSinyx . Appreciate the detailed explanation :) |
|
@pradyunsg Hi can I take this up? It looks like the last person who was working on it closed his pull request |
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: #5354 (comment) |
Re-think error swallowing for de-vendored packages #5354
|
This issue is fixed by #8391 and can be closed. |
|
Thanks! |
|
This change seems to make optional dependencies of the devendored library hard dependencies of pip, in devendered installation: Is this intended? |
Description:
Currently, in
pip._vendor, ifDEBUNDLED = Trueand both the real and the vendored modules don't import, theImportErroris 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 ofpip._vendor.requestsraisesImportError: cannot import name 'requests', without any clue about the original error. Example backtrace (when creating a virtualenv):By explicitly raising the original exception instead of swallowing it, the following error is shown instead:
See also: debian bugs 896998 and 897121
The text was updated successfully, but these errors were encountered: