classification
Title: shutil.chown should not be defined in Windows
Type: behavior Stage: patch review
Components: Library (Lib), Windows Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: andrei.avk, eryksun, jack__d, kamilturek, paul.moore, rmast, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: easy, patch

Created on 2018-03-25 22:55 by eryksun, last changed 2021-08-18 02:34 by eryksun.

Files
File name Uploaded Description Edit
roughly_add_depr_warning.diff jack__d, 2021-08-05 20:27
Pull Requests
URL Status Linked Edit
PR 26973 closed andrei.avk, 2021-06-30 18:32
Messages (13)
msg314436 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2018-03-25 22:55
shutil.chown is defined in Windows even though it's only written for Unix and only documented as available in Unix. Defining it should be skipped on Windows.

Possibly in 3.8 shutil.chown could be implemented on Windows by calling a new os.set_owner function that supports user/group names and SID strings. It could copy how icacls.exe allows using SDDL aliases and string SIDs that begin with an asterisk (e.g. "*BA" and "*S-1-32-544" for BUILTIN\Administrators). If the string starts with an asterisk, get the SID via ConvertStringSidToSid. Otherwise get the SID via LookupAccountName. Then to modify the file's user and group, try to enable SeRestorePrivilege for the current thread and call Set[Named]SecurityInfo.
msg314437 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2018-03-25 23:00
Oops, the SID for BUILTIN\Administrators is S-1-5-32-544. I left out the authority ID (5).
msg389139 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-03-20 02:24
Apparently there's no one else interested in implementing shutil.chown() in Windows. Okay, but as long as that's the case, the definition should be skipped in Windows, which is an easy problem.
msg396789 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-06-30 18:33
Created the PR: https://github.com/python/cpython/pull/26973/files
msg396796 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-06-30 19:45
We can't delete the definition without going through a deprecation process, as it will break existing code with a new exception at the point of access rather than the point of use. At best, we can short-circuit those errors and raise them with a more appropriate description.

For 3.11, we can deprecate the function on Windows and plan to remove it in 3.13. I don't see any problem with this, but it's a different change from PR 26973.
msg396797 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-06-30 20:00
Steve: it seems to me that the goal of normal deprecation process is, given that a functionality is available and documented, prepare to find a replacement for it by some future version.

In this case it's documented not to be available and doesn't work so this should perhaps fall under fixing a bug?

It's true that this is backwards incompatible for the reason you pointed out (and I missed), but I think breaking compatibility is ok in new versions for bug fixes? (as long as it's added to "what's new", which I haven't done yet).

If I'm wrong about this, I'll go ahead and make another PR for deprecation and close this one..
msg396798 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-06-30 20:08
I agree that's theoretically how it should go, but we've had enough examples of undocumented/buggy behaviour where the fix was worse than the bug (to the point where we brought back an undocumented C field that was deprecated in 3.0 because removing it in 3.8 broke too many people). 

A couple of versions with a warning on will reduce the surprise. People are (slowly) learning that DeprecationWarnings matter, so I don't think it'll be wasted effort.
msg396799 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-06-30 20:14
Steve: makes sense, I closed the PR; thanks for looking at this and taking the time to explain!
msg399038 - (view) Author: Jack DeVries (jack__d) * Date: 2021-08-05 20:27
I'm pretty sure the 3.11 dev cycle started since this conversation, right? Can we introduce the deprecation warning now? Maybe something like what is in the attached diff?

@andrei.avk, if it turns out that the time has come, you can go ahead and take the PR if you wish!
msg399040 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-08-05 20:48
Jack: I'm not sure if this eventual fix is worth doing.. it might cause issues for users to leave it as is, but can also cause issues to remove it even after deprecation period; I don't have a good feeling if one is more likely than the other, - so I'll have to leave it for others to review the PR; thanks for following up on this though!
msg399054 - (view) Author: Jack DeVries (jack__d) * Date: 2021-08-06 01:23
Yes, I definitely get that, but that's what the deprecation cycle is for. Certainly hold off on a PR until we see what @steve.dower thinks.

I personally feel that having a function that can be introspected with ``dir`` but which should not be used is confusing, and rightfully pointed out as a wrinkle in the API.
msg399812 - (view) Author: Ryan Mast (nightlark) (rmast) * Date: 2021-08-18 00:43
If this function were to be implemented on Windows would the preferred  approach be the one described in the initial message for this issue of creating a Windows `os.set_owner` function that uses the appropriate Windows API calls to change owner/group settings, and then using that for Windows in the `shutil.chown` function?
msg399815 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-08-18 02:34
> creating a Windows `os.set_owner` function that uses the 
> appropriate Windows API calls to change owner/group settings,
> and then using that for Windows in the `shutil.chown` function?

I'd gladly help with implementing os.set_owner(), as a separate issue. But I've changed my mind about supporting more POSIX functions in Windows. I'd prefer to see more Windows-only functionality added, as well as simplified cross-platform support that's not based on and biased by POSIX.
History
Date User Action Args
2021-08-18 02:34:57eryksunsetmessages: + msg399815
2021-08-18 00:43:20rmastsetnosy: + rmast
messages: + msg399812
2021-08-06 01:23:14jack__dsetmessages: + msg399054
2021-08-05 20:48:56andrei.avksetmessages: + msg399040
2021-08-05 20:27:13jack__dsetfiles: + roughly_add_depr_warning.diff
nosy: + jack__d
messages: + msg399038

2021-06-30 20:14:28andrei.avksetmessages: + msg396799
2021-06-30 20:08:04steve.dowersetmessages: + msg396798
2021-06-30 20:00:47andrei.avksetmessages: + msg396797
2021-06-30 19:45:42steve.dowersetmessages: + msg396796
versions: + Python 3.11, - Python 3.8, Python 3.9, Python 3.10
2021-06-30 18:33:25andrei.avksetmessages: + msg396789
2021-06-30 18:32:46andrei.avksetkeywords: + patch
nosy: + andrei.avk

pull_requests: + pull_request25537
stage: needs patch -> patch review
2021-03-21 12:45:57kamiltureksetnosy: + kamilturek
2021-03-20 02:26:15eryksunsettitle: shutil.chown on Windows -> shutil.chown should not be defined in Windows
2021-03-20 02:24:48eryksunsetkeywords: + easy

messages: + msg389139
components: - IO
versions: + Python 3.9, Python 3.10, - Python 3.6, Python 3.7
2018-03-25 23:00:21eryksunsetmessages: + msg314437
2018-03-25 22:55:36eryksuncreate