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, kamilturek, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: easy, patch

Created on 2018-03-25 22:55 by eryksun, last changed 2021-06-30 20:14 by andrei.avk.

Pull Requests
URL Status Linked Edit
PR 26973 closed andrei.avk, 2021-06-30 18:32
Messages (8)
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) * 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) * 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) * Date: 2021-06-30 20:14
Steve: makes sense, I closed the PR; thanks for looking at this and taking the time to explain!
History
Date User Action Args
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