runpy should check ImportError.name before wrapping it

[ncoghlan]

BPO	19771
Nosy	@ncoghlan, @bitdancer, @ericsnowcurrently, @berkerpeksag, @vadmium, @poleto
Dependencies	bpo-14285: Traceback wrong on ImportError while executing a package
Files	issue_19771_tests.patch: Test cases for this issue. issue_19771.patch issue_19771.patch.v2: Updated patch based on review. issue_19771_runpy.patch.v3: Tests + sys.modules inspection fix issue_19771_runpy.patch.v4 bad-pyc.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-12-12.07:49:09.022>
created_at = <Date 2013-11-25.12:39:47.093>
labels = ['easy', 'type-bug', 'library']
title = 'runpy should check ImportError.name before wrapping it'
updated_at = <Date 2015-12-19.01:48:27.562>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2015-12-19.01:48:27.562>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2015-12-12.07:49:09.022>
closer = 'martin.panter'
components = ['Library (Lib)']
creation = <Date 2013-11-25.12:39:47.093>
creator = 'ncoghlan'
dependencies = ['14285']
files = ['34988', '34989', '34995', '38437', '40150', '41282']
hgrepos = []
issue_num = 19771
keywords = ['patch', 'easy']
message_count = 18.0
messages = ['204334', '205617', '205636', '216934', '216935', '216951', '237829', '237848', '248193', '248241', '255691', '256194', '256195', '256196', '256206', '256268', '256693', '256717']
nosy_count = 11.0
nosy_names = ['ncoghlan', 'r.david.murray', 'SilentGhost', 'python-dev', 'eric.snow', 'Anthony.Kong', 'berker.peksag', 'martin.panter', 'Fotis.Koutoulakis', 'luiz.poleto', 'lac']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19771'
versions = ['Python 3.5', 'Python 3.6']

[ncoghlan]

bpo-19769 shows that if __main__ in a package throws ImportError, runpy will incorrectly report the package as not being directly executable (when it actually claims to be executable, it's just broken)

This can be fixed in 3.3+ by checking for an appropriate value in the name attribute of the caught exception, and only wrapping it if the failed lookup was for the __main__ submodule we're looking for.

The associated test can just use a __main__.py that deliberately raises ImportError.

[FotisKoutoulakis]

Hello, I'm in the process of trying to find a solution to this problem, but I'm afraid the choice of wording at some point is kind of...ambiguous.

I have some questions I want to ask, to clear some doubts in my head:

First one is:

"if __main__ in a package throws ImportError, runpy will incorrectly report the package as not being directly executable (when it actually claims to be executable, it's just broken)"

In the above, from what I can understand from the first part of the sentence, before the paragraph, if __main__.py is not found in a package, run py will report that it won't be directly executable. Along these lines, it's mentioned that this behaviour is incorrect, but then the sentence inside the parentheses contradicts that position, suggesting that when it does suggest it's actually executable, it's just erroneous behaviour (so both behaviours are erroneous?)

The second one is:

"This can be fixed in 3.3+ by checking for an appropriate value in the name attribute of the caught exception, and only wrapping it if the failed lookup was for the __main__ submodule we're looking for."

Ok this seems to be pretty clear, we are expected to create a new package with a main.py that all it does is raise ImportError. (Disclaimer: I may be fairly wrong here). Question is, where should we put the new package? Obviously, we shouldn't litter the "real" modules, with the testing one, but if I try to use the test folder as a package, I get "ImportError: error while finding loader for xxx"

Please be lenient with me, this is one of my very first contributions to python. I will gladly accept all feedback.

[ncoghlan]

By providing a __main__.py submodule, a package is saying "I am directly executable". When runpy says it isn't (because running that file happened to raise ImportError), then runpy is wrong.

runpy can't make the file work (it's genuinely broken), but it can avoid reporting a misleading error message - that's the bug to be fixed.

As far as the testing goes, if you look at the existing runpy tests, the usual approach is to create a temporary directory, write out a package with the desired behaviour, and then run it from there. So, for this test, it's a matter of copying the general structure of the existing package execution test in test_runpy, but replacing the body of the __main__.py file with something like "raise ImportError('This should not be replaced')"

[poleto]

The attached patch provide test cases to validate this error. As noted by R. David Murray in a discussion in the Core-Mentorship list, this error in fact happens then __init__.py throws an ImportError.

[poleto]

As suggested by Nick, the fix is done be verifying the name attribute of the raised ImportError exception; the exception is then re-raised with the appropriate description.

[bitdancer]

Hmm. It seems to me that .name not being set is a bug in importlib. It appears that importlib doesn't set it in the 'from x import y' case. After a bit of experimenting at the python prompt, I'm not even sure what that code in runpy is *doing* (find_spec('foo.__main__') seems to return None if __main__ doesn't exist but foo does). I'm going to have to leave this to Nick unless I find some more time somewhere ;)

[vadmium]

Closely related: bpo-14285, where an ImportError and various other exceptions caused by code in __init__.py are also incorrectly caught, trigger unexpected error messages, with no traceback being reported.

Not sure if checking the “name” attribute will help in even all of the most common cases, but it should be better than nothing. Another idea I had was checking if a module was inserted into sys.modules.

[vadmium]

Posting a new patch with the following changes:

• Added Poleto’s original tests, updated according to review comments. Combined the flag and source code string parameters.

• Used a different approach to guessing where the offending ImportError came from. Now it checks if the module got added to sys.modules, which seems to work in all cases I tried. It no longer does what the bug title says (check ImportError.name), but I think it is a better workaround or fix for the underlying problem.

• Removed Poleto’s “Error executing package . . .” ImportError wrapper. We already know what package we are trying to execute, and it hasn’t actually been “executed” yet, because __init__.py failed.

• Cleaned up exception name in the “Error while finding spec” message

[lac]

I tried to run some tests from the python3.4 test suite and got:
python3 -m test -ugui test_tk test_ttk_guionly test_idle
/usr/bin/python3: Error while finding spec for 'test.__main__' (<class 'ImportError'>: bad magic number in 'test': b'\x03\xf3\r\n'); 'test' is a package and cannot be directly executed

The actual problem has nothing to do with test being a package. It
had to do with finding a python2 pyc and trying to run that. It would be nice if the patch checked for this problem as well before concluding that somebody just tried to execute a package.

[vadmium]

Patch v4 just uses an explicit encoding in the test suite rather than relying on the environment’s encoding.

Laura: I think my patch may already handle your case. Assuming this is one way to reproduce it:

$ mkdir test
$ : > test/__init__.py
$ python2 -c 'import test'
$ rm test/__init__.py
$ ./python -m test
/media/disk/home/proj/python/cpython/python: Error while finding spec for 'test.__main__' (<class 'ImportError'>: bad magic number in 'test': b'\x03\xf3\r\n'); 'test' is a package and cannot be directly executed
[Exit 1]
$ hg qpush --move issue_19771_runpy.patch.v4
applying issue_19771_runpy.patch.v4
now at: issue_19771_runpy.patch.v4
$ ./python -m test
/media/disk/home/proj/python/cpython/python: Error while finding spec for 'test.__main__' (ImportError: bad magic number in 'test': b'\x03\xf3\r\n')
[Exit 1]

Perhaps the error could be improved further, though. I guess the problem is there is a bad magic number in test/init.pyc. And I have no idea what the “finding spec” bit is about; it is meaningless noise to me.

[vadmium]

My new patch for bpo-14285 should avoid the main problem. However there would still be at least one leftover minor fix worth appyling: fix the exception message to use type(ex).__name__, not repr(type(ex)).

[vadmium]

I think I have fixed the original problem via bpo-14285 (fully in 3.5+, partly in 2.7). It involved a bit more than checking ImportError.name, because runpy catches other exceptions like AttributeError, which do not identify a module name.

That leaves Laura’s error with the bad *.pyc file. I propose bad-pyc.patch, which adds the sys.modules check to fix the error message, and adds a test case. I think this patch should be applicable to Python 2 as well.