This patch avoids creating a second copy of importlib._bootstrap when a first one exists as _frozen_importlib.
This isn't perfect as it mutates the module when importlib is imported for the first time, but I think it's better than the status quo.
Also, importlib itself could be imported somewhere along the startup phase, so that all this is invisible to the user.

I'm not sure how to test this, since _frozen_importlib is an implementation detail, and changing that module's name would probably defeat the test already.

New patch with tests.

New patch also avoids calling _setup() a second time (which can be annoying since _setup() has a list.append() call somewhere).

So why the mutation? Are you that worried someone is going to import importlib._bootstrap directly?

This also costs in development complexity because not only do you have to run 'make' to get changes to be testable, but it also leads to difficult debugging situations where if you are not totally sure you got something working you won't find out until you see e.g. that the standard I/O streams were not initialized.

If you really feel the need to hide _frozen_importlib then it would be better to do the minimum required to get import up and running (should be once the encodings are up in Py_Initialize) and then pull in importlib._bootstrap and have that clear out what _frozen_importlib set like __import__(), sys.path_importer_cache(), and eventually sys.meta_path and sys.path_hooks (I wouldn't touch sys.modules, though, thanks to built-ins and extensions not liking to be reloaded).

I should also mention that all of this becomes much less important once issue #14605 is finished because at that point sys.meta_path and sys.path_hooks will have _frozen_importlib objects and that will be what importlib works off of directly. But I still understand the desire to eliminate _frozen_importlib from being exposed, it's just a matter of coming up with a reasonable solution.

My preference would also be for _frozen_importlib._bootstrap to overwrite as much evidence of itself as it can with the "real" one.

This would also mean that changes to importlib._bootstrap would actually take effect for user code almost immediately, *without* rebuilding Python, as the frozen version would *only* be used to get hold of the pure Python version.

> So why the mutation? Are you that worried someone is going to import
> importlib._bootstrap directly?

Well, importing importlib *does* import importlib._bootstrap, and
creates another copy of the module. importlib.__import__ is then wired
to _bootstrap.__import__, which is different from the built-in
__import__ (potentially using different globals, for example).

> This also costs in development complexity because not only do you have
> to run 'make' to get changes to be testable, but it also leads to
> difficult debugging situations where if you are not totally sure you
> got something working you won't find out until you see e.g. that the
> standard I/O streams were not initialized.

I'm worried that two different copies of importlib will lead to its own
difficult debugging situations.

> This would also mean that changes to importlib._bootstrap would
> actually take effect for user code almost immediately, *without*
> rebuilding Python, as the frozen version would *only* be used to get
> hold of the pure Python version.

Actually, _io, encodings and friends must be loaded before importlib
gets imported from Python code, so you will still have __loader__
entries referencing the frozen importlib, unless you also rewrite these
attributes.

My desire here is not to hide _frozen_importlib, rather to avoid subtle
issues with two instances of a module living in memory with separate
global states. Whether it's the frozen version or the on-disk Python
version that gets the preference is another question (a less important
one in my mind).

Antoine Pitrou wrote:
> 
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>> This would also mean that changes to importlib._bootstrap would
>> actually take effect for user code almost immediately, *without*
>> rebuilding Python, as the frozen version would *only* be used to get
>> hold of the pure Python version.
> 
> Actually, _io, encodings and friends must be loaded before importlib
> gets imported from Python code, so you will still have __loader__
> entries referencing the frozen importlib, unless you also rewrite these
> attributes.
> 
> My desire here is not to hide _frozen_importlib, rather to avoid subtle
> issues with two instances of a module living in memory with separate
> global states. Whether it's the frozen version or the on-disk Python
> version that gets the preference is another question (a less important
> one in my mind).

Why don't you freeze the whole importlib package to avoid all these
issues ? As side effect, it will also load a little faster.

To start, I'm *not* going to make the final call on this issue's solution. I'm inches away from importlib burnout and general integration frustration with trying to clean up the implicit behaviour. So to prevent me from making a bad decision I will you guys make the final call.

Anyway, I see two options here. One is the "let _frozen_importlib be *the* implementation, period" argument put forth by Antoine (MAL's "freeze everything" also falls under this since it suffers from the same issues). This is the easiest solution for the issue of not having overlapping implementations and cause potential mix-ups, etc. The issue becomes development difficulty goes up as now you are adding a compile step where if you screw up you can get really bad error messages (e.g. "standard streams could not be created" kind of stuff). This could theoretically be overcome if the importlib tests all used a manually created module directly from the source code to verify things before rebuilding (as well as making sure sys.path_importer_cache was cleaned out). With a restructuring of importlib's tests to use a common TestCase with the proper setUp()/teardown() for keeping things clean along with class and module fixtures to prevent obscene stuff like re-importing for every test method. Another option is we hide the source as _importlib or something to allow direct importation w/o any tricks under a protected name.

Then there is Nick's proposal of using _frozen_importlib to start up and then swap out with a new version created from the source during startup. This keeps development simple since the tests run against the code *almost* all other code will use and thus eliminate the test. The problem here is that startup is a smidgen slower and it requires you blacklist what needs to get swapped out and if you mess up that will be tough to debug as well.

Both get the same outcome but with different approaches, it's just a question of which one is easiest to maintain.

Le mardi 24 avril 2012 à 15:10 +0000, Brett Cannon a écrit :
> Both get the same outcome but with different approaches, it's just a
> question of which one is easiest to maintain.

I don't have any strong preference. Nick's proposal sounds slightly
better but Nick hasn't uploaded a patch yet :-)

test me
thod. Another option is we hide the source as _importlib or something to allow direct importation w/o any tricks under a protected name.

Using the freeze everything approach you make things easier for the
implementation, since you don't have to think about whether certain
pieces of code are already available or not.

For development, you can also have the package load bytecode
or source from an external package instead of running (all of)
the module's bytecode that was compiled into the binary.

This is fairly easy to do, since the needed exec() does not
depend on the import machinery.

The only downside is big if statement to isolate the frozen
version from the loaded one - would be great if we had a
command to stop module execution or code execution for a block to
make that more elegant, e.g. "break" at module scope :-)

> would be great if we had a
> command to stop module execution or code execution for a block to
> make that more elegant, e.g. "break" at module scope :-)

I floated that proposal on python-list a while back and the reaction was mixed. [1]  Maybe it's time to try again.  (moving over to python-ideas...)

[1] http://mail.python.org/pipermail/python-list/2011-June/1274424.html

I don't quite follow what you are suggesting, MAL. Are you saying to freeze importlib.__init__ and importlib._bootstrap and somehow have improtlib.__init__ choose what to load, frozen or source?

Brett Cannon wrote:
> 
> Brett Cannon <brett@python.org> added the comment:
> 
> I don't quite follow what you are suggesting, MAL. Are you saying to freeze importlib.__init__ and importlib._bootstrap and somehow have improtlib.__init__ choose what to load, frozen or source?

No, it always loads and runs the frozen code, but at the start of
the module code it branches between the frozen bytecode and the code
read from an external file.

Pseudo-code in every module you wish to be able to host externally:

#
# MyModule
#
if operating_in_dev_mode and '<frozen>' in __file__:
    exec(open('dev-area/MyModule.py', 'r).read(), globals(), globals())
else:
    # Normal module code
    class MyClass: ...
    # hundreds of lines of code...

Aside: With a module scope "break", the code would look more elegant:

#
# MyModule
#
if operating_in_dev_mode and '<frozen>' in __file__:
    exec(open('dev-area/MyModule.py', 'r).read(), globals(), globals())
    break

# Normal module code
class MyClass: ...
# hundreds of lines of code...

So basically if you are running in a checkout, grab the source file and compile it manually since its location is essentially hard-coded and thus you don't need to care about sys.path and all the other stuff required to do an import, while using the frozen code for when you are running an installed module since you would otherwise need to do the search for importlib's source file to do a load at startup properly.

That's an interesting idea. How do we currently tell that the interpreter is running in a checkout? Is that exposed in any way to Python code?

> That's an interesting idea. How do we currently tell that the
> interpreter is running in a checkout? Is that exposed in any way to
> Python code?

Look for _BUILDDIR_COOKIE in setup.py. But that's only for non-Windows
platforms (I don't think setup.py is invoked under Windows).

Brett Cannon wrote:
> 
> Brett Cannon <brett@python.org> added the comment:
> 
> So basically if you are running in a checkout, grab the source file and compile it manually since its location is essentially hard-coded and thus you don't need to care about sys.path and all the other stuff required to do an import, while using the frozen code for when you are running an installed module since you would otherwise need to do the search for importlib's source file to do a load at startup properly.

Right.

> That's an interesting idea. How do we currently tell that the interpreter is running in a checkout? Is that exposed in any way to Python code?

There's some magic happening in site.py for checkouts, but I'm not sure
whether any of that is persistent or even available at the time these
particular imports would happen.

Then again, I'm not sure you need to know whether you have a checkout
or not. You just need some flag to identify whether you want the
search for external module code to take place or not. sys.flags
could be used for that.

Modules/getpath.c seems to be where the C code does it when getting paths for sys.path. So it would be possible to use that same algorithm to set some sys attribute (e.g. in_checkout or something) much like sys.gettotalrefcount is optional and only shown when built with --with-pydebug. Otherwise some directory structure check could be done (e.g. find importlib/_bootstrap.py off of sys.path, and then see if ../Modules/Setup or something also exists that would never show up in an installed CPython).

> Otherwise some directory structure check could be done (e.g. find
> importlib/_bootstrap.py off of sys.path, and then see
> if ../Modules/Setup or something also exists that would never show up
> in an installed CPython).

Well, the directory structure check *is* pybuilddir.txt (under POSIX,
again; under Windows, you might want to check for Lib/importlib
directly).
But, agreed, this could be factored in a sys._private_something
attribute.

That's why I was thinking of tying into Modules/getpath.c because I assume that would work cross-platform. Is that incorrect?

> That's why I was thinking of tying into Modules/getpath.c because I
> assume that would work cross-platform. Is that incorrect?

Windows uses PC/getpathp.c, not Modules/getpath.c (with tons of
duplicate code)... So you would have to tie into both :)

Brett Cannon wrote:
> 
> Modules/getpath.c seems to be where the C code does it when getting paths for sys.path. So it would be possible to use that same algorithm to set some sys attribute (e.g. in_checkout or something) much like sys.gettotalrefcount is optional and only shown when built with --with-pydebug. Otherwise some directory structure check could be done (e.g. find importlib/_bootstrap.py off of sys.path, and then see if ../Modules/Setup or something also exists that would never show up in an installed CPython).

Why not simply use a flag that get's set based on an environment
variable, say PYTHONDEVMODE ?

Adding more cruft to getpath.c or similar routines is just going to
slow down startup time even more...

Python 2.7 has a startup time of 70ms on my machine; compare that to
Python 2.1 with 10ms and
Perl 5 with just 4ms.

> Adding more cruft to getpath.c or similar routines is just going to
> slow down startup time even more...

The code is already there.

Antoine Pitrou wrote:
> 
>> Adding more cruft to getpath.c or similar routines is just going to
>> slow down startup time even more...
> 
> The code is already there.

Code to detect whether you're running off a checkout vs. a normal
installation by looking at even more directories ? I don't
see any in getpath.c (and that's good).

> Code to detect whether you're running off a checkout vs. a normal
> installation by looking at even more directories ? I don't
> see any in getpath.c (and that's good).

Look for "pybuilddir.txt".

That solves the "I'm in a checkout" problem but it doesn't tell you necessarily where the Lib directory is if you e.g. build from within another directory like Python/, which places the executable and pybuilddir.txt in the current directory.

Now obviously you could argue supporting that case is not worth it for development-sake, but I figured since I knew it existed I should put it out there.

Antoine Pitrou wrote:
> 
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>> Code to detect whether you're running off a checkout vs. a normal
>> installation by looking at even more directories ? I don't
>> see any in getpath.c (and that's good).
> 
> Look for "pybuilddir.txt".

Oh dear. Another one of those hacks... why wasn't this done using
constants passed in by the configure script and simple string
comparison ?

BTW: The startup time of python3.3 is 113ms on my machine, that's
more than twice as long as python2.7. Given the history, it
looks like no one cares about these things anymore... :-(

> > Look for "pybuilddir.txt".
> 
> Oh dear. Another one of those hacks... why wasn't this done using
> constants passed in by the configure script and simple string
> comparison ?

How would that help distinguish between an installed Python and a
non-installed Python? If you have an idea about that, please open an
issue and explain it precisely :)

Antoine Pitrou wrote:
> 
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>>> Look for "pybuilddir.txt".
>>
>> Oh dear. Another one of those hacks... why wasn't this done using
>> constants passed in by the configure script and simple string
>> comparison ?
> 
> How would that help distinguish between an installed Python and a
> non-installed Python? If you have an idea about that, please open an
> issue and explain it precisely :)

The question pybuildir.txt apparently tries to solve is whether Python
is running from the build dir or not. It's not whether Python was
installed or not. Checking for the build dir can be done by looking
at the argv[0] of the executable and comparing that to the build dir.
This can be compiled into the interpreter using a constant, say
BUILDIR. At runtime, you'd simply compare the current argv[0] to
the BUILDDIR. If it matches, you know that you can assume the
build dir layout with reasonable certainty and proceed accordingly.
No need for extra joins, file reads, etc.

But given the enormous startup time of Python 3.3, those few stats
won't make a difference anyway. This would need a completely different
holistic approach. Perhaps something for SoC project.

> The question pybuildir.txt apparently tries to solve is whether Python
> is running from the build dir or not. It's not whether Python was
> installed or not.

That's the same, for all we're concerned.
But pybuilddir.txt does not only solve that problem. It also contains
the path to extension modules generated by setup.py, so that sys.path
can be setup appropriately at startup.

Antoine Pitrou wrote:
> 
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>> The question pybuildir.txt apparently tries to solve is whether Python
>> is running from the build dir or not. It's not whether Python was
>> installed or not.
> 
> That's the same, for all we're concerned.
> But pybuilddir.txt does not only solve that problem. It also contains
> the path to extension modules generated by setup.py, so that sys.path
> can be setup appropriately at startup.

Would be easier to tell distutils to install the extensions
in a fixed name dir (instead of using a platform and version
in the name) and then use that getpath.c. distutils is pretty
flexible at that :-)

Still no patch from me, but I did create the rudiments of a shared script for poking around at the import internals (Tools/scripts/import_diagnostics.py)

Looking at Antoine's patch, I'd be happier with it if it *didn't* mutate the attributes of _frozen_importlib, but instead just added importlib._bootstrap as an alias for accessing it.

That would bring it in line with the way we handle os.path as being just an alias for the appropriate top level module:

>>> import os.path
>>> os.path.__name__
'posixpath'

Getting access to the source level _bootstrap implementation for testing purposes would then just require the usual techniques for bypassing C accelerators (specifically, using test.support.import_fresh_module with "_frozen_importlib" blocked).

That would address the immediate problem of module duplication, without misrepresenting what is going on in potentially confusing ways.

> Would be easier to tell distutils to install the extensions
> in a fixed name dir (instead of using a platform and version
> in the name) and then use that getpath.c. distutils is pretty
> flexible at that :-)

Look, this is becoming very off-topic and you aren't proposing anything
concrete (I see neither patches nor problems being solved). Could you
open another issue, if you care so much?

> Looking at Antoine's patch, I'd be happier with it if it *didn't*
> mutate the attributes of _frozen_importlib, but instead just added
> importlib._bootstrap as an alias for accessing it.

I thought it would be nicer for __file__, __name__ and __package__ to reflect the actual source code metadata (__file__ is always a py file while __cached__ may point to the compiled bytecode). But I don't have any strong feelings about that.

Yes, __file__ can end up misleading if you modify the Python source without recompiling, but I think most people would only read the code without modifying it.

To answer MAL's question about startup, I benchmarked on my machine using the normal_startup benchmark from hg.python.org/benchmarks and the bootstrap work only caused a 5-6% slowdown in a non-debug build. If you do it in a debug build it's much worse (I think it was 12% when I benchmarked).

OK, I'm leaning back towards my original preference of getting _frozen_importlib out of the way as quickly as we can.

Specifically, I'm thinking of separating out the entry point used by importlib.__init__ from that used by pythonrun.c, such that the latter calls a "_bootstrap_from_frozen" function that returns a reference to "importlib._bootstrap", which pythonrun then places in the interpreter state.

There would be a few builtin modules that still end up with loaders from _frozen_importlib (specifically, those referenced from importlib._bootstrap._setup as well as importlib itself), but the vast majority of imported modules would only see the "real" versions from importlib._bootstrap.

Attached patch is an initial attempt (the reference counting on the two modules is likely still a bit dodgy - this is my first version that didn't segfault as I got used to the mechanics of dealing with a frozen module, so it errs on the side of leaking references)

> Attached patch is an initial attempt (the reference counting on the
> two modules is likely still a bit dodgy - this is my first version
> that didn't segfault as I got used to the mechanics of dealing with a
> frozen module, so it errs on the side of leaking references)

But does it make debugging any easier? The IO streams are not yet
initialized at that point (neither are the codecs), so you are executing
_bootstrap.py from a very bare interpreter.

Yes, in that you'll be able to pick up changes in _bootstrap.py *without* having to rebuild Python.

With this in place, we could then get rid of the automatic regeneration of importlib.h which is a complete nightmare if you ever break your built interpreter while hacking on the bootstrapping (as I now know from experience).

With my approach, the experience is instead:

- modify _bootstrap.py, hack until any new tests pass
- run a new explicit "make freeze_importlib" command
- run "make"
- check everything still works
- commit and push

If you forget to run "make freeze_importlib", it doesn't really matter all that much, since the frozen one will only be used to find the real one, so it isn't a disaster if it's a little out of date. (That said, we should still have a test that at least checks the two modules have the same attributes)

It does mean that importlib.__init__ also needs to be able to run in a partially initialised interpreter, hence the switch from "import imp" to "import _imp".

Actually, rather than a test in test suite, we would just change the current automatic rebuild to a Modules/Setup style "'Lib/importlib._bootstrap.py' is newer than 'Python/importlib.h', you may need to run 'make freeze_importlib'"

> How do we currently tell that the interpreter is running in a checkout?
sysconfig.is_python_build()

Someone has to confirm that this works on Windows too, as I’ve been told that not installed vs. installed is less clear on that OS.

> Actually, rather than a test in test suite, we would just change the
> current automatic rebuild to a Modules/Setup style
> "'Lib/importlib._bootstrap.py' is newer than 'Python/importlib.h', you
> may need to run 'make freeze_importlib'"

-1 from me. Nobody pays attention to this kind of warning.
(and the Modules/Setup thing is a nuisance)
Really, we must unsure that the frozen version of importlib is
up-to-date.
Also, normally you would write your tests in test_import, so that the
builtin import *is* tested. So you have to regenerate importlib before
committing (or you break the buildbots).

The other advantage of splitting the entry points is that we can tweak Brett's plan to make the import machinery explicit such that it happens in a separate function that's only called from __init__.py.

That way the published hooks will always be from the on-disk implementation and never from the frozen one.

If you're after the ability to emit debugging messages in a way that doesn't cause fatal errors during system startup, the only way I can see is to have a "do nothing" module level display function in _bootstrap.py that is later replaced with a reference to builtins.print:

  def _debug(*args, **kwds):
      pass

At the very least, failing to regenerate importlib.h shouldn't be a fatal build error. It should just run with what its got, and hopefully you will get a working interpreter out the other end, such that you can regenerate the frozen module on the next pass.

If we change that, then I'm OK with keeping the automatic rebuild.

So how would you tweak the explicit work I'm doing? The code is going to rely on sys.path_hooks and sys.meta_path being populated. I guess the frozen code can set up initially, and then importlib simply substitutes out classes from the frozen module to the code from the source version (which should be easy based on __class__ and __class__.__name__ or something). Or if you do this before anyone else (e.g. zipimport) gets to sys.path_hooks and sys.meta_path then you could just blow them away without care and simply set them up again.

Nick Coghlan wrote:
> 
> Nick Coghlan <ncoghlan@gmail.com> added the comment:
> 
> At the very least, failing to regenerate importlib.h shouldn't be a fatal build error. It should just run with what its got, and hopefully you will get a working interpreter out the other end, such that you can regenerate the frozen module on the next pass.
> 
> If we change that, then I'm OK with keeping the automatic rebuild.

I fixed that already today.

You now get a warning message from make, but no build error across
all buildbots like I had run into yesterday when working on the code.

Marc-Andre Lemburg wrote:
> 
> Marc-Andre Lemburg <mal@egenix.com> added the comment:
> 
> Nick Coghlan wrote:
>>
>> Nick Coghlan <ncoghlan@gmail.com> added the comment:
>>
>> At the very least, failing to regenerate importlib.h shouldn't be a fatal build error. It should just run with what its got, and hopefully you will get a working interpreter out the other end, such that you can regenerate the frozen module on the next pass.
>>
>> If we change that, then I'm OK with keeping the automatic rebuild.
> 
> I fixed that already today.

See http://bugs.python.org/issue14605 and
http://hg.python.org/lookup/acfdf46b8de1 +
http://hg.python.org/cpython/rev/5fea362b92fc

> You now get a warning message from make, but no build error across
> all buildbots like I had run into yesterday when working on the code.

My plan would be for the frozen version to be entirely implicit, and have only the subsequent import of the version from disk actually modify the public hooks.

However, I realised today that my current patch would break "stdlib-from-zipfile" approaches, so any bootstrapping of importlib from disk would have to take place after zipimport was put in place. That suggests a few possible changes:
- reordering import_init so zipimport is initialised before the bootstrapping step
- possibly downgrading failure of the bootstrapping step to a warning rather than a fatal error (i.e. continuing with the frozen version if the source version can't be found)

This may still all prove to be too complicated and fragile, but I'm not prepared to give up on it yet - having the interpreter pick up on _bootstrap.py changes for the main import system *without* needing to be rebuilt first seems worth a bit of additional complexity in the bootstrapping mechanism.

Uploaded new bootstrapping patch that handles the fully explicit import machinery.

I also tweaked a couple of things so it plays nicely in terms of building an initial version with the checked in importlib.h. Specifically: pythonrun still calls _frozen_importlib._install and can tolerate that function returning None. Longer term, we'd give the two hooks different names and returning None will become a fatal error, but for the moment, the current behaviour makes the patch much easier to work with.

Order is still wrong relative to the zipimport machinery and I haven't benchmarked the startup time overheads.

New changeset 257cbd2fac38 by Brett Cannon in branch 'default':
Issue #13959: Re-implement imp.get_suffixes() in Lib/imp.py.
http://hg.python.org/cpython/rev/257cbd2fac38

Here's my take.  No one will care about _frozen_importlib vs. importlib._bootstrap normally, right?  If __module__/__file__ says _frozen_importlib, it's no big deal.  The only time you care about the distiction for importlib._bootstrap is when you're hacking on _bootstrap.py.  So let's keep the common case in sight and go from there.

There are two sides to the uncommon case:

1. making sure importlib still works after hacking on _bootstrap.py (test_imp, test_import, test_importlib using your new _bootstrap.py).
2. making sure everything still works after hacking on _bootstrap.py (the whole test suite with a new importlib.h?).

For the first part, let's simply ignore the pure Python importlib._bootstrap by default?  Then we stick a context manager in importlib.test.util that enables it.  When you're hacking on _bootstrap.py, you switch it over.  The common path stays pretty clean.

I've attached a patch for the first part which has similarities to Antoine's.  (I didn't apply the context manager to the importlib test cases though.)

For that second part, something along the lines of what Nick has posted would be pretty close, but I'm not sure it's worth it.  From what I understand, Nick's patch would add yet another import (importlib) to startup to cover a situation that happens very infrequently (hacking _bootstrap.py).  However, I'm torn because...

...dealing with a busted importlib.h is not fun.  Is there a different approach we could take for that second part?  Perhaps something like this:

1. python starts up normally.
2. we clear out all the entire import state except for builtins.
3. we stick importlib._bootstrap in place.
4. we set builtins.__import__ to importlib.__import__.
5. we re-populate sys.modules by reloading all the modules that were in there before (?).
6. we run the test suite against this new import state.
7. ...
8. profit!

I'm probably missing something here, but I expect we could stick something like that in some place like importlib.test.util.  Would that be sufficient to mitigate the chance of breaking importlib.h?


------------------------------------

Example of using my patch:

>>> import sys
>>> import importlib.test.util as util
>>> importlib._bootstrap
<module '_frozen_importlib' from '<frozen>'>
>>> sys.modules['importlib._bootstrap']
<module '_frozen_importlib' from '<frozen>'>
>>> with util.bootstrap_context(importlib, importlib._pure_bootstrap):
...     importlib._bootstrap
...     sys.modules['importlib._bootstrap']
...
<module 'importlib._bootstrap' from '/home/esnow/projects/cpython/Lib/importlib/_bootstrap.py'>
<module 'importlib._bootstrap' from '/home/esnow/projects/cpython/Lib/importlib/_bootstrap.py'>
>>> importlib._bootstrap
<module '_frozen_importlib' from '<frozen>'>
>>> sys.modules['importlib._bootstrap']
<module '_frozen_importlib' from '<frozen>'>

The piece you're missing is that the interpreter state holds a direct reference to the import machinery in interp->importlib, and *that's* what gets used by the builtin __import__ implementation.

I'm beginning to think the thing to do is to simply say "yes, there are two copies of importlib._bootstrap". By default, the compiled in one is used, but you can replace it with the on-disk one by appropriately editing sys.meta_path and sys.path_hooks.

Trying to hide it isn't going to eliminate the potential problems - it's just going to move the problems around (and likely make them even more confusing in the process).

Forgot to add: in our own tests, we should ensure that both the frozen and on-disk versions get executed.

I believe that's already the case, since I don't recall anyone removing the test infrastructure that ensured both import.c and importlib are tested for correct behaviour.

> I believe that's already the case, since I don't recall anyone
> removing the test infrastructure that ensured both import.c and
> importlib are tested for correct behaviour.

What do you mean? I think test_importlib only tests the on-disk version.

> Here's my take.  No one will care about _frozen_importlib vs.
> importlib._bootstrap normally, right?  If __module__/__file__ says
> _frozen_importlib, it's no big deal.

The reason I'd prefer __file__ to point to the actual Python file is so
that people reading a traceback can find the source code. Of course
that's a bit minor.
(and, incidentally, the traceback itself will display the source code
lines)

> The only time you care about the distiction for importlib._bootstrap
> is when you're hacking on _bootstrap.py.  So let's keep the common
> case in sight and go from there.

Agreed.

> For the first part, let's simply ignore the pure Python
> importlib._bootstrap by default?  Then we stick a context manager in
> importlib.test.util that enables it.  When you're hacking on
> _bootstrap.py, you switch it over.  The common path stays pretty
> clean.

Looks good to me.

> I've attached a patch for the first part which has similarities to
> Antoine's.  (I didn't apply the context manager to the importlib test
> cases though.)

I think set_bootstrap() looks a bit fragile, since we have to manually
add any importlib attributes that are exported in importlib/__init__.py.
Perhaps we could detect them automatically?

To respond to Nick's "yes, there are two copies of importlib._bootstrap" leanings, distutils2 has actually run into issues with this because they initially made some assumptions about consistency in what importlib returned vs. what import does (Arfrever can explain better than I can since he keeps pointing it out to me =).

If using _frozen_importlib to bootstrap in importlib._bootstrap is looking bad, then I'm fine w/ simply having the tests for importlib and imp use importlib._bootstrap and otherwise just use _frozen_importlib for everything else since I have tried to be diligent to add any and all import-related tests to importlib. Except while developing the code should be exactly the same so hiding the details really won't make much of a difference in the very common case.

If we go this route, though, then we really should take this time to do a proper context manager/decorator/whatever that covers all import state (including uncaching modules and sys.path_importer_cache) that we might care about and put the solution into test.support (what issue #14715 is asking for and I think is reasonable). We should also then add to regrtest detection of stuff left in sys.path_importer_cache or sys.modules that do not come from _frozen_importlib (which should help with the sporadic test_imp failure).

It was distribute (fork of setuptools, with added support for Python 3), not distutils2.
distribute has been changed to directly use _frozen_importlib:
https://bitbucket.org/tarek/distribute/changeset/a2685f3af854
https://bitbucket.org/tarek/distribute/changeset/77c8b155a07d
distribute checks __loader__ and __class__.__mro__ attributes of modules.

> It was distribute (fork of setuptools, with added support for Python 3), not distutils2.
> distribute has been changed to directly use _frozen_importlib:

This sounds like a rather good hint we need to avoid duplicate copies.

I think it's beyond a hint and says we need to find a solution or else other people will run into similar issues.

And while I'm thinking about it, there is precedent for exposing modules under a different name than they are actually installed as in the system (e.g. os.path is posixpath), so I don't think we need to bend over backwards to mask every detail if the bootstrap solution is not taken (e.g. if we decided to just paper over _frozen_importlib we don't need to iterate over _frozen_importlib.__dict__ and patch up __module__). But I do think that we need to choose some solution to prevent this "forking" of code in the running interpreter.

In that case, how about we go with:

1. By default, importlib._bootstrap is never imported. Instead, it is set to be a reference to _frozen_importlib. However, _frozen_importlib does *not* lie about where it came from (and doesn't assume the on-disk source matches the frozen source).

2. We provide two private functions in importlib.__init__: one that replaces all _frozen_importlib references in the import state with importlib._bootstrap references (retrieving the latter from disk first), and one that reverses the process.

Note that the __import__ builtin should be replaced as well, since that will otherwise call in to the frozen version of the module.

This is basically the same as Eric Snow's suggestion, just with most of the nuts and bolts kept within importlib, so that the testing context manager doesn't need to know the details - it can just call the appropriate importlib functions to change the active implementation.

Should we have a separate context manager for this, or just make it a flag for a unified import_state() decorator? Or do we want to *always* force the use of the Python code instead of the frozen code?

> Should we have a separate context manager for this, or just make it a
> flag for a unified import_state() decorator? Or do we want to *always*
> force the use of the Python code instead of the frozen code?

Ideally, we would want to test both versions, so that any oddity in the
freezing mechanism gets exercised and diagnosed properly.

> Ideally, we would want to test both versions, so that any oddity in the
> freezing mechanism gets exercised and diagnosed properly.

(not to mention the speedups in import.c)

I'm +1 on Nick's recommendation.

@Antoine
> Ideally, we would want to test both versions, so that any oddity in
> the freezing mechanism gets exercised and diagnosed properly.

+1

Does this mean that the whole test suite should be run under both (whenever _bootstrap.py is modified)?  Would that warrant a new flag for the test suite or even an automated check?

That's what I was getting at with this:

> 1. python starts up normally.
> 2. we clear out all the entire import state except for builtins.
> 3. we stick importlib._bootstrap in place.
> 4. we set builtins.__import__ to importlib.__import__.
> 5. we re-populate sys.modules by reloading all the modules that
> were in there before (?).
> 6. we run the test suite against this new import state.

It might be sufficient to only run tests from the following files with both importlib._bootstrap and _frozen_importlib:
test_imp.py
test_import.py
test_importhooks.py
test_importlib.py
test_pkgimport.py
test_threaded_import.py
test_zipimport.py
test_zipimport_support.py

> It might be sufficient to only run tests from the following files with
> both importlib._bootstrap and _frozen_importlib:

I was only thinking about test_importlib myself.

I would say test_importlib and test_imp (test_import really should just get folded into test_importlib).

Realized that any decorator or context manager that is created for swapping _frozen_importlib/importlib._bootstrap should also verify no module is left in sys.modules with a bad loader and that sys.path_importer_cache doesn't have a bad finder (I would say that this would go into test.support.regrtest's state checks, but that seems overkill for only two tests).

And this might be worth doing as a decorator (method or class) to make it easier to make sure the requisite tests always run with both versions (or copying what test_warnings does). I don't want to do anything in a module's test_main() as that precludes using unittest's test discovery for running tests.

Unless someone plans to do further work on this, I'd like to commit unique_importlib3.patch. A working solution is better than nothing.

It's actually at the top of my list, but may still be a week or two until I can get to it.

+1 Antoine - your patch is better than the status quo, so it makes sense to apply it.

If Eric can make the other way work in time for 3.3, great, but at least we'll have a solid fallback position without it.

New changeset e3a984076837 by Antoine Pitrou in branch 'default':
Issue #14657: The frozen instance of importlib used for bootstrap is now also the module imported as importlib._bootstrap.
http://hg.python.org/cpython/rev/e3a984076837

Ok, I've committed the patch. I'm closing this issue, but of course potential improvements can be posted under a new issue.

Do people feel the need to change the importlib tests to run against both the frozen code and importlib/_bootstrap.py? If so either the test_warnings approach (or a new one using class decorators or something) should be used to run the tests twice.

Given Antoine's other change to the build process to fix the bootstrapping problem, I'm OK with leaving it up to anyone hacking on _bootstrap.py to remember that they need to run make if they want the interpreter to see any of their changes.

Unlike the C accelerator cases, it's not possible for the two implementations to get out of sync on the buildbots, so running the tests twice would just be a convenience change for anyone hacking on _bootstrap.py rather than being needed for correctness.

That said, if someone created a new issue and posted a patch to run the tests twice, I wouldn't object - a classic case of +0 :)