msg314436 - (view) |
Author: Eryk Sun (eryksun) * |
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) * |
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) * |
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) * |
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) * |
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!
|
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:59 | admin | set | github: 77321 |
2021-08-18 02:34:57 | eryksun | set | messages:
+ msg399815 |
2021-08-18 00:43:20 | rmast | set | nosy:
+ rmast messages:
+ msg399812
|
2021-08-06 01:23:14 | jack__d | set | messages:
+ msg399054 |
2021-08-05 20:48:56 | andrei.avk | set | messages:
+ msg399040 |
2021-08-05 20:27:13 | jack__d | set | files:
+ roughly_add_depr_warning.diff nosy:
+ jack__d messages:
+ msg399038
|
2021-06-30 20:14:28 | andrei.avk | set | messages:
+ msg396799 |
2021-06-30 20:08:04 | steve.dower | set | messages:
+ msg396798 |
2021-06-30 20:00:47 | andrei.avk | set | messages:
+ msg396797 |
2021-06-30 19:45:42 | steve.dower | set | messages:
+ msg396796 versions:
+ Python 3.11, - Python 3.8, Python 3.9, Python 3.10 |
2021-06-30 18:33:25 | andrei.avk | set | messages:
+ msg396789 |
2021-06-30 18:32:46 | andrei.avk | set | keywords:
+ patch nosy:
+ andrei.avk
pull_requests:
+ pull_request25537 stage: needs patch -> patch review |
2021-03-21 12:45:57 | kamilturek | set | nosy:
+ kamilturek
|
2021-03-20 02:26:15 | eryksun | set | title: shutil.chown on Windows -> shutil.chown should not be defined in Windows |
2021-03-20 02:24:48 | eryksun | set | keywords:
+ easy
messages:
+ msg389139 components:
- IO versions:
+ Python 3.9, Python 3.10, - Python 3.6, Python 3.7 |
2018-03-25 23:00:21 | eryksun | set | messages:
+ msg314437 |
2018-03-25 22:55:36 | eryksun | create | |