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 Path objects in the posix module #70215

Closed
serhiy-storchaka opened this issue Jan 6, 2016 · 27 comments
Closed

Support Path objects in the posix module #70215

serhiy-storchaka opened this issue Jan 6, 2016 · 27 comments
Assignees
Labels
extension-modules C modules in the Modules dir OS-windows type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 26027
Nosy @brettcannon, @pfmoore, @tjguk, @ethanfurman, @berkerpeksag, @vadmium, @zware, @serhiy-storchaka, @eryksun, @zooba, @JelleZijlstra
Dependencies
  • bpo-26671: Clean up path_converter in posixmodule.c
  • bpo-26800: Don't accept bytearray as filenames part 2
  • Files
  • path_converter_path.patch
  • issue27186-os_path_t.patch
  • path_converter.diff: Inline of PyOS_FSPath()
  • path_converter.diff: Updated patch since adding the warning about bytearrays
  • path_converter.diff: Incorporate Serhiy's review feedback
  • 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-08-27.19:59:15.038>
    created_at = <Date 2016-01-06.21:05:29.971>
    labels = ['extension-modules', 'type-feature', 'OS-windows']
    title = 'Support Path objects in the posix module'
    updated_at = <Date 2016-09-06.22:59:12.215>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-09-06.22:59:12.215>
    actor = 'python-dev'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2016-08-27.19:59:15.038>
    closer = 'berker.peksag'
    components = ['Extension Modules', 'Windows']
    creation = <Date 2016-01-06.21:05:29.971>
    creator = 'serhiy.storchaka'
    dependencies = ['26671', '26800']
    files = ['42382', '43238', '44029', '44090', '44159']
    hgrepos = []
    issue_num = 26027
    keywords = ['patch']
    message_count = 27.0
    messages = ['257642', '262963', '267427', '269212', '269220', '269797', '269801', '272061', '272556', '272650', '272677', '273161', '273734', '273735', '273745', '273748', '273749', '273754', '273786', '273788', '273791', '273792', '273793', '273799', '273800', '274650', '274654']
    nosy_count = 12.0
    nosy_names = ['brett.cannon', 'paul.moore', 'tim.golden', 'ethan.furman', 'python-dev', 'berker.peksag', 'martin.panter', 'zach.ware', 'serhiy.storchaka', 'eryksun', 'steve.dower', 'JelleZijlstra']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue26027'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    path_converter should be changed to accept objects with the "path" attribute. See bpo-22570 for details.

    @serhiy-storchaka serhiy-storchaka self-assigned this Jan 6, 2016
    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Jan 6, 2016
    @serhiy-storchaka
    Copy link
    Member Author

    Here is preliminary patch without tests. Writing tests will be tiresome.

    @JelleZijlstra
    Copy link
    Member

    The patch is obsolete because PEP-519 ended up going with a different approach. I submitted a new patch for the path_t converter in bpo-27186. That patch probably fits better here, so I'm resubmitting it.

    @brettcannon
    Copy link
    Member

    Did you still want to handle this, Serhiy, or should I assign the issue to myself?

    @serhiy-storchaka
    Copy link
    Member Author

    I'll take a look.

    @serhiy-storchaka
    Copy link
    Member Author

    At first glance bpo-27186-os_path_t.patch looks good.

    But with the patch applied the error message in case of incorrect argument type is always "expected str, bytes or os.PathLike object, not ...". Currently it is more detailed and specific: contains the function and the argument names, and is aware that some functions accept an integer or None. I think the best way is to inline the code of PyOS_FSPath in path_converter.

    Also we first resolve bpo-26800.

    @brettcannon
    Copy link
    Member

    I have no issues inlining -- with a comment about the inlining -- if it buys us better error messages in this critical case.

    @brettcannon
    Copy link
    Member

    Here is a version of Jelle's patch but with PyOS_FSPath() inlined.

    Serhiy, does this work for you?

    @brettcannon
    Copy link
    Member

    Here is an updated patch that adds in change to posixmodule.c stemming from the new warning about using bytearrays. It also makes type checking more stringent for what __fspath__() returns.

    @serhiy-storchaka
    Copy link
    Member Author

    Added comments on Rietveld. Needed tests for supporting path-like objects. And it would be nice to have few tests for error messages.

    @brettcannon
    Copy link
    Member

    Thanks for the review! I'll probably update the patch next week based on your feedback (which I agree with).

    As for error messages and tests, they exist in my patches on issues dependent on this one (e.g. the tests included in issues bpo-27524 and bpo-27182 which pull in Jelle's test from his original patch); I have been doing all of my os package-related changes in a single checkout and so this one isn't wholly written in isolation.

    @brettcannon
    Copy link
    Member

    Here is a patch that incorporates Serhiy's feedback.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 26, 2016

    New changeset b64f83d6ff24 by Brett Cannon in branch 'default':
    Issue bpo-26027, bpo-27524: Add PEP 519/fspath() support to os and
    https://hg.python.org/cpython/rev/b64f83d6ff24

    @brettcannon
    Copy link
    Member

    Thanks to Jelle for the initial commit and Serhiy for the code review!

    @vadmium
    Copy link
    Member

    vadmium commented Aug 27, 2016

    This change causes test_os to produce warnings, and can fail:

    $ hg update b64f83d6ff24
    $ ./python -bWerror -m test -u all -W test_os
    [. . .]

    ======================================================================
    ERROR: test_path_t_converter (test.test_os.PathTConverterTests) (name='stat', path=bytearray(b'@test_12055_tmp'))
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/media/disk/home/proj/python/cpython/Lib/test/test_os.py", line 2865, in test_path_t_converter
        result = fn(path, *extra_args)
    DeprecationWarning: stat: path should be string, bytes, os.PathLike or integer, not bytearray

    Similar warnings:
    DeprecationWarning: lstat: path should be string, bytes or os.PathLike, not bytearray
    DeprecationWarning: access: path should be string, bytes, os.PathLike or integer, not bytearray
    DeprecationWarning: open: path should be string, bytes or os.PathLike, not bytearray

    @vadmium vadmium reopened this Aug 27, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 27, 2016

    New changeset 32b93ba32aa0 by Brett Cannon in branch 'default':
    Issue bpo-26027: Don't test for bytearray in path_t as that's now
    https://hg.python.org/cpython/rev/32b93ba32aa0

    @brettcannon
    Copy link
    Member

    Thanks for catching that, Martin. I removed the test for bytearray as it was originally written before the deprecation.

    @vadmium
    Copy link
    Member

    vadmium commented Aug 27, 2016

    One more thing, ;) the Windows buildbots are failing to removing a temporary file:

    http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/8173/steps/test/logs/stdio

    ======================================================================
    ERROR: test_path_t_converter (test.test_os.PathTConverterTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\support\__init__.py", line 365, in unlink
        _unlink(filename)
      File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\support\__init__.py", line 336, in _unlink
        _waitfor(os.unlink, filename)
      File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\support\__init__.py", line 304, in _waitfor
        func(pathname)
    PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: '@test_5716_tmp'

    Subsequently, other tests fail, probably because this file already exists.

    @vadmium vadmium reopened this Aug 27, 2016
    @brettcannon
    Copy link
    Member

    Hopefully https://hg.python.org/cpython/rev/775158408ecb will fix the problem.

    @brettcannon
    Copy link
    Member

    It's still failing: http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/8176/steps/test/logs/stdio .

    Don't have time to look at why right now and I'm on a Mac ATM so I can't test locally to try and fix it until I'm at work on Monday. If someone has an idea as to why this is only happening on Windows I'm open to understanding.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 27, 2016

    New changeset 8ec5a00e5d75 by Berker Peksag in branch 'default':
    Issue bpo-26027: Fix test_path_t_converter on Windows
    https://hg.python.org/cpython/rev/8ec5a00e5d75

    @berkerpeksag
    Copy link
    Member

    test_path_t_converter failure looks similar to http://bugs.python.org/issue27493#msg271047 (fixed by 5424252ce174.) I've tested a fix on my Windows box and the test passed for me. Hopefully 8ec5a00e5d75 will fix the problem on buildbots too :)

    @eryksun
    Copy link
    Contributor

    eryksun commented Aug 27, 2016

    I wish the name was "pushCleanup" to emphasize that cleanup functions are popped and called in LIFO order.

    @brettcannon
    Copy link
    Member

    Thanks for fixing it, Berker!

    On Sat, Aug 27, 2016, 12:59 Berker Peksag <report@bugs.python.org> wrote:

    Berker Peksag added the comment:

    Builtbots look happy now:

    ----------
    status: open -> closed


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue26027\>


    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 6, 2016

    New changeset d0d9d7f55cb5 by Brett Cannon in branch 'default':
    Issue bpo-26027: Support path-like objects in PyUnicode-FSConverter().
    https://hg.python.org/cpython/rev/d0d9d7f55cb5

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 6, 2016

    New changeset 9be0286772bf by Brett Cannon in branch 'default':
    Issue bpo-26027, bpo-27524: Document the support for path-like objects in os and os.path.
    https://hg.python.org/cpython/rev/9be0286772bf

    @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 OS-windows type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants