Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(156)

#2377: Replace __import__ w/ importlib.__import__

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 9 months ago by brett
Modified:
5 years, 7 months ago
Reviewers:
pjenvey, pitrou, thomas, benjamin, merwok
CC:
brett.cannon, Nick Coghlan, AntoinePitrou, haypo, eric.smith, Benjamin Peterson, serwy, eric.araujo, Arfrever, alex, Trundle, devnull_psf.upfronthosting.co.za, eric.snow
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 22

Patch Set 3 #

Total comments: 71

Patch Set 4 #

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/idlelib/EditorWindow.py View 2 chunks +7 lines, -3 lines 0 comments Download

Messages

Total messages: 9
pjenvey
http://bugs.python.org/review/2377/diff/4262/15279 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4262/15279#newcode2828 Python/import.c:2828: if (__import__ == NULL) { All of this screams ...
5 years, 9 months ago #1
brett.cannon
http://bugs.python.org/review/2377/diff/4262/15279 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4262/15279#newcode2828 Python/import.c:2828: if (__import__ == NULL) { On 2012/02/25 08:02:44, pjenvey ...
5 years, 9 months ago #2
AntoinePitrou
Some comments. I haven't looked at everything in detail (especially not the C code). http://bugs.python.org/review/2377/diff/4262/15250 ...
5 years, 9 months ago #3
brett.cannon
http://bugs.python.org/review/2377/diff/4262/15250 File Lib/_importlib.py (right): http://bugs.python.org/review/2377/diff/4262/15250#newcode1 Lib/_importlib.py:1: """Core implementation of import. There was when I was ...
5 years, 8 months ago #4
Thomas Wouters
LGTM. Only commenting on the easily fixed or obvious issues, we can fix more things ...
5 years, 8 months ago #5
Benjamin Peterson
Mostly, I just hope you've run with refleak detection enabled. :) http://bugs.python.org/review/2377/diff/4307/15432 File FAILING (right): ...
5 years, 8 months ago #6
eric.araujo
http://bugs.python.org/review/2377/diff/4307/15456 File Python/freeze_importlib.py (right): http://bugs.python.org/review/2377/diff/4307/15456#newcode1 Python/freeze_importlib.py:1: #! /usr/bin/env python > Can we put this in ...
5 years, 8 months ago #7
AntoinePitrou
http://bugs.python.org/review/2377/diff/4307/15458 File Python/import.c (right): http://bugs.python.org/review/2377/diff/4307/15458#newcode283 Python/import.c:283: PyErr_Print(); Bogus indentation here. http://bugs.python.org/review/2377/diff/4307/15461 File Python/pythonrun.c (right): http://bugs.python.org/review/2377/diff/4307/15461#newcode227 ...
5 years, 8 months ago #8
brett.cannon
5 years, 7 months ago #9
Still have three failures left, but they do not stem from any changes made here.

http://bugs.python.org/review/2377/diff/4307/15432
File FAILING (right):

http://bugs.python.org/review/2377/diff/4307/15432#newcode1
FAILING:1: # Code to detect import failure no longer works (relied on stack
depth).
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> The whole file should of course die before it is integrated.

Yes., of course. I will probably just generate a massive patch against default
and commit that since the revision history for this branch is not critical (and
full of branch merges anyway).

http://bugs.python.org/review/2377/diff/4307/15434
File Include/pystate.h (right):

http://bugs.python.org/review/2377/diff/4307/15434#newcode36
Include/pystate.h:36: PyObject *importlib;
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Maybe this should go up with the other import related things like sysdict,
> builtins, modules_reloading...

Done.

http://bugs.python.org/review/2377/diff/4307/15436
File Lib/importlib/machinery.py (left):

http://bugs.python.org/review/2377/diff/4307/15436#oldcode2
Lib/importlib/machinery.py:2: 
On 2012/03/13 01:00:03, twouters wrote:
> gratuitous change?

It's an artifact from when I renamed importlib._bootstrap to _importlib for a
time. I inserted the newline again.

http://bugs.python.org/review/2377/diff/4307/15441
File Lib/importlib/test/source/test_finder.py (right):

http://bugs.python.org/review/2377/diff/4307/15441#newcode4
Lib/importlib/test/source/test_finder.py:4: from importlib import _bootstrap
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Why are half of these files importing import._bootstrap and the other half
just
> _bootstrap?

Beats me. =) This code has grown somewhat organically, especially the test
suite. This can be unified, but it should be done in trunk and probably part of
moving the test suite out of Lib/importlib/test and into Lib/test/importlib.

http://bugs.python.org/review/2377/diff/4307/15444
File Lib/importlib/test/util.py (left):

http://bugs.python.org/review/2377/diff/4307/15444#oldcode38
Lib/importlib/test/util.py:38: "cannot uncache {0} as it will break
_importlib".format(name))
On 2012/03/13 01:00:03, twouters wrote:
> I suggest removing the 'as it will break importlib' part altogether. 

Done.

http://bugs.python.org/review/2377/diff/4307/15445
File Lib/os.py (right):

http://bugs.python.org/review/2377/diff/4307/15445#newcode45
Lib/os.py:45: # Any introduction of a new "_os" module and/or path separator
requires
On 2012/03/13 01:00:03, twouters wrote:
> "_os" is a little vague. How about "Any new dependencies of the os module
and/or
> changes in path separator requires updating importlib as well"?

Done.

http://bugs.python.org/review/2377/diff/4307/15446
File Lib/site.py (right):

http://bugs.python.org/review/2377/diff/4307/15446#newcode85
Lib/site.py:85: if m.__loader__.__module__ != '_frozen_importlib':
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> This condition can be folded into the if above with a getattr. Also, do you
want
> to require __loader__ to have a __module__?

Done.

http://bugs.python.org/review/2377/diff/4307/15447
File Lib/test/test_frozen.py (right):

http://bugs.python.org/review/2377/diff/4307/15447#newcode15
Lib/test/test_frozen.py:15: self.assertEqual(len(dir(__hello__)), 8,
dir(__hello__))
On 2012/03/13 01:00:03, twouters wrote:
> This should probably document why you expect 8 attributes, or (better yet)
> explicitly compare a set of attribute names.

Done.

http://bugs.python.org/review/2377/diff/4307/15453
File Makefile.pre.in (right):

http://bugs.python.org/review/2377/diff/4307/15453#newcode555
Makefile.pre.in:555: ./$(BUILDPYTHON) Python/freeze_importlib.py \
On 2012/03/13 01:00:03, twouters wrote:
> I suspect this will break obj-directory builds again. Have you tried it like
> that?

I had not. Fixed and tested by running ./configure from Objects/.

http://bugs.python.org/review/2377/diff/4307/15456
File Python/freeze_importlib.py (right):

http://bugs.python.org/review/2377/diff/4307/15456#newcode1
Python/freeze_importlib.py:1: #! /usr/bin/env python
On 2012/03/15 23:03:53, eric.araujo wrote:
> > Can we put this in Tools/scripts?
> Seconded; some modules are regenerated by themselves (e.g. token), other by
> scripts in Tools (e.g. unicodedata), and only one Python script is found in
the
> Python directory (makeopcodetargets.py).

So I had it there but I think Antoine asked me to move it, so I will wait for a
more solid answer from people on where to keep it.

http://bugs.python.org/review/2377/diff/4307/15456#newcode10
Python/freeze_importlib.py:10: with open(input_path, 'r') as input_file:
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> No encoding? :)

Done.

http://bugs.python.org/review/2377/diff/4307/15456#newcode16
Python/freeze_importlib.py:16: lines.append('unsigned char M__importlib[] = {')
On 2012/03/13 01:00:03, twouters wrote:
> This should have a _Py_ prefix.

Done.

http://bugs.python.org/review/2377/diff/4307/15456#newcode21
Python/freeze_importlib.py:21: for c in bytes(data[i:i+16]):
On 2012/03/13 01:00:03, twouters wrote:
> This bytes() call seems unnecessary considering you are running this (and you
> have to run this) with the built python. It will always be Python 3.

Done.

http://bugs.python.org/review/2377/diff/4307/15456#newcode34
Python/freeze_importlib.py:34: raise RuntimeError('need to specify input and
output file paths')
On 2012/03/15 23:03:53, eric.araujo wrote:
> > This seems rather weird use of RuntimeError.
> A suggestion: sys.exit('need to specify etc.')

Done.

http://bugs.python.org/review/2377/diff/4307/15457
File Python/frozen.c (right):

http://bugs.python.org/review/2377/diff/4307/15457#newcode35
Python/frozen.c:35: {"__hello__", M___hello__, SIZE},
On 2012/03/13 01:00:03, twouters wrote:
> These should also be fixed to use _Py or Py as a prefix, as while we're at it
:)

Done.

http://bugs.python.org/review/2377/diff/4307/15458
File Python/import.c (right):

http://bugs.python.org/review/2377/diff/4307/15458#newcode272
Python/import.c:272: goto error;
On 2012/03/13 01:00:03, twouters wrote:
> This change appears to be leaking a reference.

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode283
Python/import.c:283: PyErr_Print();
On 2012/03/16 23:55:21, AntoinePitrou wrote:
> Bogus indentation here.
> 

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2804
Python/import.c:2804: if (!(name = PyUnicode_InternFromString(# name))) \
On 2012/03/13 01:00:03, twouters wrote:
> Predominate style in the codebase is to use no space between '#' and 'name'.

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2830
Python/import.c:2830: ADD_INTERNED(__import__);
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> You should use the _Py_identifier API for all these.

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2843
Python/import.c:2843: Py_FatalError("can't create an empty dict");
On 2012/03/13 01:00:03, twouters wrote:
> Why are these fatal errors but the failures to intern, above, not?

No good reason. But I will move all of this over to _Py_IDENTIFIER usage so that
will make this a moot point.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2855
Python/import.c:2855: globals = empty_dict;
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Instead of using these static dicts, how about just calling PyDict_New() if
you
> need an empty dict every time? You could even just check for NULL in the
places
> that are using the globals and fromlist objects. It's always nicer not to have
> immortal static stuff sitting around.

Done. I originally did all of this static stuff to avoid any perf overhead since
importlib's bootstrap acceptance depended on it.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2887
Python/import.c:2887: if (package == NULL) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> If __name__ is simply not in globals, shouldn't we set an error?

That's what PyDict_GetItemWithError() does.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2892
Python/import.c:2892: if (PyDict_GetItem(globals, __path__) == NULL) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Why do we use PyDict_GetItemWithError some of the time and PyDict_GetItem
other
> times?

Sometimes lacking a key is an error, sometimes it's not. =)

http://bugs.python.org/review/2377/diff/4307/15458#newcode2893
Python/import.c:2893: PyObject *partition = PyUnicode_RPartition(package, dot);
On 2012/03/13 01:00:03, twouters wrote:
> Need to check the returnvalue of PyUnicode_RPartition() and/or the type of
> package (which you do for the __package__ case but not the __name__ case.)

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2909
Python/import.c:2909: if (PyObject_Not(name)) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> We know name is name is unicode so how about using PyUnicode_GET_LENGTH? (If
you
> do that, make sure to call PyUnicode_READY.)

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2935
Python/import.c:2935: if (PyObject_IsTrue(name)) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Once again, PyUnicode_GET_LENGTH to the rescue. Alternatively check if
last_dot
> == 0.

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2935
Python/import.c:2935: if (PyObject_IsTrue(name)) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Once again, PyUnicode_GET_LENGTH to the rescue. Alternatively check if
last_dot
> == 0.

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2936
Python/import.c:2936: PyObject *seq = PyTuple_New(2);
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> PyTuple_Pack instead?

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2960
Python/import.c:2960: if (PyDict_Check(globals)) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Haven't you already been treating it like a dictionary?

Only when level > 0. And I have to let non-dicts through; lib2to3 does a very
naughty import of setting globals to []. =)

http://bugs.python.org/review/2377/diff/4307/15458#newcode2988
Python/import.c:2988: if (PyObject_Not(fromlist)) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> PyList_GET_SIZE? I suppose you would have to force it to be a list.

Exactly.

http://bugs.python.org/review/2377/diff/4307/15458#newcode2989
Python/import.c:2989: if (level == 0 || PyObject_IsTrue(name)) {
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> PyUnicode_GET_LENGTH again.

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode3012
Python/import.c:3012: final_mod = PyDict_GetItem(interp->modules, to_return);
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> Don't we need to INCREF final_mod somewhere now?

Done.

http://bugs.python.org/review/2377/diff/4307/15458#newcode3039
Python/import.c:3039: Try to move sys.modules check as high up as possible
On 2012/03/13 01:00:03, twouters wrote:
> Get rid of this comment? Or do you still want to duplicate the sys.module
checks
> in C as well?

Removed.

http://bugs.python.org/review/2377/diff/4307/15461
File Python/pythonrun.c (right):

http://bugs.python.org/review/2377/diff/4307/15461#newcode227
Python/pythonrun.c:227: Py_FatalError("Py_Initialize: importlib install
failed");
On 2012/03/16 23:55:21, AntoinePitrou wrote:
> Uh, please print the actual error before the fatal error. It will make
debugging
> much easier.

I'm hoping PyErr_Print() will use some fallback stderr since the I/O system is
not up and running at this point.

http://bugs.python.org/review/2377/diff/4307/15461#newcode709
Python/pythonrun.c:709: import_init(interp, sysmod);
On 2012/03/15 17:18:34, Benjamin Peterson wrote:
> We don't init zipimport here?

You might be mislead by the diff as there is a call to _PyImportZip_Init() in
the actual code. But if you mean why don't I call _PyImportZip_Init(), I will
move it into import_init().
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7