PEP 3147 implementation

[Antoine Pitrou, 2010-04-04 15:55:30]

Hi Barry,

here are some comments.
I should stress than I have not tried applying the patch, nor have I tried to
check that it complied fully to the PEP.

http://codereview.appspot.com/842043/diff/1/9
File Lib/importlib/test/source/test_finder.py (right):

http://codereview.appspot.com/842043/diff/1/9#newcode60
Lib/importlib/test/source/test_finder.py:60: if error.errno != errno.ENOENT:
You should comment why it's ok to fail with ENOENT.

http://codereview.appspot.com/842043/diff/1/6
File Lib/importlib/test/source/util.py (right):

http://codereview.appspot.com/842043/diff/1/6#newcode30
Lib/importlib/test/source/util.py:30: def bytecode_path(source_path):
Shouldn't it be removed, if it's just an empty wrapper?

http://codereview.appspot.com/842043/diff/1/6#newcode34
Lib/importlib/test/source/util.py:34: def ensure_bytecode_path(bytecode_path):
How about a docstring?

http://codereview.appspot.com/842043/diff/1/5
File Lib/py_compile.py (right):

http://codereview.appspot.com/842043/diff/1/5#newcode78
Lib/py_compile.py:78: """Byte-compile one Python source file to Python bytecode.
Perhaps compile() should now return the path of the compiled file, since it's
not as easy to derive from the source file than it was before?

http://codereview.appspot.com/842043/diff/1/15
File Lib/test/script_helper.py (right):

http://codereview.appspot.com/842043/diff/1/15#newcode70
Lib/test/script_helper.py:70: return cache_from_source(script_name)
See previous comment: let py_compile.compile() return the path to the cached
file?

http://codereview.appspot.com/842043/diff/1/22
File Lib/test/support.py (right):

http://codereview.appspot.com/842043/diff/1/22#newcode192
Lib/test/support.py:192: """Turn the PEP 3147 pyc into a bytecode-only pyc."""
What does "bytecode-only" mean here? Isn't a PEP 3147 pyc bytecode-only as well?

http://codereview.appspot.com/842043/diff/1/22#newcode204
Lib/test/support.py:204: unlink(imp.cache_from_source(os.path.join(dirname,
modname + '.py')))
I think it should still try to unlink the legacy pyc/pyo files.

http://codereview.appspot.com/842043/diff/1/16
File Lib/test/test_imp.py (right):

http://codereview.appspot.com/842043/diff/1/16#newcode200
Lib/test/test_imp.py:200: self.assertEqual(imp.magic_tags[b'X\x0c\r\n'],
'cpython-32')
It would be nice if this test were parametered using sys.version, or
sys.version_info. It's no fun having to fix version numbers manually :)

(even better if the "cpython" part can be derived from a sys member, but I don't
think we have a sys.implementation yet)

http://codereview.appspot.com/842043/diff/1/16#newcode233
Lib/test/test_imp.py:233:
self.assertEqual(imp.source_from_cache('/foo/bar/bazqux.pyc'), None)
Wouldn't it be better to raise a ValueError for bad paths?

http://codereview.appspot.com/842043/diff/1/16#newcode255
Lib/test/test_imp.py:255: os.mkdir('pep3147')
You can use `with support.temp_cwd('pep3147')` instead.

http://codereview.appspot.com/842043/diff/1/16#newcode257
Lib/test/test_imp.py:257: open('pep3147/__init__.py', 'w').close()
This is wrong on platforms without refcounting. You should use 'with' instead.

http://codereview.appspot.com/842043/diff/1/16#newcode264
Lib/test/test_imp.py:264: del sys.path[0]
This will wreck sys.path if writing to pep3147/__init__.py failed. Not that it's
a frequent condition of course.

http://codereview.appspot.com/842043/diff/1/21
File Lib/test/test_import.py (right):

http://codereview.appspot.com/842043/diff/1/21#newcode34
Lib/test/test_import.py:34: def umask(umask):
You could open a separate issue for temp_umask() to be added to test.support.

http://codereview.appspot.com/842043/diff/1/21#newcode526
Lib/test/test_import.py:526: def tearDown(self):
tearDown() should also remove self.source.

http://codereview.appspot.com/842043/diff/1/21#newcode541
Lib/test/test_import.py:541: with umask(0o222):
Does it work under Windows? Or should the test be POSIX-specific?

http://codereview.appspot.com/842043/diff/1/21#newcode563
Lib/test/test_import.py:563: pyc_file = make_legacy_pyc(self.source)
There should be a try..finally to remove the pyc_file.

http://codereview.appspot.com/842043/diff/1/18
File Lib/test/test_pydoc.py (right):

http://codereview.appspot.com/842043/diff/1/18#newcode187
Lib/test/test_pydoc.py:187: """.strip() # ' <- emacs turd
Hmm, well, why not.

http://codereview.appspot.com/842043/diff/1/3
File Python/import.c (right):

http://codereview.appspot.com/842043/diff/1/3#newcode101
Python/import.c:101: static char *pyc_tag = TAG;
I think this should be a "const char *" instead.

http://codereview.appspot.com/842043/diff/1/3#newcode102
Python/import.c:102: static char *pyc_dir = CACHEDIR;
This one doesn't seem to have any point, since "__pycache__" is hardwired
everywhere. Just keep the macro.

http://codereview.appspot.com/842043/diff/1/3#newcode115
Python/import.c:115: static struct pymagics _PyMagicNumberTags[] = {
This is going to be painful to update and maintain.
Perhaps we can find another approach (a private header file auto-updated by a
Python script?).

http://codereview.appspot.com/842043/diff/1/3#newcode774
Python/import.c:774: char *pos;
Are you mixing tabs and spaces for indentation?

http://codereview.appspot.com/842043/diff/1/3#newcode781
Python/import.c:781: if (len + strlen(CACHEDIR) + strlen(pyc_tag) + 4 >= buflen)
Let's count:
- 1 slash after pycache
- 2 extra dots
- 1 extra trailing character ('c' or 'o')
- 1 NUL character

I think it makes 5 :-)
Or perhaps there's only one extra dot?

http://codereview.appspot.com/842043/diff/1/3#newcode787
Python/import.c:787: if ((pos = strrchr(pathname, SEP)) == NULL) {
You should search for ALTSEP too (and keep the right-most one of both).
(even better if you can add tests for mixed separators under Windows)

http://codereview.appspot.com/842043/diff/1/3#newcode806
Python/import.c:806: if ((pos = strrchr(buf, '.')) != NULL)
What if pos is NULL?

http://codereview.appspot.com/842043/diff/1/3#newcode809
Python/import.c:809: strcat(buf, Py_OptimizeFlag ? ".pyo" : ".pyc");
If you have removed less than 3 characters, can't it lead to a buffer overflow?

http://codereview.appspot.com/842043/diff/1/3#newcode832
Python/import.c:832: if ((right = strrchr(pathname, SEP)) == NULL)
See comments about ALTSEP above.
A custom while loop would probably be better than all the strrchr dance.

http://codereview.appspot.com/842043/diff/1/3#newcode841
Python/import.c:841: if (right-left != 11 || strncmp(left, "__pycache__",
right-left) != 0)
You already have a macro and a static char *, and now you're hardwiring
__pycache__. Hum :))

http://codereview.appspot.com/842043/diff/1/3#newcode1032
Python/import.c:1032: dirpath = strrchr(cpathname, '/');
SEP? ALTSEP?
(same for two other lines below)

http://codereview.appspot.com/842043/diff/1/3#newcode1040
Python/import.c:1040: if (mkdir(cpathname, dirmode) < 0 && errno != EEXIST) {
Looking at posixmodule.c, Windows might have to use CreateDirectoryA() instead.

http://codereview.appspot.com/842043/diff/1/3#newcode1046
Python/import.c:1046: }
I think you should clear errno.

http://codereview.appspot.com/842043/diff/1/3#newcode1214
Python/import.c:1214: if (make_source_pathname((char *)file, py) == NULL) {
The cast to "char *" is obviously worrying.

http://codereview.appspot.com/842043/diff/1/3#newcode3341
Python/import.c:3341: /* Hack the Py_OptimizeFlag based on whether __debug__ is
forced or
make_compiled_pathname() is an internal function. Can't its signature just be
changed, rather than this horrible hack?

http://codereview.appspot.com/842043/diff/1/3#newcode3362
Python/import.c:3362: return NULL;
You should raise an error here.
I suppose there are no tests for this :)

http://codereview.appspot.com/842043/diff/1/3#newcode3370
Python/import.c:3370: .pyc/.py file calculated as if the .py file were imported.
 The extension\n\
".pyc/.py" => ".pyc/.pyo"?

http://codereview.appspot.com/842043/diff/1/3#newcode3371
Python/import.c:3371: will be .pyc unless __debug__ is defined, then it will be
.pyo.\n\
"unless __debug__ is defined" => "unless __debug__ is undefined", no?

http://codereview.appspot.com/842043/diff/1/4
File Python/importdl.h (right):

http://codereview.appspot.com/842043/diff/1/4#newcode35
Python/importdl.h:35: struct pymagics {
Why is this exported? It doesn't seem to be used anywhere else.

[barry, 2010-04-06 23:05:09]

Thanks for the great review Antoine.  I will have a new patchset to upload based
on these and other previous changes I've made to the branch.

http://codereview.appspot.com/842043/diff/1/9
File Lib/importlib/test/source/test_finder.py (right):

http://codereview.appspot.com/842043/diff/1/9#newcode60
Lib/importlib/test/source/test_finder.py:60: if error.errno != errno.ENOENT:
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> You should comment why it's ok to fail with ENOENT.
> 

When a test doesn't set compile_=True, there will be no PEP 3147 pyc file to
rename.  I've added a comment.

http://codereview.appspot.com/842043/diff/1/6
File Lib/importlib/test/source/util.py (right):

http://codereview.appspot.com/842043/diff/1/6#newcode30
Lib/importlib/test/source/util.py:30: def bytecode_path(source_path):
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Shouldn't it be removed, if it's just an empty wrapper?
> 

Done.

http://codereview.appspot.com/842043/diff/1/6#newcode34
Lib/importlib/test/source/util.py:34: def ensure_bytecode_path(bytecode_path):
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> How about a docstring?

Done.

http://codereview.appspot.com/842043/diff/1/5
File Lib/py_compile.py (right):

http://codereview.appspot.com/842043/diff/1/5#newcode78
Lib/py_compile.py:78: """Byte-compile one Python source file to Python bytecode.
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Perhaps compile() should now return the path of the compiled file, since it's
> not as easy to derive from the source file than it was before?
> 

Good idea.  Done.

http://codereview.appspot.com/842043/diff/1/15
File Lib/test/script_helper.py (right):

http://codereview.appspot.com/842043/diff/1/15#newcode70
Lib/test/script_helper.py:70: return cache_from_source(script_name)
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> See previous comment: let py_compile.compile() return the path to the cached
> file?
> 

I'll do you one better :).  We can remove compile_script() entirely and just
change the call sites to py_compile.compile(..., doraise=True)

http://codereview.appspot.com/842043/diff/1/22
File Lib/test/support.py (right):

http://codereview.appspot.com/842043/diff/1/22#newcode192
Lib/test/support.py:192: """Turn the PEP 3147 pyc into a bytecode-only pyc."""
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> What does "bytecode-only" mean here? Isn't a PEP 3147 pyc bytecode-only as
well?
> 

I've reworked this docstring.

http://codereview.appspot.com/842043/diff/1/22#newcode204
Lib/test/support.py:204: unlink(imp.cache_from_source(os.path.join(dirname,
modname + '.py')))
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> I think it should still try to unlink the legacy pyc/pyo files.
> 

I don't think there's a case in the test suite where the legacy pyc/pyo file
exists.  I suppose it can't hurt though.

http://codereview.appspot.com/842043/diff/1/16
File Lib/test/test_imp.py (right):

http://codereview.appspot.com/842043/diff/1/16#newcode200
Lib/test/test_imp.py:200: self.assertEqual(imp.magic_tags[b'X\x0c\r\n'],
'cpython-32')
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> It would be nice if this test were parametered using sys.version, or
> sys.version_info. It's no fun having to fix version numbers manually :)
> 
> (even better if the "cpython" part can be derived from a sys member, but I
don't
> think we have a sys.implementation yet)
> 

This test actually tests all previous magic tags.  Since we'll only ever append
to that set in future versions, this test will not need to be modified (but it
should be appended to).  test_magic_tags() is what tests the current tag.

I've added a comment to clarify.

http://codereview.appspot.com/842043/diff/1/16#newcode233
Lib/test/test_imp.py:233:
self.assertEqual(imp.source_from_cache('/foo/bar/bazqux.pyc'), None)
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Wouldn't it be better to raise a ValueError for bad paths?
> 

I went back and forth on this one, so I'm okay with making it a ValueError. 
Note that imp.cache_from_source() does not really validate its input.  It'll
always give an answer, though it might be nonsense if the path isn't to an
actual .py file (it'll never overrun the buffer or anything like that of
course).  I'm not sure it's worth adding validating to cache_from_source()
though.

http://codereview.appspot.com/842043/diff/1/16#newcode255
Lib/test/test_imp.py:255: os.mkdir('pep3147')
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> You can use `with support.temp_cwd('pep3147')` instead.

Except that I don't want to cd to pep3147/ which that function also does.

http://codereview.appspot.com/842043/diff/1/16#newcode257
Lib/test/test_imp.py:257: open('pep3147/__init__.py', 'w').close()
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> This is wrong on platforms without refcounting. You should use 'with' instead.

Done.

http://codereview.appspot.com/842043/diff/1/16#newcode264
Lib/test/test_imp.py:264: del sys.path[0]
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> This will wreck sys.path if writing to pep3147/__init__.py failed. Not that
it's
> a frequent condition of course.

I put that in a "if sys.path[0] == os.curdir" test.

http://codereview.appspot.com/842043/diff/1/21
File Lib/test/test_import.py (right):

http://codereview.appspot.com/842043/diff/1/21#newcode34
Lib/test/test_import.py:34: def umask(umask):
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> You could open a separate issue for temp_umask() to be added to test.support.
> 

Or I'll just move it to test.support now <wink>

http://codereview.appspot.com/842043/diff/1/21#newcode526
Lib/test/test_import.py:526: def tearDown(self):
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> tearDown() should also remove self.source.
> 

_rmcache() does that, but it's less that obvious.  I cleaned that up.

http://codereview.appspot.com/842043/diff/1/21#newcode541
Lib/test/test_import.py:541: with umask(0o222):
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Does it work under Windows? Or should the test be POSIX-specific?
> 

Good catch.  I wrapped that in a @unittest.skipUnless() like
test_execute_bit_not_copied().

http://codereview.appspot.com/842043/diff/1/21#newcode563
Lib/test/test_import.py:563: pyc_file = make_legacy_pyc(self.source)
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> There should be a try..finally to remove the pyc_file.
> 

Done.

http://codereview.appspot.com/842043/diff/1/18
File Lib/test/test_pydoc.py (right):

http://codereview.appspot.com/842043/diff/1/18#newcode187
Lib/test/test_pydoc.py:187: """.strip() # ' <- emacs turd
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Hmm, well, why not.
> 

Stupid python-mode gets font-lock flummoxed.

http://codereview.appspot.com/842043/diff/1/3
File Python/import.c (right):

http://codereview.appspot.com/842043/diff/1/3#newcode101
Python/import.c:101: static char *pyc_tag = TAG;
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> I think this should be a "const char *" instead.

Done.

http://codereview.appspot.com/842043/diff/1/3#newcode102
Python/import.c:102: static char *pyc_dir = CACHEDIR;
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> This one doesn't seem to have any point, since "__pycache__" is hardwired
> everywhere. Just keep the macro.
> 

Done.

http://codereview.appspot.com/842043/diff/1/3#newcode115
Python/import.c:115: static struct pymagics _PyMagicNumberTags[] = {
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> This is going to be painful to update and maintain.
> Perhaps we can find another approach (a private header file auto-updated by a
> Python script?).
> 

I think it's better to keep everything together, but I have added more
documentation to the magic number table comment, and around the relevant
definitions.

http://codereview.appspot.com/842043/diff/1/3#newcode774
Python/import.c:774: char *pos;
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Are you mixing tabs and spaces for indentation?
> 

Done.

http://codereview.appspot.com/842043/diff/1/3#newcode781
Python/import.c:781: if (len + strlen(CACHEDIR) + strlen(pyc_tag) + 4 >= buflen)
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Let's count:
> - 1 slash after pycache
> - 2 extra dots
> - 1 extra trailing character ('c' or 'o')
> - 1 NUL character
> 
> I think it makes 5 :-)
> Or perhaps there's only one extra dot?
> 

You're right that that makes 5.  What's also confusing is the >= test.  I think
the code should read:

if (len + strlen(CACHEDIR) + strlen(pyc_tag) + 5 > buflen)
    return NULL;

Let's look at example.  Say we have 'foo.py', that's strlen == 6. 
strlen('__pycache__') == 11 and strlen('cpython-32') is 10.  Plus 5 (slash, two
dots, c, and NUL) == a total buffer requirement of 32.  If buflen==32 we're
good, if buflen == 31, no good.

The old code only added 4 so the sum would have been 31.  With >= if buflen were
31 (i.e. ==) we would have failed, but with buflen==32 it would be okay.

Still, I think you're right that we should add 5 and use a > test for clarity.

http://codereview.appspot.com/842043/diff/1/3#newcode787
Python/import.c:787: if ((pos = strrchr(pathname, SEP)) == NULL) {
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> You should search for ALTSEP too (and keep the right-most one of both).
> (even better if you can add tests for mixed separators under Windows)

Curse your ALTSEP!  Well, I have some code, but no way right now to test it on
Windows.  I agree it should be tested under Windows with os.altsep, though I
don't really have an environment I can test that on.  I'll write some tests and
ask for help.

http://codereview.appspot.com/842043/diff/1/3#newcode806
Python/import.c:806: if ((pos = strrchr(buf, '.')) != NULL)
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> What if pos is NULL?

The strcat will append a terminating NUL and you'll get a file name like this:

>>> imp.cache_from_source('foo')
'__pycache__/foocpython-32.pyc'

which is might be nonsense, but won't crash. :)

http://codereview.appspot.com/842043/diff/1/3#newcode809
Python/import.c:809: strcat(buf, Py_OptimizeFlag ? ".pyo" : ".pyc");
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> If you have removed less than 3 characters, can't it lead to a buffer
overflow?

I don't think so.  Doesn't the space calculation guarantee we'll have enough
room?

http://codereview.appspot.com/842043/diff/1/3#newcode832
Python/import.c:832: if ((right = strrchr(pathname, SEP)) == NULL)
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> See comments about ALTSEP above.
> A custom while loop would probably be better than all the strrchr dance.
> 

See above.

http://codereview.appspot.com/842043/diff/1/3#newcode841
Python/import.c:841: if (right-left != 11 || strncmp(left, "__pycache__",
right-left) != 0)
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> You already have a macro and a static char *, and now you're hardwiring
> __pycache__. Hum :))
> 

Done.

http://codereview.appspot.com/842043/diff/1/3#newcode1032
Python/import.c:1032: dirpath = strrchr(cpathname, '/');
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> SEP? ALTSEP?
> (same for two other lines below)

Resolved as above.

http://codereview.appspot.com/842043/diff/1/3#newcode1040
Python/import.c:1040: if (mkdir(cpathname, dirmode) < 0 && errno != EEXIST) {
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Looking at posixmodule.c, Windows might have to use CreateDirectoryA()
instead.
> 

Maybe I should just arrange to call os.mkdir()?

I've marked this with an XXX to come back to later.

http://codereview.appspot.com/842043/diff/1/3#newcode1046
Python/import.c:1046: }
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> I think you should clear errno.
> 

Hmm.  If you look at the following code, open_exclusive() can set errno, but
that's not cleared here.  Maybe it's more right to do it in both cases, but I'd
rather stay consistent with existing code.

http://codereview.appspot.com/842043/diff/1/3#newcode1214
Python/import.c:1214: if (make_source_pathname((char *)file, py) == NULL) {
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> The cast to "char *" is obviously worrying.
> 

It shuts up the compiler and is probably safe.  OTOH, there's no real reason why
get_sourcefile() needs to take a const char*, so I'll just demote that parameter
and remove the cast.

http://codereview.appspot.com/842043/diff/1/3#newcode3341
Python/import.c:3341: /* Hack the Py_OptimizeFlag based on whether __debug__ is
forced or
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> make_compiled_pathname() is an internal function. Can't its signature just be
> changed, rather than this horrible hack?
> 

Done.

http://codereview.appspot.com/842043/diff/1/3#newcode3362
Python/import.c:3362: return NULL;
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> You should raise an error here.
> I suppose there are no tests for this :)

Yep, already added a SystemError for this one. :)

http://codereview.appspot.com/842043/diff/1/3#newcode3370
Python/import.c:3370: .pyc/.py file calculated as if the .py file were imported.
 The extension\n\
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> ".pyc/.py" => ".pyc/.pyo"?

Done.

http://codereview.appspot.com/842043/diff/1/3#newcode3371
Python/import.c:3371: will be .pyc unless __debug__ is defined, then it will be
.pyo.\n\
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> "unless __debug__ is defined" => "unless __debug__ is undefined", no?
> 

Done.

http://codereview.appspot.com/842043/diff/1/4
File Python/importdl.h (right):

http://codereview.appspot.com/842043/diff/1/4#newcode35
Python/importdl.h:35: struct pymagics {
On 2010/04/04 15:55:30, Antoine Pitrou wrote:
> Why is this exported? It doesn't seem to be used anywhere else.
> 

I thought other code might want to get at it.  OTOH, YAGNI.

[Benjamin, 2010-04-07 02:50:04]

Looks pretty good to me. I basically only picked on style things.

I think it would be nice, though, to have a central location for the current
pyc/pyo mode. I see this a lot:

pyc_file = ".pyc" if __debug__ else ".pyo"

Could this be standardized in test/support.py?

http://codereview.appspot.com/842043/diff/13001/14009
File Lib/importlib/_bootstrap.py (right):

http://codereview.appspot.com/842043/diff/13001/14009#newcode494
Lib/importlib/_bootstrap.py:494: return (bytecode_path if
_path_exists(bytecode_path) else None)
Are these parenthesis a common style? I've never seen them before on the
conditional operator.

http://codereview.appspot.com/842043/diff/13001/14008
File Lib/importlib/test/__main__.py (right):

http://codereview.appspot.com/842043/diff/13001/14008#newcode17
Lib/importlib/test/__main__.py:17: from os import sep
os (in the form of os.path) is already imported at the top of the file.

http://codereview.appspot.com/842043/diff/13001/14010
File Lib/importlib/util.py (right):

http://codereview.appspot.com/842043/diff/13001/14010#newcode2
Lib/importlib/util.py:2: 
What's this?

http://codereview.appspot.com/842043/diff/13001/14003
File Lib/py_compile.py (right):

http://codereview.appspot.com/842043/diff/13001/14003#newcode15
Lib/py_compile.py:15: from imp import cache_from_source
What's the benefit on having this in the module namespace when imp is already
imported above?

http://codereview.appspot.com/842043/diff/13001/14003#newcode80
Lib/py_compile.py:80: :param file: The source file name.
Doc/library/py_compile.rst should be updated, too.

http://codereview.appspot.com/842043/diff/13001/14014
File Lib/test/script_helper.py (right):

http://codereview.appspot.com/842043/diff/13001/14014#newcode14
Lib/test/script_helper.py:14: from imp import cache_from_source,
source_from_cache
cache_from_source isn't actually used anywhere.

http://codereview.appspot.com/842043/diff/13001/14019
File Lib/test/test_cmd_line_script.py (right):

http://codereview.appspot.com/842043/diff/13001/14019#newcode7
Lib/test/test_cmd_line_script.py:7: import py_compile
Generally stdlib imports are above test.support ones.

http://codereview.appspot.com/842043/diff/13001/14013
File Lib/test/test_compileall.py (right):

http://codereview.appspot.com/842043/diff/13001/14013#newcode68
Lib/test/test_compileall.py:68: self.assertTrue(os.path.isfile(self.bc_path)
As long as you're changing style you may as well put the "and" on this line as
PEP 8 says.

http://codereview.appspot.com/842043/diff/13001/14015
File Lib/test/test_imp.py (right):

http://codereview.appspot.com/842043/diff/13001/14015#newcode290
Lib/test/test_imp.py:290: try:
You can use TestCase.addCleanup instead of this try, finally block.

http://codereview.appspot.com/842043/diff/13001/14015#newcode297
Lib/test/test_imp.py:297: del sys.modules['pep3147']
Isn't this what support.forget() is for?

http://codereview.appspot.com/842043/diff/13001/14020
File Lib/test/test_import.py (right):

http://codereview.appspot.com/842043/diff/13001/14020#newcode556
Lib/test/test_import.py:556: try:
Once again, addCleanup to the rescue!

http://codereview.appspot.com/842043/diff/13001/14002
File Python/import.c (right):

http://codereview.appspot.com/842043/diff/13001/14002#newcode782
Python/import.c:782: static char *rightmost_sep(const char *s)
The convention seems to have the return type on its own line before the function
name.

http://codereview.appspot.com/842043/diff/13001/14002#newcode782
Python/import.c:782: static char *rightmost_sep(const char *s)
I don't know if it's worth bothering with const if you're just going to cast
back to (char *) at the end.

http://codereview.appspot.com/842043/diff/13001/14002#newcode791
Python/import.c:791: {
The brace if it is here at all should be right after the parenthesis.

http://codereview.appspot.com/842043/diff/13001/14002#newcode819
Python/import.c:819: return NULL;
Shouldn't make_compiled_pathname set an error here? I see a caller does, but
that seems fragile.

http://codereview.appspot.com/842043/diff/13001/14002#newcode821
Python/import.c:821: /* Find the last path separator and copy everything from
the start of
What if the pathname is "/"? Then you'll end up inserting something in the cwd.

http://codereview.appspot.com/842043/diff/13001/14002#newcode885
Python/import.c:885: if ((dot0 = strchr(right+1, '.')) == NULL)
I think there should be space between "+" and operands.

http://codereview.appspot.com/842043/diff/13001/14002#newcode1061
Python/import.c:1061: mode_t dirmode = srcstat->st_mode | S_IEXEC; /* XXX */
Perhaps you could expand on this XXX.

http://codereview.appspot.com/842043/diff/13001/14002#newcode3369
Python/import.c:3369: imp_cache_from_source(PyObject *self, PyObject *args,
PyObject *kws)
These functions needs documentation in Doc/library/imp.rst.

http://codereview.appspot.com/842043/diff/13001/14002#newcode3371
Python/import.c:3371: static char *kwlist[] = {"path", "force__debug__", NULL};
I think the argument should either be called force___debug__ or something less
ugly. force__debug__ is just strangely inconsistent.

http://codereview.appspot.com/842043/diff/13001/14002#newcode3383
Python/import.c:3383: if (force__debug__ != Py_None) {
Why not just use PyObject_IsTrueBool?

http://codereview.appspot.com/842043/diff/13001/14002#newcode3695
Python/import.c:3695: if (PyModule_AddObject(m, "magic_tags", magics) < 0) goto
failure;
This attribute needs to be documented, too.

[brett.cannon, 2010-04-07 20:19:02]

Just reviewed the importlib stuff.

http://codereview.appspot.com/842043/diff/13001/14009
File Lib/importlib/_bootstrap.py (right):

http://codereview.appspot.com/842043/diff/13001/14009#newcode491
Lib/importlib/_bootstrap.py:491: def _find_path(self, ext_type):
The docstring is missing.

http://codereview.appspot.com/842043/diff/13001/14009#newcode493
Lib/importlib/_bootstrap.py:493: bytecode_path =
imp.cache_from_source(self._base_path + '.py')
You hard-coded the source file extension; use imp.PY_SOURCE instead and search
for all possibile extensions (could be more than one).

http://codereview.appspot.com/842043/diff/13001/14009#newcode494
Lib/importlib/_bootstrap.py:494: return (bytecode_path if
_path_exists(bytecode_path) else None)
On 2010/04/07 02:50:04, Benjamin wrote:
> Are these parenthesis a common style? I've never seen them before on the
> conditional operator.

Must be a FLUFL, but that doesn't make it PEP 8 compliant. =)

http://codereview.appspot.com/842043/diff/13001/14004
File Lib/importlib/test/source/util.py (right):

http://codereview.appspot.com/842043/diff/13001/14004#newcode33
Lib/importlib/test/source/util.py:33: :param bytecode_path: File system path to
PEP 3147 pyc file.
What the heck is this? If you want to specify this you should follow PEP 257 or
287.

Plus you are missing the trailing newline for a multi-line docstring as per your
own PEP 8. =)

[barry, 2010-04-08 12:53:47]

Thanks for the review guys.  I'll be uploading a new patch set soon.

http://codereview.appspot.com/842043/diff/13001/14009
File Lib/importlib/_bootstrap.py (right):

http://codereview.appspot.com/842043/diff/13001/14009#newcode491
Lib/importlib/_bootstrap.py:491: def _find_path(self, ext_type):
On 2010/04/07 20:19:02, brett.cannon wrote:
> The docstring is missing.

Done.

http://codereview.appspot.com/842043/diff/13001/14009#newcode493
Lib/importlib/_bootstrap.py:493: bytecode_path =
imp.cache_from_source(self._base_path + '.py')
On 2010/04/07 20:19:02, brett.cannon wrote:
> You hard-coded the source file extension; use imp.PY_SOURCE instead and search
> for all possibile extensions (could be more than one).

It actually doesn't matter:

>>> from imp import *
>>> cache_from_source('baz.xx')
'__pycache__/baz.cpython-32.pyc'

I've added a comment explaining this.

http://codereview.appspot.com/842043/diff/13001/14009#newcode494
Lib/importlib/_bootstrap.py:494: return (bytecode_path if
_path_exists(bytecode_path) else None)
On 2010/04/07 02:50:04, Benjamin wrote:
> Are these parenthesis a common style? I've never seen them before on the
> conditional operator.

It's become common style in the projects I've worked on because it's felt to
improve readability.  YMMV and I suppose this is a useful conversation to have
(perhaps for a PEP 8 update).

http://codereview.appspot.com/842043/diff/13001/14009#newcode494
Lib/importlib/_bootstrap.py:494: return (bytecode_path if
_path_exists(bytecode_path) else None)
On 2010/04/07 20:19:02, brett.cannon wrote:
> On 2010/04/07 02:50:04, Benjamin wrote:
> > Are these parenthesis a common style? I've never seen them before on the
> > conditional operator.
> 
> Must be a FLUFL, but that doesn't make it PEP 8 compliant. =)

But it's not actually non-compliant <wink>

http://codereview.appspot.com/842043/diff/13001/14008
File Lib/importlib/test/__main__.py (right):

http://codereview.appspot.com/842043/diff/13001/14008#newcode17
Lib/importlib/test/__main__.py:17: from os import sep
On 2010/04/07 02:50:04, Benjamin wrote:
> os (in the form of os.path) is already imported at the top of the file.

Done.

http://codereview.appspot.com/842043/diff/13001/14004
File Lib/importlib/test/source/util.py (right):

http://codereview.appspot.com/842043/diff/13001/14004#newcode33
Lib/importlib/test/source/util.py:33: :param bytecode_path: File system path to
PEP 3147 pyc file.
On 2010/04/07 20:19:02, brett.cannon wrote:
> What the heck is this? If you want to specify this you should follow PEP 257
or
> 287.

This is reStructuredText style epydoc API documentation.

http://epydoc.sourceforge.net/epydoc.html#epydoc-fields

We should standardize on this, as many of the projects I've worked on do.  I
believe this is compliant with PEP 287 (see "Markup for "tagged lists" or "label
lists": field lists.") 

> Plus you are missing the trailing newline for a multi-line docstring as per
your
> own PEP 8. =)

Well, that's a BDFL-ism I've never subscribed to.  And PEP 8 does say
"preferably" which doesn't seem like a requirement to me :).

http://codereview.appspot.com/842043/diff/13001/14010
File Lib/importlib/util.py (right):

http://codereview.appspot.com/842043/diff/13001/14010#newcode2
Lib/importlib/util.py:2: 
On 2010/04/07 02:50:04, Benjamin wrote:
> What's this?

An extra blank line to separate the module docstring from the import statements
for improved readability?  Maybe not strictly PEP 8, but within the spirit I
think.

http://codereview.appspot.com/842043/diff/13001/14003
File Lib/py_compile.py (right):

http://codereview.appspot.com/842043/diff/13001/14003#newcode15
Lib/py_compile.py:15: from imp import cache_from_source
On 2010/04/07 02:50:04, Benjamin wrote:
> What's the benefit on having this in the module namespace when imp is already
> imported above?

Done.

http://codereview.appspot.com/842043/diff/13001/14003#newcode80
Lib/py_compile.py:80: :param file: The source file name.
On 2010/04/07 02:50:04, Benjamin wrote:
> Doc/library/py_compile.rst should be updated, too.

I've been putting off updating the docs, but in this case I've made the change.

http://codereview.appspot.com/842043/diff/13001/14014
File Lib/test/script_helper.py (right):

http://codereview.appspot.com/842043/diff/13001/14014#newcode14
Lib/test/script_helper.py:14: from imp import cache_from_source,
source_from_cache
On 2010/04/07 02:50:04, Benjamin wrote:
> cache_from_source isn't actually used anywhere.

Done.

http://codereview.appspot.com/842043/diff/13001/14019
File Lib/test/test_cmd_line_script.py (right):

http://codereview.appspot.com/842043/diff/13001/14019#newcode7
Lib/test/test_cmd_line_script.py:7: import py_compile
On 2010/04/07 02:50:04, Benjamin wrote:
> Generally stdlib imports are above test.support ones.

Done.

http://codereview.appspot.com/842043/diff/13001/14013
File Lib/test/test_compileall.py (right):

http://codereview.appspot.com/842043/diff/13001/14013#newcode68
Lib/test/test_compileall.py:68: self.assertTrue(os.path.isfile(self.bc_path)
On 2010/04/07 02:50:04, Benjamin wrote:
> As long as you're changing style you may as well put the "and" on this line as
> PEP 8 says.

Done.

http://codereview.appspot.com/842043/diff/13001/14015
File Lib/test/test_imp.py (right):

http://codereview.appspot.com/842043/diff/13001/14015#newcode290
Lib/test/test_imp.py:290: try:
On 2010/04/07 02:50:04, Benjamin wrote:
> You can use TestCase.addCleanup instead of this try, finally block.

Done.

http://codereview.appspot.com/842043/diff/13001/14015#newcode297
Lib/test/test_imp.py:297: del sys.modules['pep3147']
On 2010/04/07 02:50:04, Benjamin wrote:
> Isn't this what support.forget() is for?

Done.

http://codereview.appspot.com/842043/diff/13001/14020
File Lib/test/test_import.py (right):

http://codereview.appspot.com/842043/diff/13001/14020#newcode556
Lib/test/test_import.py:556: try:
On 2010/04/07 02:50:04, Benjamin wrote:
> Once again, addCleanup to the rescue!

Done.

http://codereview.appspot.com/842043/diff/13001/14002
File Python/import.c (right):

http://codereview.appspot.com/842043/diff/13001/14002#newcode782
Python/import.c:782: static char *rightmost_sep(const char *s)
On 2010/04/07 02:50:04, Benjamin wrote:
> The convention seems to have the return type on its own line before the
function
> name.

Done.

http://codereview.appspot.com/842043/diff/13001/14002#newcode782
Python/import.c:782: static char *rightmost_sep(const char *s)
On 2010/04/07 02:50:04, Benjamin wrote:
> The convention seems to have the return type on its own line before the
function
> name.

Done.

http://codereview.appspot.com/842043/diff/13001/14002#newcode791
Python/import.c:791: {
On 2010/04/07 02:50:04, Benjamin wrote:
> The brace if it is here at all should be right after the parenthesis.

I tried it both ways, but because of the intervening #ifdef, it looks worse to
me to hang the brace after the close paren.  Neither one looks particularly
good, but brace-on-separate-line looks less ugly to me in this case.

http://codereview.appspot.com/842043/diff/13001/14002#newcode819
Python/import.c:819: return NULL;
On 2010/04/07 02:50:04, Benjamin wrote:
> Shouldn't make_compiled_pathname set an error here? I see a caller does, but
> that seems fragile.

I'm not so sure about this.  m_c_p() is used in two places, one where the caller
is a Python function (where you see the exception being set) and the other is in
load_source_module().  If m_c_p() returns NULL inside l_s_m() then
parse_source_module() will be called and ISTM that that function would have to
clear the exception.  So it seemed better to me to only set the exception where
it will actually be used and propagated to Python.

http://codereview.appspot.com/842043/diff/13001/14002#newcode821
Python/import.c:821: /* Find the last path separator and copy everything from
the start of
On 2010/04/07 02:50:04, Benjamin wrote:
> What if the pathname is "/"? Then you'll end up inserting something in the
cwd.

>>> imp.cache_from_source('/')
'/__pycache__/cpython-32.pyc'

which is nonsense, but doesn't go into cwd. :)

>>> imp.cache_from_source('/foo.py')
'/__pycache__/foo.cpython-32.pyc'


Ah, that's more like it.

http://codereview.appspot.com/842043/diff/13001/14002#newcode885
Python/import.c:885: if ((dot0 = strchr(right+1, '.')) == NULL)
On 2010/04/07 02:50:04, Benjamin wrote:
> I think there should be space between "+" and operands.

Done.

http://codereview.appspot.com/842043/diff/13001/14002#newcode1061
Python/import.c:1061: mode_t dirmode = srcstat->st_mode | S_IEXEC; /* XXX */
On 2010/04/07 02:50:04, Benjamin wrote:
> Perhaps you could expand on this XXX.

I expanded the comment.  Basically I'm unsure if this is correct for Windows.

http://codereview.appspot.com/842043/diff/13001/14002#newcode3369
Python/import.c:3369: imp_cache_from_source(PyObject *self, PyObject *args,
PyObject *kws)
On 2010/04/07 02:50:04, Benjamin wrote:
> These functions needs documentation in Doc/library/imp.rst.

Done.

http://codereview.appspot.com/842043/diff/13001/14002#newcode3371
Python/import.c:3371: static char *kwlist[] = {"path", "force__debug__", NULL};
On 2010/04/07 02:50:04, Benjamin wrote:
> I think the argument should either be called force___debug__ or something less
> ugly. force__debug__ is just strangely inconsistent.

How about `debug_override`?

http://codereview.appspot.com/842043/diff/13001/14002#newcode3383
Python/import.c:3383: if (force__debug__ != Py_None) {
On 2010/04/07 02:50:04, Benjamin wrote:
> Why not just use PyObject_IsTrueBool?

You mean PyObject_IsTrue()?  Because I really just want to allow None, True and
False.  I don't want to allow true-ish and false-ish values.

http://codereview.appspot.com/842043/diff/13001/14002#newcode3695
Python/import.c:3695: if (PyModule_AddObject(m, "magic_tags", magics) < 0) goto
failure;
On 2010/04/07 02:50:04, Benjamin wrote:
> This attribute needs to be documented, too.

Done.

[benjamin_python.org, 2010-04-08 20:54:53]

2010/4/8  <warsaw@gmail.com>:
> http://codereview.appspot.com/842043/diff/13001/14002#newcode3371
> Python/import.c:3371: static char *kwlist[] = {"path", "force__debug__",
> NULL};
> On 2010/04/07 02:50:04, Benjamin wrote:
>>
>> I think the argument should either be called force___debug__ or
>
> something less
>>
>> ugly. force__debug__ is just strangely inconsistent.
>
> How about `debug_override`?

Sounds good.

>
> http://codereview.appspot.com/842043/diff/13001/14002#newcode3383
> Python/import.c:3383: if (force__debug__ != Py_None) {
> On 2010/04/07 02:50:04, Benjamin wrote:
>>
>> Why not just use PyObject_IsTrueBool?
>
> You mean PyObject_IsTrue()?  Because I really just want to allow None,
> True and False.  I don't want to allow true-ish and false-ish values.

It would be more consistent with a python implementation of this
function, though.



-- 
Regards,
Benjamin

[barry, 2010-04-09 15:09:29]

On 2010/04/08 20:54:53, benjamin_python.org wrote:
> 2010/4/8  <warsaw@gmail.com>:
> > http://codereview.appspot.com/842043/diff/13001/14002#newcode3371
> > Python/import.c:3371: static char *kwlist[] = {"path", "force__debug__",
> > NULL};
> > On 2010/04/07 02:50:04, Benjamin wrote:
> >>
> >> I think the argument should either be called force___debug__ or
> >
> > something less
> >>
> >> ugly. force__debug__ is just strangely inconsistent.
> >
> > How about `debug_override`?
> 
> Sounds good.

Cool.

> >
> > http://codereview.appspot.com/842043/diff/13001/14002#newcode3383
> > Python/import.c:3383: if (force__debug__ != Py_None) {
> > On 2010/04/07 02:50:04, Benjamin wrote:
> >>
> >> Why not just use PyObject_IsTrueBool?
> >
> > You mean PyObject_IsTrue()? &nbsp;Because I really just want to allow None,
> > True and False. &nbsp;I don't want to allow true-ish and false-ish values.
> 
> It would be more consistent with a python implementation of this
> function, though.

That's a good point.  Okay, I'll make that change.  I'll be pushing a new
patchset soon which will also include a few fixes for Windows.

[Antoine Pitrou, 2010-04-09 23:39:32]

Some further comments.

http://codereview.appspot.com/842043/diff/32001/33003
File Include/import.h (right):

http://codereview.appspot.com/842043/diff/32001/33003#newcode14
Include/import.h:14: PyAPI_FUNC(PyObject *) PyImport_ExecCodeModuleExEx(
How about `PyImport_ExecCodeModuleWithPathname`? It's longer but a bit less
silly.

http://codereview.appspot.com/842043/diff/32001/33008
File Lib/importlib/test/source/util.py (right):

http://codereview.appspot.com/842043/diff/32001/33008#newcode33
Lib/importlib/test/source/util.py:33: :param bytecode_path: File system path to
PEP 3147 pyc file.
I would really discourage epydoc-style docs, even though it might look like "the
future". It is inconsistent with our current practices.

http://codereview.appspot.com/842043/diff/32001/33028
File Lib/test/support.py (right):

http://codereview.appspot.com/842043/diff/32001/33028#newcode183
Lib/test/support.py:183: # The filename need not exist.
Hmm, why? It might be what your tests want, but not necessarily other people's
:)

http://codereview.appspot.com/842043/diff/32001/33020
File Lib/test/test_pkg.py (right):

http://codereview.appspot.com/842043/diff/32001/33020#newcode199
Lib/test/test_pkg.py:199: ['__cached__', '__doc__', '__file__', '__name__',
Do we want to check the value of the __cached__ attribute (instead of just its
presence)?

http://codereview.appspot.com/842043/diff/32001/33002
File Python/import.c (right):

http://codereview.appspot.com/842043/diff/32001/33002#newcode845
Python/import.c:845: bytes include the slash afer __pycache__, the two extra
dots, the
s/4/5/ !

http://codereview.appspot.com/842043/diff/32001/33002#newcode874
Python/import.c:874: if ((pos = strrchr(buf, '.')) != NULL)
Perhaps you should check that (pos - buf + strlen(pyc_tag) + 5 <= buflen),
otherwise there can be a buffer overflow. For example if pathname ends with ".p"
(can it happen?).

http://codereview.appspot.com/842043/diff/32001/33002#newcode909
Python/import.c:909: if (right-left != 11 || strncmp(left, CACHEDIR, right-left)
!= 0)
Well, perhaps you shouldn't hardcode strlen(CACHEDIR) since the latter form is
used elsewhere.

http://codereview.appspot.com/842043/diff/32001/33002#newcode1320
Python/import.c:1320: PyObject *cached = NULL;
This variable doesn't seem to be used.

http://codereview.appspot.com/842043/diff/32001/33002#newcode3420
Python/import.c:3420: debug = PyObject_IsTrue(debug_override);
You should check for error return.

[Georg, 2010-04-10 19:07:56]

http://codereview.appspot.com/842043/diff/32001/33001
File .bzrignore (right):

http://codereview.appspot.com/842043/diff/32001/33001#newcode35
.bzrignore:35: __pycache__
Should also add that to .hgignore :)

http://codereview.appspot.com/842043/diff/32001/33006
File Doc/library/imp.rst (left):

http://codereview.appspot.com/842043/diff/32001/33006#oldcode206
Doc/library/imp.rst:206: 
These entries need ".. versionadded::" tags (or a separate heading with one such
tag below).

http://codereview.appspot.com/842043/diff/32001/33006
File Doc/library/imp.rst (right):

http://codereview.appspot.com/842043/diff/32001/33006#newcode210
Doc/library/imp.rst:210: .. function:: cache_from_source(path[, debug_override])
Since this function supports keyword args, you should document it in the new 3.x
style:

.. function:: cache_from_source(path, debug_override=None)

http://codereview.appspot.com/842043/diff/32001/33004
File Doc/library/py_compile.rst (right):

http://codereview.appspot.com/842043/diff/32001/33004#newcode37
Doc/library/py_compile.rst:37: returns the path to byte-compilation file, i.e.
whatever *cfile* value was
better "byte-compiled file"?

http://codereview.appspot.com/842043/diff/32001/33005
File Doc/library/unittest.rst (left):

http://codereview.appspot.com/842043/diff/32001/33005#oldcode1132
Doc/library/unittest.rst:1132: 
This change seems to be unrelated.

http://codereview.appspot.com/842043/diff/32001/33008
File Lib/importlib/test/source/util.py (right):

http://codereview.appspot.com/842043/diff/32001/33008#newcode33
Lib/importlib/test/source/util.py:33: :param bytecode_path: File system path to
PEP 3147 pyc file.
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> I would really discourage epydoc-style docs, even though it might look like
"the
> future". It is inconsistent with our current practices.

Do we even have current practices?

http://codereview.appspot.com/842043/diff/32001/33028
File Lib/test/support.py (right):

http://codereview.appspot.com/842043/diff/32001/33028#newcode183
Lib/test/support.py:183: # The filename need not exist.
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> Hmm, why? It might be what your tests want, but not necessarily other people's
> :)

If other tests don't break, I would be in favor of keeping this.  Why should we
be unconditionally ignoring these errors?

http://codereview.appspot.com/842043/diff/32001/33021
File Lib/test/test_imp.py (right):

http://codereview.appspot.com/842043/diff/32001/33021#newcode235
Lib/test/test_imp.py:235: @unittest.skipUnless(os.altsep is not None,
These would read better with "skipIf" IMO.

http://codereview.appspot.com/842043/diff/32001/33026
File Lib/test/test_import.py (right):

http://codereview.appspot.com/842043/diff/32001/33026#newcode504
Lib/test/test_import.py:504: try:
Looks like you could use support.unlink() here?

http://codereview.appspot.com/842043/diff/32001/33023
File Lib/test/test_pydoc.py (right):

http://codereview.appspot.com/842043/diff/32001/33023#newcode187
Lib/test/test_pydoc.py:187: """.strip() # ' <- emacs turd
I'm not sure we should leave those in the source nowadays.

http://codereview.appspot.com/842043/diff/32001/33027
File Lib/test/test_zipfile.py (right):

http://codereview.appspot.com/842043/diff/32001/33027#newcode591
Lib/test/test_zipfile.py:591: if '__pycache__' in fn:
Probably too nitpicky, but this could pick up an unrelated
"Margaret__pycache__i9" along the path.

http://codereview.appspot.com/842043/diff/32001/33002
File Python/import.c (right):

http://codereview.appspot.com/842043/diff/32001/33002#newcode101
Python/import.c:101: tag: cpython-32
there should probably be no tabs here

http://codereview.appspot.com/842043/diff/32001/33002#newcode779
Python/import.c:779: v = Py_None;
Does `__cached__ == None` already have a meaning assigned?

http://codereview.appspot.com/842043/diff/32001/33002#newcode1107
Python/import.c:1107: PySys_WriteStderr(
Can this call fail and set an exception?

http://codereview.appspot.com/842043/diff/32001/33002#newcode1286
Python/import.c:1286: /* Start by trying to turn PEP 3147 path into source path.
 If that
Spaces instead of tabs here.

http://codereview.appspot.com/842043/diff/32001/33002#newcode3740
Python/import.c:3740: Py_XDECREF(magics);
I think this decref is wrong after the PyModule_AddObject() call for
"magic_tags" succeeded, since it already steals the reference.

[barry, 2010-04-12 21:59:05]

Thanks for the review.  Will be pushing a new patch set momentarily that also
includes a Windows fix from Brian Curtin.

http://codereview.appspot.com/842043/diff/32001/33001
File .bzrignore (right):

http://codereview.appspot.com/842043/diff/32001/33001#newcode35
.bzrignore:35: __pycache__
On 2010/04/10 19:07:56, Georg wrote:
> Should also add that to .hgignore :)

Done.

http://codereview.appspot.com/842043/diff/32001/33006
File Doc/library/imp.rst (left):

http://codereview.appspot.com/842043/diff/32001/33006#oldcode206
Doc/library/imp.rst:206: 
On 2010/04/10 19:07:56, Georg wrote:
> These entries need ".. versionadded::" tags (or a separate heading with one
such
> tag below).

Done.

I added a versionadded after the sentence that introduces these three new
functions.  I'm not sure this looks right, but I think if I add a section for
these, I'll have to add sections for other parts of this document.  Let me know
what you think.

http://codereview.appspot.com/842043/diff/32001/33006
File Doc/library/imp.rst (right):

http://codereview.appspot.com/842043/diff/32001/33006#newcode210
Doc/library/imp.rst:210: .. function:: cache_from_source(path[, debug_override])
On 2010/04/10 19:07:56, Georg wrote:
> Since this function supports keyword args, you should document it in the new
3.x
> style:
> 
> .. function:: cache_from_source(path, debug_override=None)

Done.

http://codereview.appspot.com/842043/diff/32001/33004
File Doc/library/py_compile.rst (right):

http://codereview.appspot.com/842043/diff/32001/33004#newcode37
Doc/library/py_compile.rst:37: returns the path to byte-compilation file, i.e.
whatever *cfile* value was
On 2010/04/10 19:07:56, Georg wrote:
> better "byte-compiled file"?

Done.

http://codereview.appspot.com/842043/diff/32001/33005
File Doc/library/unittest.rst (left):

http://codereview.appspot.com/842043/diff/32001/33005#oldcode1132
Doc/library/unittest.rst:1132: 
On 2010/04/10 19:07:56, Georg wrote:
> This change seems to be unrelated.

Yep, just a drive-by fix that I'll land separately in the relevant branches.

http://codereview.appspot.com/842043/diff/32001/33003
File Include/import.h (right):

http://codereview.appspot.com/842043/diff/32001/33003#newcode14
Include/import.h:14: PyAPI_FUNC(PyObject *) PyImport_ExecCodeModuleExEx(
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> How about `PyImport_ExecCodeModuleWithPathname`? It's longer but a bit less
> silly.
> 

Great minds think alike I guess!  But I added an 's' on the end.

http://codereview.appspot.com/842043/diff/32001/33008
File Lib/importlib/test/source/util.py (right):

http://codereview.appspot.com/842043/diff/32001/33008#newcode33
Lib/importlib/test/source/util.py:33: :param bytecode_path: File system path to
PEP 3147 pyc file.
On 2010/04/10 19:07:56, Georg wrote:
> On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> > I would really discourage epydoc-style docs, even though it might look like
> "the
> > future". It is inconsistent with our current practices.
> 
> Do we even have current practices?
> 

My question exactly.  Probably a good discussion for the doc-sig.  I'll start
things going there.

http://codereview.appspot.com/842043/diff/32001/33028
File Lib/test/support.py (right):

http://codereview.appspot.com/842043/diff/32001/33028#newcode183
Lib/test/support.py:183: # The filename need not exist.
On 2010/04/10 19:07:56, Georg wrote:
> On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> > Hmm, why? It might be what your tests want, but not necessarily other
people's
> > :)
> 
> If other tests don't break, I would be in favor of keeping this.  Why should
we
> be unconditionally ignoring these errors? 

Nothing else breaks.  I think it was just laziness in the original code.  I
can't understand why you'd want to ignore all OSErrors in an unlink().

http://codereview.appspot.com/842043/diff/32001/33021
File Lib/test/test_imp.py (right):

http://codereview.appspot.com/842043/diff/32001/33021#newcode235
Lib/test/test_imp.py:235: @unittest.skipUnless(os.altsep is not None,
On 2010/04/10 19:07:56, Georg wrote:
> These would read better with "skipIf" IMO.

Done.

http://codereview.appspot.com/842043/diff/32001/33026
File Lib/test/test_import.py (right):

http://codereview.appspot.com/842043/diff/32001/33026#newcode504
Lib/test/test_import.py:504: try:
On 2010/04/10 19:07:56, Georg wrote:
> Looks like you could use support.unlink() here?

Done.

http://codereview.appspot.com/842043/diff/32001/33020
File Lib/test/test_pkg.py (right):

http://codereview.appspot.com/842043/diff/32001/33020#newcode199
Lib/test/test_pkg.py:199: ['__cached__', '__doc__', '__file__', '__name__',
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> Do we want to check the value of the __cached__ attribute (instead of just its
> presence)?
>

Here?  No I don't think we need to.  test_import.py does that.  This just fixes
an existing test to handle the added attribute.

http://codereview.appspot.com/842043/diff/32001/33023
File Lib/test/test_pydoc.py (right):

http://codereview.appspot.com/842043/diff/32001/33023#newcode187
Lib/test/test_pydoc.py:187: """.strip() # ' <- emacs turd
On 2010/04/10 19:07:56, Georg wrote:
> I'm not sure we should leave those in the source nowadays.

Removed, but jeebus are these horrible tests! ;)

http://codereview.appspot.com/842043/diff/32001/33027
File Lib/test/test_zipfile.py (right):

http://codereview.appspot.com/842043/diff/32001/33027#newcode591
Lib/test/test_zipfile.py:591: if '__pycache__' in fn:
On 2010/04/10 19:07:56, Georg wrote:
> Probably too nitpicky, but this could pick up an unrelated
> "Margaret__pycache__i9" along the path.

True.  Probably doesn't matter for the tests.  I did however split the pathname
along os.sep and checked for __pycache__ in there.  It's a bit nasty because you
also have to include os.altsep if it's not None.  Not sure I like the new code
any better, but see what you think.

http://codereview.appspot.com/842043/diff/32001/33002
File Python/import.c (right):

http://codereview.appspot.com/842043/diff/32001/33002#newcode101
Python/import.c:101: tag: cpython-32
On 2010/04/10 19:07:56, Georg wrote:
> there should probably be no tabs here

How horribly inconsistent!  OTOH, if you look up to other indented text in that
big comment, there are leading tabs, so I think I'll keep this one.

http://codereview.appspot.com/842043/diff/32001/33002#newcode779
Python/import.c:779: v = Py_None;
On 2010/04/10 19:07:56, Georg wrote:
> Does `__cached__ == None` already have a meaning assigned?

Yes and no.  __cached__ is actually implementation specific, so we're free to do
what we want.  Would you suggest raising the exception here instead?  Seems like
that's outside the intent of this function, but other than that I don't know
what else you'd do.

http://codereview.appspot.com/842043/diff/32001/33002#newcode845
Python/import.c:845: bytes include the slash afer __pycache__, the two extra
dots, the
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> s/4/5/ !
> 

Done.

http://codereview.appspot.com/842043/diff/32001/33002#newcode874
Python/import.c:874: if ((pos = strrchr(buf, '.')) != NULL)
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> Perhaps you should check that (pos - buf + strlen(pyc_tag) + 5 <= buflen),
> otherwise there can be a buffer overflow. For example if pathname ends with
".p"
> (can it happen?).
> 
> 

I've thought about this fairly extensively and done lots of little graphs, as
well as hard code some limits down to some more easily debugged sizes (e.g.
buflen=32 instead of MAXPATHLEN).  I think I now understand how it's possible to
overflow the buffer with the old code, though it's not *++pos='\0' that can
cause it to happen.

The problem is that the first test assumes we're only adding one character for
the extension, but if for example, there is no extension, then we're really
adding 4 (plus the nul).  Since we already account for the null and the
separating dot, it's really 3 characters when we only figured in 1.  However, in
a no-extension input string like 'xxxxx' we don't write the first dot, we only
overflow by one character, which of course is still bad. ;)

I believe it's only possible for the writing of the extension to overflow the
buffer.  Everything else has been accounted for.  So I've added another
conditional to just before the writing of the extension.  I've also added an
assert statement and an *extensive* comment.  Once I upload the new patchset,
I'd love to know whether you agree with my analysis or not!

http://codereview.appspot.com/842043/diff/32001/33002#newcode909
Python/import.c:909: if (right-left != 11 || strncmp(left, CACHEDIR, right-left)
!= 0)
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> Well, perhaps you shouldn't hardcode strlen(CACHEDIR) since the latter form is
> used elsewhere.
> 

Done.

http://codereview.appspot.com/842043/diff/32001/33002#newcode1107
Python/import.c:1107: PySys_WriteStderr(
On 2010/04/10 19:07:56, Georg wrote:
> Can this call fail and set an exception?

What, the PySys_WriteStderr()?  No, I don't believe so.

http://codereview.appspot.com/842043/diff/32001/33002#newcode1286
Python/import.c:1286: /* Start by trying to turn PEP 3147 path into source path.
 If that
On 2010/04/10 19:07:56, Georg wrote:
> Spaces instead of tabs here.

Done.

http://codereview.appspot.com/842043/diff/32001/33002#newcode1320
Python/import.c:1320: PyObject *cached = NULL;
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> This variable doesn't seem to be used.

Done.

http://codereview.appspot.com/842043/diff/32001/33002#newcode3420
Python/import.c:3420: debug = PyObject_IsTrue(debug_override);
On 2010/04/09 23:39:32, Antoine Pitrou wrote:
> You should check for error return.

Done, but note that (at least in this file) it seems to be the exception <wink>.

http://codereview.appspot.com/842043/diff/32001/33002#newcode3740
Python/import.c:3740: Py_XDECREF(magics);
On 2010/04/10 19:07:56, Georg wrote:
> I think this decref is wrong after the PyModule_AddObject() call for
> "magic_tags" succeeded, since it already steals the reference.

Actually, this code is gone now.

[pjenvey, 2010-04-13 22:47:41]

Barry, just one small thing:

__pycached__ is probably just like __file__ in that it could end up being a
relative path before site has been fully imported. So I'm guessing site needs to
abspath() the initial set of sys.modules' __pycached__ values like it already
does for their __file__s

You should be able to see a case where they would be relative with an svn
checkout, via PYTHONPATH=Lib/ ./python

[barry, 2010-04-14 23:56:57]

On 2010/04/13 22:47:41, pjenvey wrote:
> Barry, just one small thing:
> 
> __pycached__ is probably just like __file__ in that it could end up being a
> relative path before site has been fully imported. So I'm guessing site needs
to
> abspath() the initial set of sys.modules' __pycached__ values like it already
> does for their __file__s
> 
> You should be able to see a case where they would be relative with an svn
> checkout, via PYTHONPATH=Lib/ ./python

Note that test_abs__file__() in test_site.py is pretty crappy, and broken.  I
have a much better test for this, and added a test to ensure abspath'd
__cached__.

Thanks for pointing this one out.