classification
Title: runpy should check ImportError.name before wrapping it
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: 14285 Superseder:
Assigned To: Nosy List: Anthony.Kong, Fotis.Koutoulakis, SilentGhost, berker.peksag, eric.snow, lac, luiz.poleto, martin.panter, ncoghlan, python-dev, r.david.murray
Priority: normal Keywords: easy, patch

Created on 2013-11-25 12:39 by ncoghlan, last changed 2015-12-19 01:48 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
issue_19771_tests.patch luiz.poleto, 2014-04-21 02:18 Test cases for this issue. review
issue_19771.patch luiz.poleto, 2014-04-21 02:36 review
issue_19771.patch.v2 luiz.poleto, 2014-04-21 21:39 Updated patch based on review. review
issue_19771_runpy.patch.v3 martin.panter, 2015-03-11 08:13 Tests + sys.modules inspection fix review
issue_19771_runpy.patch.v4 martin.panter, 2015-08-08 01:43 review
bad-pyc.patch martin.panter, 2015-12-11 05:03 review
Messages (18)
msg204334 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-11-25 12:39
Issue 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.
msg205617 - (view) Author: Fotis Koutoulakis (Fotis.Koutoulakis) * Date: 2013-12-08 23:09
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.
msg205636 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-09 01:26
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')"
msg216934 - (view) Author: Luiz Poleto (luiz.poleto) * Date: 2014-04-21 02:18
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.
msg216935 - (view) Author: Luiz Poleto (luiz.poleto) * Date: 2014-04-21 02:36
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.
msg216951 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-04-21 17:00
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 ;)
msg237829 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-11 00:37
Closely related: Issue 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.
msg237848 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-11 08:13
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
msg248193 - (view) Author: Laura Creighton (lac) Date: 2015-08-07 14:06
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.
msg248241 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-08-08 01:43
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.
msg255691 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-02 03:53
My new patch for Issue 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)).
msg256194 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-11 05:00
I think I have fixed the original problem via Issue 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.
msg256195 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-11 05:03
BTW I already applied the type(ex).__name__ fix in revision 3202d143a194.

New error message with this patch:

$ ./python -bWall -m bad_pyc
[. . .]/python: Error while finding spec for 'bad_pyc.__main__' (ImportError: bad magic number in 'bad_pyc': b'')
msg256196 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-11 05:08
Actually I forgot, this isn’t applicable to Python 2; see <https://bugs.python.org/issue14285#msg256190>.
msg256206 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-12-11 07:44
The bad-pyc patch looks good to me.
msg256268 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-12-12 07:31
New changeset 850cc65ceda4 by Martin Panter in branch '3.5':
Issue #19771: Omit irrelevant message if package could not be initialized
https://hg.python.org/cpython/rev/850cc65ceda4

New changeset 323c10701e5d by Martin Panter in branch 'default':
Issue #19771: Merge runpy error adjustment from 3.5
https://hg.python.org/cpython/rev/323c10701e5d
msg256693 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-12-18 16:48
Martin, could you please escape * in Misc/NEWS, it causes a warning when building documentation.
msg256717 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-19 01:48
I think Serhiy just fixed this in revision ed2420379b8d, so it should be okay now.
History
Date User Action Args
2015-12-19 01:48:27martin.pantersetmessages: + msg256717
2015-12-18 16:48:06SilentGhostsetnosy: + SilentGhost
messages: + msg256693
2015-12-12 07:49:09martin.pantersetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2015-12-12 07:31:25python-devsetnosy: + python-dev
messages: + msg256268
2015-12-12 00:22:23berker.peksagsetstage: patch review -> commit review
2015-12-11 07:44:06ncoghlansetmessages: + msg256206
2015-12-11 05:08:56martin.pantersetmessages: + msg256196
versions: - Python 2.7
2015-12-11 05:03:04martin.pantersetfiles: + bad-pyc.patch

messages: + msg256195
2015-12-11 05:00:31martin.pantersetmessages: + msg256194
versions: + Python 2.7, - Python 3.4
2015-12-02 03:53:32martin.pantersetdependencies: + Traceback wrong on ImportError while executing a package
messages: + msg255691
versions: + Python 3.6
2015-08-08 01:43:57martin.pantersetfiles: + issue_19771_runpy.patch.v4

messages: + msg248241
2015-08-07 14:06:09lacsetnosy: + lac
messages: + msg248193
2015-03-11 08:13:02martin.pantersetfiles: + issue_19771_runpy.patch.v3

messages: + msg237848
2015-03-11 00:37:55martin.pantersetnosy: + martin.panter
messages: + msg237829
2015-03-05 03:40:53berker.peksagsetnosy: + berker.peksag
stage: needs patch -> patch review

versions: + Python 3.5, - Python 3.3
2014-04-21 21:39:11luiz.poletosetfiles: + issue_19771.patch.v2
2014-04-21 17:00:20r.david.murraysetnosy: + r.david.murray
messages: + msg216951
2014-04-21 02:36:55luiz.poletosetfiles: + issue_19771.patch

messages: + msg216935
2014-04-21 02:18:08luiz.poletosetfiles: + issue_19771_tests.patch

nosy: + luiz.poleto
messages: + msg216934

keywords: + patch
2014-04-18 23:57:12eric.snowsetnosy: + eric.snow
2013-12-30 02:59:42Anthony.Kongsetnosy: + Anthony.Kong
2013-12-09 01:26:22ncoghlansetmessages: + msg205636
2013-12-08 23:09:23Fotis.Koutoulakissetnosy: + Fotis.Koutoulakis
messages: + msg205617
2013-11-25 12:39:47ncoghlancreate