New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runpy should check ImportError.name before wrapping it #63970
Comments
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. |
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 Please be lenient with me, this is one of my very first contributions to python. I will gladly accept all feedback. |
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')" |
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. |
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. |
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 ;) |
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. |
Posting a new patch with the following changes:
|
I tried to run some tests from the python3.4 test suite and got: The actual problem has nothing to do with test being a package. It |
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. |
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)). |
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. |
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'') |
Actually I forgot, this isn’t applicable to Python 2; see <https://bugs.python.org/issue14285#msg256190\>. |
The bad-pyc patch looks good to me. |
New changeset 850cc65ceda4 by Martin Panter in branch '3.5': New changeset 323c10701e5d by Martin Panter in branch 'default': |
Martin, could you please escape * in Misc/NEWS, it causes a warning when building documentation. |
I think Serhiy just fixed this in revision ed2420379b8d, so it should be okay now. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: