gh-59999: Add option to preserve permissions in ZipFile.extract #32289
Conversation
|
Thanks for making this. I didn't really get back to my PR after it look a year to be reviewed. What I did differently is that I made preserving the permissions the default. The reason for this is that the unzip command which comes with any Linux distribution, MacOS or BSD system will preserve permissions by default as well. So the user will probably expect that to be the default. |
It's no problem
On the BPO issue (#59999) I proposed not doing this to maintain backwards compatibility |
|
Makes some sense, thanks for clarifying. Keep up the good work! |
|
Running test_zipfile I get two errors: Traceback (most recent call last): FAIL: test_extractall_preserve_none (test.test_zipfile.TestsPermissionExtraction.test_extractall_preserve_none) Ran 274 tests in 94.070s |
|
Ok, interesting. What OS and file system are you running this on? And, do the other test_extract tests pass, or were some of them skipped? And, do tests still fail on 6470201? |
|
I am running Fedora 35. I did a clean start (compiling python from scratch just in case) and ran the test again and got the same result. Other test_extract tests are ok ( except I don't know what you mean by 6470201, I didn't see any tests there. |
Oh of course, they didn't exist in that revision. I've just checked and the default mode for created files differs on Fedora. On CI and my system it's 644 on Fedora it seems to be 664. I'll write the test so it checks against what the default for the system is instead of the 0o644 constant. |
Co-authored-by: Éric <merwok@netwok.org>
Co-authored-by: Éric <merwok@netwok.org>
Lib/zipfile/__init__.py
Outdated
| # Ignore permissions if the archive was created on Windows | ||
| if member.create_system == 0 or preserve_permissions == PreserveMode.NONE: | ||
| return targetpath | ||
|
|
||
| if preserve_permissions == PreserveMode.SAFE: | ||
| mode = (member.external_attr >> 16) & 0o777 | ||
| elif preserve_permissions == PreserveMode.ALL: | ||
| mode = (member.external_attr >> 16) & 0xFFFF | ||
|
|
||
| os.chmod(targetpath, mode) |
There was a problem hiding this comment.
Looking at the diff today, it's easy to see this code is related to a single purpose. After merging, however, it becomes an inline part of a larger narrative.
For future readers, it might be clearer to have this behavior extracted as its own method.
e.g.
def _apply_permissions(self, member, path, mode):
if mode == PreserveMode.NONE:
return path
# Ignore permissions if the archive was created on Windows
if member.create_system == 0:
return path
mask = {
PreserveMode.SAFE: 0o777,
PreserveMode.ALL: 0xFFFF,
}
new_mode = (member.external_attr >> 16) & mask[mode]
os.chmod(path, new_mode)
return path
Then in this method, return self._apply_permissions(member, targetpath, preserve_permissions).
I also took the liberty to address a couple of nitpicks in the approach, such as to align the comment with the check, and to only perform the new mode calculation in a single place. WDYT?
There was a problem hiding this comment.
Definitely agree, although I changed _apply_permissions to not return anything. How do you feel about that?
cpython/Lib/zipfile/__init__.py
Lines 1898 to 1903 in a2d7e77
cpython/Lib/zipfile/__init__.py
Lines 1835 to 1852 in a2d7e77
Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
|
(Please follow the devguide: avoid force pushes, they create notifications with broken links for reviewers, and make it harder to see changes compared to previous time. Thanks!) |
|
Seems another look is needed from a reviewer? |
Co-authored by Alexey Boriskin
https://bugs.python.org/issue15795
TODO