classification
Title: Support Path objects in the posix module
Type: enhancement Stage: resolved
Components: Extension Modules, Windows Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: 26671 26800 Superseder:
Assigned To: brett.cannon Nosy List: Jelle Zijlstra, berker.peksag, brett.cannon, eryksun, ethan.furman, martin.panter, paul.moore, python-dev, serhiy.storchaka, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2016-01-06 21:05 by serhiy.storchaka, last changed 2016-09-06 22:59 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
path_converter_path.patch serhiy.storchaka, 2016-04-06 20:33 review
issue27186-os_path_t.patch Jelle Zijlstra, 2016-06-05 16:34 review
path_converter.diff brett.cannon, 2016-08-05 21:56 Inline of PyOS_FSPath() review
path_converter.diff brett.cannon, 2016-08-12 18:50 Updated patch since adding the warning about bytearrays review
path_converter.diff brett.cannon, 2016-08-19 22:09 Incorporate Serhiy's review feedback review
Messages (27)
msg257642 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-01-06 21:05
path_converter should be changed to accept objects with the "path" attribute. See issue22570 for details.
msg262963 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-06 20:33
Here is preliminary patch without tests. Writing tests will be tiresome.
msg267427 - (view) Author: Jelle Zijlstra (Jelle Zijlstra) * (Python triager) Date: 2016-06-05 16:34
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 issue27186. That patch probably fits better here, so I'm resubmitting it.
msg269212 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-06-24 21:03
Did you still want to handle this, Serhiy, or should I assign the issue to myself?
msg269220 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-25 05:41
I'll take a look.
msg269797 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-04 16:50
At first glance issue27186-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 issue26800.
msg269801 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-07-04 17:28
I have no issues inlining -- with a comment about the inlining -- if it buys us better error messages in this critical case.
msg272061 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-05 21:56
Here is a version of Jelle's patch but with PyOS_FSPath() inlined.

Serhiy, does this work for you?
msg272556 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-12 18:50
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.
msg272650 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-14 06:03
Added comments on Rietveld. Needed tests for supporting path-like objects. And it would be nice to have few tests for error messages.
msg272677 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-14 16:00
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 #27524 and #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.
msg273161 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-19 22:09
Here is a patch that incorporates Serhiy's feedback.
msg273734 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-26 21:45
New changeset b64f83d6ff24 by Brett Cannon in branch 'default':
Issue #26027, #27524: Add PEP 519/__fspath__() support to os and
https://hg.python.org/cpython/rev/b64f83d6ff24
msg273735 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-26 21:46
Thanks to Jelle for the initial commit and Serhiy for the code review!
msg273745 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-27 01:56
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
msg273748 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-27 02:30
New changeset 32b93ba32aa0 by Brett Cannon in branch 'default':
Issue #26027: Don't test for bytearray in path_t as that's now
https://hg.python.org/cpython/rev/32b93ba32aa0
msg273749 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-27 02:31
Thanks for catching that, Martin. I removed the test for bytearray as it was originally written before the deprecation.
msg273754 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-27 03:16
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.
msg273786 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-27 16:43
Hopefully https://hg.python.org/cpython/rev/775158408ecb will fix the problem.
msg273788 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-27 17:11
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.
msg273791 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-27 18:25
New changeset 8ec5a00e5d75 by Berker Peksag in branch 'default':
Issue #26027: Fix test_path_t_converter on Windows
https://hg.python.org/cpython/rev/8ec5a00e5d75
msg273792 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-08-27 18:31
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 :)
msg273793 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-08-27 18:39
I wish the name was "pushCleanup" to emphasize that cleanup functions are popped and called in LIFO order.
msg273799 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-08-27 19:59
Builtbots look happy now:

* Windows 7 SP1: http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/8177
* Windows 8: http://buildbot.python.org/all/builders/AMD64%20Windows8%203.x/builds/2476
* Windows 10: http://buildbot.python.org/all/builders/AMD64%20Windows10%203.x/builds/1377
msg273800 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-08-27 21:09
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:
>
> * Windows 7 SP1:
> http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/8177
> * Windows 8:
> http://buildbot.python.org/all/builders/AMD64%20Windows8%203.x/builds/2476
> * Windows 10:
> http://buildbot.python.org/all/builders/AMD64%20Windows10%203.x/builds/1377
>
> ----------
> status: open -> closed
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue26027>
> _______________________________________
>
msg274650 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-06 22:50
New changeset d0d9d7f55cb5 by Brett Cannon in branch 'default':
Issue #26027: Support path-like objects in PyUnicode-FSConverter().
https://hg.python.org/cpython/rev/d0d9d7f55cb5
msg274654 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-06 22:59
New changeset 9be0286772bf by Brett Cannon in branch 'default':
Issue #26027, #27524: Document the support for path-like objects in os and os.path.
https://hg.python.org/cpython/rev/9be0286772bf
History
Date User Action Args
2016-09-06 22:59:12python-devsetmessages: + msg274654
2016-09-06 22:50:48python-devsetmessages: + msg274650
2016-08-27 21:09:12brett.cannonsetmessages: + msg273800
2016-08-27 19:59:15berker.peksagsetstatus: open -> closed

messages: + msg273799
2016-08-27 18:39:35eryksunsetnosy: + eryksun
messages: + msg273793
2016-08-27 18:31:36berker.peksagsetnosy: + berker.peksag
messages: + msg273792
2016-08-27 18:25:18python-devsetmessages: + msg273791
2016-08-27 17:11:46brett.cannonsetstatus: closed -> open

nosy: + paul.moore, tim.golden, zach.ware, steve.dower
messages: + msg273788

components: + Windows
2016-08-27 16:43:18brett.cannonsetstatus: open -> closed

messages: + msg273786
2016-08-27 03:16:56martin.pantersetstatus: closed -> open

messages: + msg273754
2016-08-27 02:31:11brett.cannonsetstatus: open -> closed

messages: + msg273749
2016-08-27 02:30:19python-devsetmessages: + msg273748
2016-08-27 01:56:37martin.pantersetstatus: closed -> open
nosy: + martin.panter
messages: + msg273745

2016-08-26 21:46:03brett.cannonsetstatus: open -> closed
resolution: fixed
messages: + msg273735

stage: commit review -> resolved
2016-08-26 21:45:25python-devsetnosy: + python-dev
messages: + msg273734
2016-08-19 22:10:40brett.cannonsetstage: test needed -> commit review
2016-08-19 22:09:42brett.cannonsetfiles: + path_converter.diff

messages: + msg273161
2016-08-14 16:00:35brett.cannonsetmessages: + msg272677
2016-08-14 06:03:58serhiy.storchakasetassignee: serhiy.storchaka -> brett.cannon
messages: + msg272650
stage: commit review -> test needed
2016-08-12 18:50:11brett.cannonsetfiles: + path_converter.diff

messages: + msg272556
2016-08-05 21:56:31brett.cannonsetfiles: + path_converter.diff

messages: + msg272061
stage: patch review -> commit review
2016-07-15 21:06:56brett.cannonlinkissue27524 dependencies
2016-07-04 17:28:29brett.cannonsetmessages: + msg269801
2016-07-04 16:50:47serhiy.storchakasetdependencies: + Don't accept bytearray as filenames part 2
messages: + msg269797
2016-06-25 05:41:27serhiy.storchakasetmessages: + msg269220
2016-06-24 21:03:23brett.cannonsetmessages: + msg269212
2016-06-05 16:34:21Jelle Zijlstrasetfiles: + issue27186-os_path_t.patch
nosy: + Jelle Zijlstra
messages: + msg267427

2016-06-02 17:38:54ethan.furmanlinkissue27182 dependencies
2016-06-02 17:14:33ethan.furmansetnosy: + ethan.furman
2016-04-06 20:33:56serhiy.storchakasetfiles: + path_converter_path.patch
keywords: + patch
messages: + msg262963

stage: needs patch -> patch review
2016-03-30 09:31:48serhiy.storchakasetdependencies: + Clean up path_converter in posixmodule.c
2016-03-29 18:53:38serhiy.storchakalinkissue26667 dependencies
2016-01-10 18:23:01gvanrossumunlinkissue22570 dependencies
2016-01-07 17:52:39brett.cannonsetnosy: + brett.cannon
2016-01-06 21:08:42serhiy.storchakalinkissue22570 dependencies
2016-01-06 21:05:29serhiy.storchakacreate