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

Regression in pkgutil: iter_modules stopped taking Path argument in python 3.8.10 and 3.9.5 #88227

Closed
rikardnordgren mannequin opened this issue May 6, 2021 · 22 comments
Assignees
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes easy release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@rikardnordgren
Copy link
Mannequin

rikardnordgren mannequin commented May 6, 2021

BPO 44061
Nosy @gpshead, @ambv, @zooba, @shreyanavigyan, @miguendes, @magul
PRs
  • bpo-44061: Fix pkgutil.iter_modules regression #25964
  • [3.9] bpo-44061: Fix pkgutil.iter_modules regression when passed a pathlib.Path object (GH-25964) #26052
  • [3.8] bpo-44061: Fix pkgutil.iter_modules regression when passed a pathlib.Path object (GH-25964) #26056
  • 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/zooba'
    closed_at = <Date 2021-05-11.23:34:04.647>
    created_at = <Date 2021-05-06.17:54:01.802>
    labels = ['easy', 'type-bug', '3.8', '3.9', '3.10', '3.11', 'release-blocker', 'library']
    title = 'Regression in pkgutil: iter_modules stopped taking Path argument in python 3.8.10 and 3.9.5'
    updated_at = <Date 2021-05-12.10:18:11.716>
    user = 'https://bugs.python.org/rikardnordgren'

    bugs.python.org fields:

    activity = <Date 2021-05-12.10:18:11.716>
    actor = 'lukasz.langa'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2021-05-11.23:34:04.647>
    closer = 'steve.dower'
    components = ['Library (Lib)']
    creation = <Date 2021-05-06.17:54:01.802>
    creator = 'rikard.nordgren'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44061
    keywords = ['patch', 'easy', '3.8regression', '3.9regression']
    message_count = 12.0
    messages = ['393120', '393128', '393130', '393132', '393133', '393135', '393147', '393154', '393168', '393478', '393494', '393498']
    nosy_count = 7.0
    nosy_names = ['gregory.p.smith', 'lukasz.langa', 'steve.dower', 'shreyanavigyan', 'miguendes', 'rikard.nordgren', 'magul']
    pr_nums = ['25964', '26052', '26056']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue44061'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10', 'Python 3.11']

    @rikardnordgren
    Copy link
    Mannequin Author

    rikardnordgren mannequin commented May 6, 2021

    The pkgutil.iter_modules crash when using Path object in the first argument. The code below works in python 3.8.9 and 3.9.4, but stopped working in python 3.8.10 and 3.9.5. Changing from Path to str works in all versions.

    import pkgutil
    from pathlib import Path

    for _, modname, ispkg in pkgutil.iter_modules([Path("/home")], 'somepackage.somesubpackage'):
    print(modname, ispkg)

    Error message from python 3.8.10 (other path was used):

    Traceback (most recent call last):
      File "/home/devel/Python-3.8.10/Lib/pkgutil.py", line 415, in get_importer
        importer = sys.path_importer_cache[path_item]
    KeyError: PosixPath('/home/devel/pharmpy/src/pharmpy/plugins')
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "pyissue.py", line 5, in <module>
        for _, modname, ispkg in pkgutil.iter_modules([Path("/home/devel/pharmpy/src/pharmpy/plugins")], 'pharmpy.plugins'):
      File "/home/devel/Python-3.8.10/Lib/pkgutil.py", line 129, in iter_modules
        for i in importers:
      File "/home/devel/Python-3.8.10/Lib/pkgutil.py", line 419, in get_importer
        importer = path_hook(path_item)
      File "<frozen importlib._bootstrap_external>", line 1594, in path_hook_for_FileFinder
      File "<frozen importlib._bootstrap_external>", line 1469, in __init__
      File "<frozen importlib._bootstrap_external>", line 177, in _path_isabs
    AttributeError: 'PosixPath' object has no attribute 'startswith'

    @rikardnordgren rikardnordgren mannequin added type-crash A hard crash of the interpreter, possibly with a core dump 3.8 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir labels May 6, 2021
    @gpshead
    Copy link
    Member

    gpshead commented May 6, 2021

    https://bugs.python.org/issue43105 and #25121 caused this regression.

    @gpshead gpshead added 3.10 only security fixes release-blocker labels May 6, 2021
    @gpshead gpshead added 3.10 only security fixes release-blocker labels May 6, 2021
    @gpshead
    Copy link
    Member

    gpshead commented May 6, 2021

    (obviously we're missing any tests for use of Path objects in this situation)

    @gpshead gpshead added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels May 6, 2021
    @zooba
    Copy link
    Member

    zooba commented May 6, 2021

    Looks like it was always getting lucky in the past, as sys.path requires strings, and the "path" argument here is an alternative to it. The cache was definitely not working as intended.

    So while it's not clearly documented anywhere (other than the related pkgutil.extend_path() method saying it'll ignore non-str paths), we do need to only be passing str into _importlib_bootstrap.

    I can't do the pkgutil updates right now, but I think it's just get_importer than needs an os.fsdecode() call around its argument. Maybe some others in the same module. And add tests :)

    @zooba zooba added easy labels May 6, 2021
    @zooba
    Copy link
    Member

    zooba commented May 6, 2021

    To be clear, I'll get to this when I can, but if someone else wants to write the fix I'm happy to review and merge. Don't wait for me to do this one just because I'm assigned.

    @zooba zooba added 3.11 bug and security fixes labels May 6, 2021
    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented May 6, 2021

    It's mentioned that Python 3.9.5 has this regression but the code works fine on my windows machine. Is this only reproducible on POSIX?

    @shreyanavigyan shreyanavigyan mannequin removed 3.11 bug and security fixes labels May 6, 2021
    @shreyanavigyan shreyanavigyan mannequin added 3.11 bug and security fixes labels May 6, 2021
    @miguendes
    Copy link
    Mannequin

    miguendes mannequin commented May 6, 2021

    I can reproduce it on latest master running on Linux.

    steve.dower: I wrote some tests and wrapping get_importer argument with os.fsdecode() fixes it.

    I'm happy to open an PR, just let me know or if OP is not willing to do so either.

    @zooba
    Copy link
    Member

    zooba commented May 6, 2021

    I can reproduce it on latest master running on Linux.

    Interesting. Perhaps my actual change was to cause it to raise an error
    earlier (outside of a handler)? Which would mean that the Path object
    was always failing before, just silently.

    Arguably we should filter them out instead, then, rather than
    introducing a new supported type (as has been argued with similar
    changes elsewhere) in a security branch. Though I'm sure the OP would
    have noticed if their call was never returning any modules, so
    presumably it must have worked somehow.

    I'm happy to open an PR, just let me know or if OP is not willing to do so either.

    Feel free. I assume they'd have arrived with a PR if they were keen to
    write it themselves.

    @rikardnordgren
    Copy link
    Mannequin Author

    rikardnordgren mannequin commented May 7, 2021

    Thanks all for looking into this!

    Though I'm sure the OP would
    have noticed if their call was never returning any modules, so
    presumably it must have worked somehow.

    Yes, that snippet returns modules with the previous versions of python 3.8 and 3.9.

    For more background to what I tested please see:
    actions/runner-images#3330

    I'm happy to open an PR, just let me know or if OP is not willing to do so either.

    Please go ahead!

    @zooba
    Copy link
    Member

    zooba commented May 11, 2021

    Thanks for the patch!

    I don't think this meets the 3.8 bar now, but if Łukasz wants it then it should be easy.

    @zooba zooba closed this as completed May 11, 2021
    @zooba zooba closed this as completed May 11, 2021
    @ambv
    Copy link
    Contributor

    ambv commented May 12, 2021

    New changeset 0cb9775 by Steve Dower in branch '3.9':
    bpo-44061: Fix pkgutil.iter_modules regression when passed a pathlib.Path object (GH-25964) (GH-26052)
    0cb9775

    @ambv
    Copy link
    Contributor

    ambv commented May 12, 2021

    New changeset 4844abd by Łukasz Langa in branch '3.8':
    [3.8] bpo-44061: Fix pkgutil.iter_modules regression when passed a pathlib.Path object (GH-25964). (GH-26056)
    4844abd

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @dsp25no
    Copy link

    dsp25no commented May 12, 2022

    Issue still actual for python 3.10.4

    @aniongithub
    Copy link

    I'm on Python 3.8.10 (Ubuntu 20.04) and still see this issue

    @caleb-depotanalytics
    Copy link

    Same, seeing this issue on Python 3.8.10 with ubuntu

    @zooba
    Copy link
    Member

    zooba commented Aug 17, 2022

    Yes, you'll need to get a newer version of 3.8. If Ubuntu doesn't offer one, you'll need to speak to them - we released it already, but we can't insert it into someone else's operating system.

    @mpf82
    Copy link

    mpf82 commented Apr 26, 2023

    We can still see this issue in Python 3.10.11 on Red Hat

    When searching https://docs.python.org/release/3.10.11/whatsnew/changelog.html#changelog there's no mention of bpo-44061

    But it seems to be fixed in 3.11 - https://docs.python.org/release/3.11.2/whatsnew/changelog.html#changelog

    And also in 3.8 and 3.9, but somehow 3.10 was skipped.

    @zooba
    Copy link
    Member

    zooba commented Apr 28, 2023

    Whoops. It probably came up just as the 3.10 branch was made, and so didn't get explicitly backported to it.

    I suspect a backport would be okay still, though you could also ask Red Hat to do it.

    @encukou
    Copy link
    Member

    encukou commented Apr 28, 2023

    Whoops. It probably came up just as the 3.10 branch was made, and so didn't get explicitly backported to it.
    I suspect a backport would be okay still

    That's @pablogsal's call. is a forgotten backport worth an exception?

    you could also ask Red Hat to do it.

    Red Hat doesn't ship 3.10, it'll go straight to 11 :)
    I assume this is a Python from another source, installed on a Red Hat OS.

    @gforcada
    Copy link

    (for what is worth) we are also being hit by this issue not being backported to 3.10 😕

    I see that since August 2023 there has been no release of 3.10, from what I see on the PEP 619 security releases are made "as-needed", so we don't have much chances to get this, right? 🤔

    @zooba
    Copy link
    Member

    zooba commented Jan 2, 2024

    we don't have much chances to get this, right?

    Not from us, though it's a one line patch for your own installs (or the code that's calling into it) to use os.fsdecode here. Or migrating to 3.11/3.12 ought to be easy from the Python side, and will get you other benefits (I assume that since this wasn't discovered earlier that you got to 3.10 after it was already done with maintenance releases... is there a reason you didn't go straight to one of the active versions at that time?)

    @gpshead
    Copy link
    Member

    gpshead commented Jan 2, 2024

    my 2 cents: for this kind of thing when you cannot control your specific python runtime: write some code that detects if the problem is present in a given python runtime (basically a pkgutil.get_importer test case?) and use monkeypatching to hot-fix the issue if so. due to the nature of the bugfix that happens to be feasible for this one.

    a functionality test case rather than a specific version check is ideal because it is fair to assume that others might also do the same and that some runtimes might have a backported patch already applied despite version numbers, etc.

    facebook-github-bot pushed a commit to facebookincubator/cinder that referenced this issue Jan 3, 2024
    …Path object (GH-25964)
    
    Summary:
    upstream issue: python/cpython#88227
    upstream PR: python/cpython#25964
    upstream commit: python/cpython@e9d7f88
    
    Reviewed By: carljm
    
    Differential Revision: D52495633
    
    fbshipit-source-id: b6d1b3dab4a66d63e4a0171542b236a663b3d6fb
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes 3.10 only security fixes 3.11 bug and security fixes easy release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    9 participants