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

Support resources in namespace packages #86295

Closed
jaraco opened this issue Oct 23, 2020 · 12 comments
Closed

Support resources in namespace packages #86295

jaraco opened this issue Oct 23, 2020 · 12 comments
Assignees
Labels
3.10 only security fixes

Comments

@jaraco
Copy link
Member

jaraco commented Oct 23, 2020

BPO 42129
Nosy @warsaw, @brettcannon, @jaraco, @vstinner, @FFY00
PRs
  • bpo-42129: Add support for resources in namespaces #24670
  • 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/jaraco'
    closed_at = <Date 2021-03-22.09:01:32.527>
    created_at = <Date 2020-10-23.16:03:26.864>
    labels = ['3.10']
    title = 'Support resources in namespace packages'
    updated_at = <Date 2021-03-22.09:01:32.527>
    user = 'https://github.com/jaraco'

    bugs.python.org fields:

    activity = <Date 2021-03-22.09:01:32.527>
    actor = 'vstinner'
    assignee = 'jaraco'
    closed = True
    closed_date = <Date 2021-03-22.09:01:32.527>
    closer = 'vstinner'
    components = []
    creation = <Date 2020-10-23.16:03:26.864>
    creator = 'jaraco'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42129
    keywords = ['patch']
    message_count = 12.0
    messages = ['379451', '384770', '384776', '384781', '386958', '387197', '387805', '387808', '388121', '389163', '389164', '389288']
    nosy_count = 5.0
    nosy_names = ['barry', 'brett.cannon', 'jaraco', 'vstinner', 'FFY00']
    pr_nums = ['24670']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42129'
    versions = ['Python 3.10']

    @jaraco
    Copy link
    Member Author

    jaraco commented Oct 23, 2020

    In importlib_resources#68, the project identified a deficiency with respect to pkg_resources for resources in namespace packages.

    The project has since merged support for these packages, slated to go out with the importlib_resources 3.2 release.

    This issue is to track the incorporation of those changes into importlib.resources.

    @jaraco jaraco added the 3.10 only security fixes label Oct 23, 2020
    @jaraco jaraco self-assigned this Oct 23, 2020
    @jaraco
    Copy link
    Member Author

    jaraco commented Jan 10, 2021

    In this commit, I've reconciled and merged the changes from importlib_resources 3.2-5.0, mainly the namespace package support (https://importlib-resources.readthedocs.io/en/latest/history.html#v5-0-0).

    Applying these changes to cpython and running the tests, there appear to be two emergent failure modes:

    cpython master $ http --follow https://github.com/python/importlib_resources/archive/cpython.zip | bsdtar --strip-components 1 -x
    $ ./python.exe Tools/scripts/run_tests.py test.test_importlib 2>&1 | gist -Pc
    https://gist.github.com/dde7d5a951d92726380dced21503f843
    

    In my estimation, the first two failures are due to the change in the abc.ResourceReader for contents to raise FileNotFoundError.

    And the latter failures are seemingly due to the built-in NamespaceLoader not having a resource reader. I suspect the fix here is to add a .get_resource_reader() method to _NamespaceLoader to return a NamespaceReader. (hint: when updating _bootstrap_external.py, you'll need to run make regen-importlib or make regen-all and recompile).

    Filipe, would you be interested in exploring these issues and propose some fixes and prepare the PR that applies the changes to CPython?

    @FFY00
    Copy link
    Member

    FFY00 commented Jan 10, 2021

    Yes, I will look into it. Do we have a time schedule for when this should be ready, so that I can organize myself?

    @jaraco
    Copy link
    Member Author

    jaraco commented Jan 10, 2021

    Thanks!

    No rush, but ideally soon enough to be merged before the beta release of Python 3.10 (2021-05-03).

    @jaraco
    Copy link
    Member Author

    jaraco commented Feb 14, 2021

    For the first two errors, the issue seems to be that CPython includes tests for the ResourceReader ABC and asserts that .contents() returns an empty list as the default degenerate behavior (

    def test_contents(self):
    self.assertEqual([], list(self.ins.contents()))
    ). It appears that the ABC in importlib_resources has for a very long time raised FileNotFound.

    Oh, this is interesting - these ABCs have diverged since their inception; here's where both the empty list behavior and test were introduced in cpython and later amended to return an iterable instead of iterator.

    Brett, is this a divergence worth maintaining? Do you recall if there was a reason for changing the ABC during the port to CPython? There's no mention of 'contents' in bpo-32248.

    My temptation is to go with the implementation as found in importlib_resources as it presents a more consistent interface for ResourceReaders (and explicitly defaults to raising FileNotFoundError when no implementation for contents is present) and thus to change the test to:

    cpython master $ git diff Lib/test/test_importlib/test_abc.py
    diff --git a/Lib/test/test_importlib/test_abc.py b/Lib/test/test_importlib/test_abc.py
    index d8b9fc89f2..d1c89c183b 100644
    --- a/Lib/test/test_importlib/test_abc.py
    +++ b/Lib/test/test_importlib/test_abc.py
    @@ -338,7 +338,9 @@ def test_is_resource(self):
                 self.ins.is_resource('dummy_file')
     
         def test_contents(self):
    -        self.assertEqual([], list(self.ins.contents()))
    +        with self.assertRaises(FileNotFoundError):
    +            self.ins.contents()
    +
     
     (Frozen_RRDefaultTests,
      Source_RRDefaultsTests
    

    Brett, let me know if you have concern about harmonizing the two implementations around this behavior and if you have a strong preference for harmonizing toward the CPython implementation instead. Thanks.

    @brettcannon
    Copy link
    Member

    I would harmonize towards what the concrete implementations are doing since people who don't implement the defaults will get the appropriate results regardless. An entry in What's New will cover any potential notification of the change since you can't exactly deprecate this sort of thing.

    @jaraco
    Copy link
    Member Author

    jaraco commented Feb 28, 2021

    I would harmonize towards what the concrete implementations...

    For FileReader and ZipReader, both rely on TraversableResources to implement contents, which rely on files().iterdir(), which could raise FileNotFoundError and definitely don't return lists. Most importantly, this method is an abstract method, so subclasses can't rely on it (must override it). Searching Github, I couldn't find any other classes subclassing this ABC.

    @jaraco
    Copy link
    Member Author

    jaraco commented Feb 28, 2021

    I've pushed this branch, which includes fixes for the above two identified issues. Still one issue remains:

    ERROR: test_package_has_no_reader_fallback (test.test_importlib.test_resource.ResourceCornerCaseTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/Users/jaraco/code/public/cpython/Lib/test/test_importlib/test_resource.py", line 98, in test_package_has_no_reader_fallback
        self.assertFalse(resources.is_resource(module, 'A'))
      File "/Users/jaraco/code/public/cpython/Lib/importlib/resources.py", line 157, in is_resource
        package_contents = set(contents(package))
      File "/Users/jaraco/code/public/cpython/Lib/importlib/resources.py", line 174, in contents
        transversable = _common.from_package(package)
      File "/Users/jaraco/code/public/cpython/Lib/importlib/_common.py", line 75, in from_package
        reader = spec.loader.get_resource_reader(spec.name)
    AttributeError: 'object' object has no attribute 'get_resource_reader'

    This same test passes on importlib_resources, and the difference seems to be rooted in how from_package resolves the package spec using a compatibility wrapper. On the backport, this causes the package without a resource reader to have a resource reader and return a degenerate value:

    > /Users/jaraco/code/public/importlib_resources/importlib_resources/_py3.py(139)is_resource()
    -> package_contents = set(contents(package))
    (Pdb) _common.from_package(package)
    PosixPath('/path/which/shall/not/be')
    (Pdb) from . import _compat
    (Pdb) _compat.package_spec(package).loader.get_resource_reader('any').files().is_dir()
    False
    

    This means that the compatibility shim in from_package is masking test failure in the backport, and probably the best course of action will be to unmask that failure in the backport and figure out the best behavior there.

    @jaraco
    Copy link
    Member Author

    jaraco commented Mar 4, 2021

    New changeset 6714825 by Jason R. Coombs in branch 'master':
    bpo-42129: Add support for resources in namespaces (GH-24670)
    6714825

    @jaraco jaraco closed this as completed Mar 4, 2021
    @vstinner
    Copy link
    Member

    Please see bpo-43569: "test_importlib failed on installed Python" regression introduced by commit 6714825.

    @vstinner
    Copy link
    Member

    I reopen the issue.

    @vstinner vstinner reopened this Mar 20, 2021
    @vstinner
    Copy link
    Member

    Please see bpo-43569: "test_importlib failed on installed Python" regression introduced by commit 6714825.

    It's now fixed, I close again this issue.

    @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
    3.10 only security fixes
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants