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

Deprecation warning in zoneinfo module #90282

Closed
tirkarthi opened this issue Dec 18, 2021 · 10 comments
Closed

Deprecation warning in zoneinfo module #90282

tirkarthi opened this issue Dec 18, 2021 · 10 comments
Assignees
Labels
3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@tirkarthi
Copy link
Member

BPO 46124
Nosy @jaraco, @pganssle, @miss-islington, @tirkarthi
PRs
  • bpo-46124: Update zoneinfo to rely on importlib.resources traversable API. #30190
  • 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/pganssle'
    closed_at = <Date 2022-01-21.21:59:16.268>
    created_at = <Date 2021-12-18.14:04:21.971>
    labels = ['type-bug', 'library', '3.11']
    title = 'Deprecation warning in zoneinfo module'
    updated_at = <Date 2022-01-21.21:59:16.267>
    user = 'https://github.com/tirkarthi'

    bugs.python.org fields:

    activity = <Date 2022-01-21.21:59:16.267>
    actor = 'jaraco'
    assignee = 'p-ganssle'
    closed = True
    closed_date = <Date 2022-01-21.21:59:16.268>
    closer = 'jaraco'
    components = ['Library (Lib)']
    creation = <Date 2021-12-18.14:04:21.971>
    creator = 'xtreak'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46124
    keywords = ['patch']
    message_count = 10.0
    messages = ['408851', '408870', '408871', '408873', '409259', '409260', '409272', '409273', '411185', '411194']
    nosy_count = 4.0
    nosy_names = ['jaraco', 'p-ganssle', 'miss-islington', 'xtreak']
    pr_nums = ['30190']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue46124'
    versions = ['Python 3.11']

    @tirkarthi
    Copy link
    Member Author

    zoneinfo module currently emits deprecation warnings due to API changes in importlib.resources

    ./python -Wonce -m test test_zoneinfo
    0:00:00 load avg: 0.73 Run tests sequentially
    0:00:00 load avg: 0.73 [1/1] test_zoneinfo
    /home/karthikeyan/stuff/python/cpython/Lib/zoneinfo/_tzpath.py:121: DeprecationWarning: open_text is deprecated. Use files() instead. Refer to https://importlib-resources.readthedocs.io/en/latest/using.html#migrating-from-legacy for migration advice.
    with resources.open_text("tzdata", "zones") as f:
    /home/karthikeyan/stuff/python/cpython/Lib/zoneinfo/_common.py:12: DeprecationWarning: open_binary is deprecated. Use files() instead. Refer to https://importlib-resources.readthedocs.io/en/latest/using.html#migrating-from-legacy for migration advice.
    return importlib.resources.open_binary(package_name, resource_name)
    /home/karthikeyan/stuff/python/cpython/Lib/zoneinfo/_tzpath.py:121: DeprecationWarning: open_text is deprecated. Use files() instead. Refer to https://importlib-resources.readthedocs.io/en/latest/using.html#migrating-from-legacy for migration advice.
    with resources.open_text("tzdata", "zones") as f:
    /home/karthikeyan/stuff/python/cpython/Lib/zoneinfo/_common.py:12: DeprecationWarning: open_binary is deprecated. Use files() instead. Refer to https://importlib-resources.readthedocs.io/en/latest/using.html#migrating-from-legacy for migration advice.
    return importlib.resources.open_binary(package_name, resource_name)

    == Tests result: SUCCESS ==

    1 test OK.

    Total duration: 376 ms
    Tests result: SUCCESS

    A fix would be to adapt the _legacy module changes inline like below patch though normalize_path is not public API and need to be inlined too.

    diff --git a/Lib/zoneinfo/_common.py b/Lib/zoneinfo/_common.py
    index 4c24f01bd7..bfe3fe4c3c 100644
    --- a/Lib/zoneinfo/_common.py
    +++ b/Lib/zoneinfo/_common.py
    @@ -2,14 +2,15 @@
     
     
     def load_tzdata(key):
    -    import importlib.resources
    +    from importlib.resources import files
    +    from importlib._common import normalize_path
     
         components = key.split("/")
         package_name = ".".join(["tzdata.zoneinfo"] + components[:-1])
         resource_name = components[-1]
     
         try:
    -        return importlib.resources.open_binary(package_name, resource_name)
    +        return (files(package_name) / normalize_path(resource_name)).open('rb')
         except (ImportError, FileNotFoundError, UnicodeEncodeError):
             # There are three types of exception that can be raised that all amount
             # to "we cannot find this key":
    diff --git a/Lib/zoneinfo/_tzpath.py b/Lib/zoneinfo/_tzpath.py
    index 672560b951..b1efe5d99e 100644
    --- a/Lib/zoneinfo/_tzpath.py
    +++ b/Lib/zoneinfo/_tzpath.py
    @@ -111,14 +111,15 @@ def available_timezones():
             determine if a given file on the time zone search path is to open it
             and check for the "magic string" at the beginning.
         """
    -    from importlib import resources
    +    from importlib.resources import files
    +    from importlib._common import normalize_path
     
         valid_zones = set()
     
         # Start with loading from the tzdata package if it exists: this has a
         # pre-assembled list of zones that only requires opening one file.
         try:
    -        with resources.open_text("tzdata", "zones") as f:
    +        with (files("tzdata") / normalize_path("zones")).open('r') as f:
                 for zone in f:
                     zone = zone.strip()
                     if zone:

    @tirkarthi tirkarthi added 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 18, 2021
    @jaraco
    Copy link
    Member

    jaraco commented Dec 18, 2021

    I would hope that normalize_path would not be needed. It might be for drop-in compatibility. If it's needed for zoneinfo, I'd like to consider why and what implications that has for the deprecation of the legacy API.

    @jaraco jaraco self-assigned this Dec 18, 2021
    @jaraco
    Copy link
    Member

    jaraco commented Dec 18, 2021

    I just confirmed, and normalize_path has been moved to the _legacy module for importlib_resources, so I'd expect that change to land in CPython soon too.

    @jaraco
    Copy link
    Member

    jaraco commented Dec 18, 2021

    I filed bpo-46125 to track that issue separately.

    @jaraco jaraco assigned pganssle and unassigned jaraco Dec 19, 2021
    @pganssle
    Copy link
    Member

    Jason's patch looks good to me, but I don't understand why Karthikeyan originally suggested using normalize_path. Trying to dig through exactly how files().joinpath().open is implemented has so many layers of indirection and abstract classes that I can't quite figure out if the two things are equivalent or not. Seems like joinpath() is the right thing to do, but does it have less validation than the normalize_path method?

    @tirkarthi
    Copy link
    Member Author

    I just copied the implementation and normalize_path function was part of it. Looking into the implementation of normalize_path it validates the given argument to be a filename without any separator. I will leave it to Jason for a better understanding of the API and compatibility here.

    @jaraco
    Copy link
    Member

    jaraco commented Dec 28, 2021

    Does joinpath have less validation?

    Yes. Previously, resources.* would perform some validation on the path to ensure that it didn't contain path separators (to avoid users attempting to get resources in subdirectories or perhaps manipulating the path in other ways).

    So no, they're not equivalent. If resource_name or "zones" ever contained path separators, the former implementation would raise an error whereas this implementation would attempt to join those characters to the path. Since "zones" is a static string, it's clearly not affected. And resource_name can't have posixpath.sep. If key had an ntpath.sep, that might behave differently, but that seemed like an edge case not worth exploring.

    If it is worth exploring, I would recommend not to use normalize_path, but instead to implement the validation in zoneinfo._common. That is, wrap key in _validate_key() that protects against invalid paths. But in practice, it's better to do that closer to where the unsanitized data would be encountered (if at all).

    @jaraco
    Copy link
    Member

    jaraco commented Dec 28, 2021

    @miss-islington
    Copy link
    Contributor

    New changeset 00b2b57 by Jason R. Coombs in branch 'main':
    bpo-46124: Update zoneinfo to rely on importlib.resources traversable API. (GH-30190)
    00b2b57

    @jaraco
    Copy link
    Member

    jaraco commented Jan 21, 2022

    Closing, presumed fixed. Please re-open if not.

    @jaraco jaraco closed this as completed Jan 21, 2022
    @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.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants