Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zipimport's handling of namespace packages is incorrect #61833

Closed
phmc mannequin opened this issue Apr 4, 2013 · 13 comments
Closed

zipimport's handling of namespace packages is incorrect #61833

phmc mannequin opened this issue Apr 4, 2013 · 13 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@phmc
Copy link
Mannequin

phmc mannequin commented Apr 4, 2013

BPO 17633
Nosy @brettcannon, @theller, @gpshead, @ncoghlan, @ericvsmith, @ericsnowcurrently, @phmc
Files
  • issue17633.diff
  • issue17633-2.diff
  • issue17633-3.diff: zipimport namespace pkg fix + expanded test cases
  • issue17633-hg.diff: fix for issue17633 as mercurial patch off of repository
  • issue17633-hg-2.patch: patch for 17633 (with review suggested changes)
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/brettcannon'
    closed_at = <Date 2016-01-15.19:26:30.397>
    created_at = <Date 2013-04-04.10:10:23.770>
    labels = ['extension-modules', 'type-bug']
    title = "zipimport's handling of namespace packages is incorrect"
    updated_at = <Date 2016-01-15.19:40:19.018>
    user = 'https://github.com/phmc'

    bugs.python.org fields:

    activity = <Date 2016-01-15.19:40:19.018>
    actor = 'python-dev'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2016-01-15.19:26:30.397>
    closer = 'brett.cannon'
    components = ['Extension Modules']
    creation = <Date 2013-04-04.10:10:23.770>
    creator = 'pconnell'
    dependencies = []
    files = ['29925', '29927', '41498', '41501', '41557']
    hgrepos = []
    issue_num = 17633
    keywords = ['patch', 'needs review']
    message_count = 13.0
    messages = ['186025', '186030', '186164', '187286', '187295', '257476', '257502', '257504', '257506', '257507', '257856', '258321', '258323']
    nosy_count = 12.0
    nosy_names = ['brett.cannon', 'romberg', 'theller', 'gregory.p.smith', 'ncoghlan', 'eric.smith', 'Arfrever', 'python-dev', 'eric.snow', 'pconnell', 'isoschiz', 'superluser']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue17633'
    versions = ['Python 3.5', 'Python 3.6']

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Apr 4, 2013

    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).

    @phmc phmc mannequin added the extension-modules C modules in the Modules dir label Apr 4, 2013
    @ericvsmith
    Copy link
    Member

    Could you construct a test (that would fail without your patch)?

    Thanks.

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Apr 6, 2013

    Here's a test that fails without the patch and succeeds with the patch.

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Apr 18, 2013

    The attached patch is ready for review.

    @phmc
    Copy link
    Mannequin Author

    phmc mannequin commented Apr 18, 2013

    Updated patch with markups suggested by Serhiy.

    @ericsnowcurrently ericsnowcurrently added the type-bug An unexpected behavior, bug, or error label Aug 5, 2015
    @romberg
    Copy link
    Mannequin

    romberg mannequin commented Jan 4, 2016

    I have expanded on the bpo-17633-2 patch to fix an issue with the enumerated value returned by find_loader(). The patch (bpo-17633-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.

    @brettcannon
    Copy link
    Member

    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.

    @romberg
    Copy link
    Mannequin

    romberg mannequin commented Jan 4, 2016

    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...

    @romberg
    Copy link
    Mannequin

    romberg mannequin commented Jan 5, 2016

    Try bpo-17633-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.

    @brettcannon
    Copy link
    Member

    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.

    @brettcannon brettcannon self-assigned this Jan 5, 2016
    @romberg
    Copy link
    Mannequin

    romberg mannequin commented Jan 9, 2016

    This patch modifies bpo-17633-hg.diff by adding changes suggested by the reviewers.

    Note. I did cleanup the use of __import__ outside of the area involved with bpo-17633 as it seemed low risk. The tests for bpo-17633 (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.

    @brettcannon
    Copy link
    Member

    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!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 15, 2016

    New changeset 07a615a8f9ad by Brett Cannon in branch '3.5':
    Issue bpo-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 bpo-17633
    https://hg.python.org/cpython/rev/87f87673af7b

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants