classification
Title: importlib: ExtensionFileLoader not used to load packages from __init__.so files
Type: behavior Stage: test needed
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Arfrever, brett.cannon, eric.snow, python-dev, r.david.murray, scoder
Priority: release blocker Keywords:

Created on 2012-08-07 18:35 by scoder, last changed 2012-08-11 15:02 by scoder. This issue is now closed.

Messages (14)
msg167630 - (view) Author: Stefan Behnel (scoder) * Date: 2012-08-07 18:35
The new importlib shows a regression w.r.t. previous CPython versions. It no longer recognises an "__init__.so" file as a package. All previous CPython versions have always tested first for an extension file before testing for a .py/.pyc file. The new importlib explicitly excludes the ExtensionFileLoader from loading packages. See importlib/_bootstrap.py, line 1579 onwards:

   1579 def _get_supported_file_loaders():
   1580     """Returns a list of file-based module loaders.
   1581 
   1582     Each item is a tuple (loader, suffixes, allow_packages).
   1583     """
   1584     extensions = ExtensionFileLoader, _imp.extension_suffixes(), False    # <== bug here
   1585     source = SourceFileLoader, SOURCE_SUFFIXES, True
   1586     bytecode = SourcelessFileLoader, BYTECODE_SUFFIXES, True
   1587     return [extensions, source, bytecode]

(BTW, I'm not sure what to file this bug against - "Interpreter Core" or "Library"?)
msg167632 - (view) Author: Stefan Behnel (scoder) * Date: 2012-08-07 19:02
Additional info: working around this bug from user code is fairly involved because some FileLoader instances are already created early at initialisation time and the overall configuration of the FileLoaders is kept in the closure of a path hook.
msg167633 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-08-07 19:09
I wouldn't worry about working around it from user code...finding and fixing regressions is what the beta period is for, after all.

As for which component, that's a good question with importlib ;)  Both, I suppose.  Not that components are used for much other than auto-nosy, and there's no auto-nosy for either interpreter core or library.
msg167634 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-08-07 19:17
So this was on purpose. At some point over the past five years I was working on importlib I think I was told I shouldn't support it (or something) as I explicitly remember making the decision to not support __init__.so on purpose. Plus the package specification never suggested anything but __init__.py to be used to begin with, so it isn't a bug as much as a design oversight (as is a large portion of import semantics unfortunately). But I take it Cython has been relying on this for years so that kind of kills treating this as part of import semantics cleanup.

Now to reverse this several things need to happen. First the whole third item in the tuples passed to PathFinder is unneeded (it existed purely to block extension module __init__ files) and thus should be removed where necessary. Second, the tests for extensions (test.test_importlib.extension.test_finder) need to be updated, but that gets messy as there is no __init__.so in the stdlib to test against to verify everything works. Tests could cheat and muck with PathFinder's file cache, but that is definitely not a blackbox test anymore. Don't know how best to deal with this short of an xx/__init__.so module being added to the stdlib.

And just for future reference, Stefan, since this is a direct import issue and not something to do with importlib's API I consider it an Interpreter Core bug.

And I'm glad we cache directory stat results now as changing this will have minuscule overhead compared to adding another stat call per package search without the cache.
msg167637 - (view) Author: Stefan Behnel (scoder) * Date: 2012-08-07 19:39
Hi, thanks for bringing in the 'historical details'. It's not so much that "Cython has been relying on it" - it's entirely up to users what they compile and what not. It's more that I don't see anything being wrong with it as a feature and that it worked before.

The reason why I found it was that I'm trying to make Python benchmark suite run better in Cython, and that requires compiling all code, some of which is often hidden in the package file.

I understand that this is probably a rare real-world requirement because things can usually be made to work by moving code into a separate extension module and reimporting it from there into an __init__.py module. I guess that's why people told you to drop the feature.

There's also issue13429, which still restricts the init time compatibility of binary extensions with normal Python modules. Arguably, to make __init__.so work correctly, the "__path__" attribute would have to be set at module creation time as well, otherwise, relative imports won't work during initialisation. I guess I should update my patch in that ticket to reflect that.
msg167638 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-08-07 20:30
That's another thing that would need to be changed; ExtensionFileLoader would need to work for is_package() and would need to set __path__ as needed.
msg167756 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-08-09 03:44
Addressed question of introspecting loader_details in issue 15600.  (see msg167632)
msg167891 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-08-10 16:45
To add a nice wrinkle to all of this, Windows debug builds use _d.pyd as the extension suffix. Notice how that doesn't start with a nice '.' to make finding the suffix easy? Yeah, that complicates ExtensionLoader.is_package() as I will have to now directly reference imp.extension_suffixes() which sucks as it hard codes what extensions are used instead of being more flexible and just ignoring that bit of detail.
msg167900 - (view) Author: Roundup Robot (python-dev) Date: 2012-08-10 17:48
New changeset 1db6553f3f8c by Brett Cannon in branch 'default':
Issue #15576: Allow extension modules to be a package's __init__
http://hg.python.org/cpython/rev/1db6553f3f8c
msg167933 - (view) Author: Stefan Behnel (scoder) * Date: 2012-08-11 04:28
I understand that this is not trivial to test as part of the regression test suite. However, when I try it now I get this:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "__init__.py", line 8, in init my_test_package (my_test_package/__init__.c:1032)
SystemError: Parent module 'my_test_package' not loaded, cannot perform relative import

This is running Cython's "initial_file_path" test, which is meant to check that we provide a fake __path__ for the package module for the init function (as long as issue13429 isn't resolved).

The test code is here, failing in line 21 (each section is a separate file):

https://github.com/cython/cython/blob/master/tests/run/initial_file_path.srctree

I'm running the test like this:

python3 runtests.py --no-cpp --no-pyregr --no-unit --debug -vv initial_file_path

I also let it print sys.modules and the package isn't registered there yet at the time of the import.
msg167955 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-08-11 14:00
So that test case is hard for me to follow since I don't know what a .srctree file is and the file seems to have some magical comments in it.

If my assumptions are correct you are generating a C file that does a relative import in the extension module's init function? Has that actually worked in the past? The error you are seeing suggests that the module is not being inserted into sys.modules before executing the body which I have no control over since that's the purview of imp.load_dynamic(). If you skip the relative imports does the import work otherwise?
msg167956 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-08-11 14:04
The reason I ask this is that if I simply move e.g. audioop.so to audioop/__init__.so everything works fine in terms of being able to import that module as a package.
msg167959 - (view) Author: Stefan Behnel (scoder) * Date: 2012-08-11 14:37
Hmm, yes, sounds more like a separate issue than something to add to this ticket. It worked before (starting with Py2.5, which was the first Python version to support relative imports) and only stopped working in 3.3 now.

The .srctree test file basically just unfolds into a file system hierarchy with each file named as in the preceding "magical" comment. The commands in the lines before the first comment line are then executed for the test (usually: build module with distutils, import it, do something with it).
msg167968 - (view) Author: Stefan Behnel (scoder) * Date: 2012-08-11 15:02
I've created issue15623, so that we can keep this one as fixed.
History
Date User Action Args
2012-08-11 15:02:43scodersetstatus: open -> closed
resolution: fixed
messages: + msg167968
2012-08-11 14:37:39scodersetmessages: + msg167959
2012-08-11 14:34:34Arfreversetnosy: + Arfrever
2012-08-11 14:04:17brett.cannonsetmessages: + msg167956
2012-08-11 14:00:27brett.cannonsetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg167955

stage: resolved -> test needed
2012-08-11 04:28:43scodersetmessages: + msg167933
2012-08-10 17:48:52brett.cannonsetstatus: open -> closed
assignee: brett.cannon
resolution: fixed
stage: needs patch -> resolved
2012-08-10 17:48:03python-devsetnosy: + python-dev
messages: + msg167900
2012-08-10 16:46:00brett.cannonsetmessages: + msg167891
2012-08-09 03:44:38eric.snowsetmessages: + msg167756
2012-08-07 20:30:16brett.cannonsetmessages: + msg167638
2012-08-07 19:39:13scodersetmessages: + msg167637
components: + Library (Lib), - Interpreter Core
2012-08-07 19:17:30brett.cannonsetmessages: + msg167634
components: - Library (Lib)
2012-08-07 19:09:31r.david.murraysetpriority: normal -> release blocker

nosy: + r.david.murray
messages: + msg167633

components: + Interpreter Core
stage: needs patch
2012-08-07 19:02:30scodersetmessages: + msg167632
2012-08-07 18:42:33eric.snowsetnosy: + eric.snow
2012-08-07 18:42:23eric.snowsetnosy: + brett.cannon
2012-08-07 18:35:49scodercreate