classification
Title: zipimport's handling of namespace packages is incorrect
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Arfrever, brett.cannon, eric.smith, eric.snow, gregory.p.smith, isoschiz, ncoghlan, pconnell, python-dev, romberg, superluser, theller
Priority: normal Keywords: needs review, patch

Created on 2013-04-04 10:10 by pconnell, last changed 2016-01-15 19:40 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue17633.diff pconnell, 2013-04-18 20:00 review
issue17633-2.diff pconnell, 2013-04-18 21:12 review
issue17633-3.diff romberg, 2016-01-04 18:01 zipimport namespace pkg fix + expanded test cases
issue17633-hg.diff romberg, 2016-01-05 00:04 fix for issue17633 as mercurial patch off of repository review
issue17633-hg-2.patch romberg, 2016-01-09 20:06 patch for 17633 (with review suggested changes) review
Messages (13)
msg186025 - (view) Author: Phil Connell (pconnell) * Date: 2013-04-04 10:10
Only one level of namespace package nesting is handled correctly:

$ unzip -l foo.zip
Archive:  foo.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  2013-04-03 17:28   a/b/c/foo.py
        0  2013-04-03 17:34   a/
        0  2013-04-03 17:34   a/b/
        0  2013-04-03 17:34   a/b/c/
---------                     -------
        0                     4 files
$ ls
foo.zip
$ PYTHONPATH=foo.zip ~/dev/cpython/python
Python 3.4.0a0 (default:3b1dbe7a2aa0+, Apr  3 2013, 17:31:54) 
[GCC 4.8.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import a
>>> import a.b
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named 'a.b'
>>>


The problem appears to be that check_is_directory constructs the wrong directory path (it should be 'a/b'):

check_is_directory (self=0x7ffff6d3dc88, prefix='a/', path='a.b')
    at ./Modules/zipimport.c:280
280     dirpath = PyUnicode_FromFormat("%U%U%c", prefix, path, SEP);
(gdb) n
281     if (dirpath == NULL)
(gdb) p dirpath
$11 = 'a/a.b/'


I've attached a tentative initial patch that appears to fix the issue, although it probably needs some more thought (and definitely some more testing - the existing tests still pass though).
msg186030 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2013-04-04 13:19
Could you construct a test (that would fail without your patch)?

Thanks.
msg186164 - (view) Author: Phil Connell (pconnell) * Date: 2013-04-06 20:40
Here's a test that fails without the patch and succeeds with the patch.
msg187286 - (view) Author: Phil Connell (pconnell) * Date: 2013-04-18 20:00
The attached patch is ready for review.
msg187295 - (view) Author: Phil Connell (pconnell) * Date: 2013-04-18 21:12
Updated patch with markups suggested by Serhiy.
msg257476 - (view) Author: Mike Romberg (romberg) * Date: 2016-01-04 18:01
I have expanded on the issue17633-2 patch to fix an issue with the enumerated value returned by find_loader().  The patch (issue17633-3.diff) is against 3.5.1.

  I have also added more test cases that cover spreading a namespace package across two zip files and between a zip file and a filesystem.  The expanded tests cover the iterable __path__ member of a namespace package.

  No new failures in 'make test'  (gdb and ssl were broken with in 3.5.1 before this patch).

  The expanded test cases should continue to be relevant even if zipimport is rewritten.
msg257502 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-01-04 22:58
Any chance you can do the diff against an hg checkout, Mike? That way we can use our review tool to do the code review, else we have to work only from the raw diff file which isn't as nice.
msg257504 - (view) Author: Mike Romberg (romberg) * Date: 2016-01-04 23:15
Yes.  I can do this.  I've not used hg before.  But I bet I can figure it out.  I'm assuming hg has a diff/pach genterator of some kind ('hg diff' perhaps).

  Lemme try and find the hg repository and check out a copy...
msg257506 - (view) Author: Mike Romberg (romberg) * Date: 2016-01-05 00:04
Try issue17633-hg.diff.   Caution it was made after literally minutes of experience with hg.  :)

  I checked out the source applied the changes, compiled and ran 'make test' (gdb still fails), and did an hg commit.  The diff was made following the instructions here:

https://docs.python.org/devguide/gitdevs.html

  It seems to have worked.  But again, I have literally minutes of hg experience.  If this needs to be redone let me know.
msg257507 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-01-05 00:46
Nope, looks like it worked! I will try to have a look at the patch some time this week.

And for future reference, you can also use https://docs.python.org/devguide/patch.html as a guide.
msg257856 - (view) Author: Mike Romberg (romberg) * Date: 2016-01-09 20:06
This patch modifies issue17633-hg.diff by adding changes suggested by the reviewers.

  Note.  I did cleanup the use of __import__ outside of the area involved with issue17633 as it seemed low risk.  The tests for issue17633 (and the refactored doTest/makeZip now use addCleanup().  However there are still tests in the module that use the old try/finally way of cleanup.  I did not modify these in order to keep this from being a rewrite of test_zipimport.

  But if more changes are desired, I'll add them.
msg258321 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-01-15 19:26
The fix for 3.5 is in https://hg.python.org/cpython/rev/07a615a8f9ad and 3.6 in https://hg.python.org/cpython/rev/87f87673af7b.

Thanks to Phil and Mike for tackling this problem!
msg258323 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-15 19:40
New changeset 07a615a8f9ad by Brett Cannon in branch '3.5':
Issue #17633: Improve support for namespace packages with zipimport.
https://hg.python.org/cpython/rev/07a615a8f9ad

New changeset 87f87673af7b by Brett Cannon in branch 'default':
Merge for issue #17633
https://hg.python.org/cpython/rev/87f87673af7b
History
Date User Action Args
2016-01-15 19:40:19python-devsetnosy: + python-dev
messages: + msg258323
2016-01-15 19:26:30brett.cannonsetstatus: open -> closed
versions: + Python 3.5
messages: + msg258321

resolution: fixed
stage: patch review -> resolved
2016-01-09 20:06:27rombergsetfiles: + issue17633-hg-2.patch

messages: + msg257856
2016-01-05 00:46:34brett.cannonsetassignee: brett.cannon
messages: + msg257507
2016-01-05 00:04:57rombergsetfiles: + issue17633-hg.diff

messages: + msg257506
2016-01-04 23:15:23rombergsetmessages: + msg257504
2016-01-04 22:58:20brett.cannonsetmessages: + msg257502
2016-01-04 18:01:17rombergsetfiles: + issue17633-3.diff
nosy: + romberg
messages: + msg257476

2015-08-05 16:07:07eric.snowsettype: behavior
stage: patch review
2015-08-05 16:06:49eric.snowsetnosy: + gregory.p.smith, superluser

versions: + Python 3.6, - Python 3.5
2014-01-31 20:48:04thellersetnosy: + theller
2014-01-31 20:26:22eric.smithsetkeywords: + needs review
versions: + Python 3.5, - Python 3.4
2013-04-18 21:12:27pconnellsetfiles: + issue17633-2.diff

messages: + msg187295
2013-04-18 20:00:50pconnellsetfiles: + issue17633.diff

messages: + msg187286
2013-04-18 19:58:51pconnellsetfiles: - test.diff
2013-04-18 19:58:47pconnellsetfiles: - zipimport_ns.diff
2013-04-06 20:40:40pconnellsetfiles: + test.diff

messages: + msg186164
2013-04-06 20:39:03pconnellsetfiles: + zipimport_ns.diff
2013-04-06 20:37:56pconnellsetfiles: - zipimport_ns.diff
2013-04-04 13:19:58eric.smithsetmessages: + msg186030
2013-04-04 13:18:01eric.smithsetnosy: + eric.smith
2013-04-04 12:39:27Arfreversetnosy: + Arfrever
2013-04-04 11:49:38pconnellsetnosy: + isoschiz
2013-04-04 10:10:23pconnellcreate