Fixed wheel unpack not setting the executable bit
#514
Conversation
…les in the archive Fixes #505.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
==========================================
+ Coverage 72.76% 72.83% +0.07%
==========================================
Files 13 13
Lines 1061 1064 +3
==========================================
+ Hits 772 775 +3
Misses 289 289
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
This seems to have been a zipfile bug since at least 2012, python/cpython#59999. |
|
I copied this technique from |
|
I think so, looks okay, but planning to check it in fully about an hour. I think this code path might get hit in the retagging command. |
|
Ah, never mind, that was an older version of the tags PR. The merged one doesn't go through this code. Something weird is happening, though. I tried this on something I thought had a script (based on a GitHub code search), and it seems |
|
Can you tell me how to repro? |
src/wheel/cli/unpack.py
Outdated
| with WheelFile(path) as wf: | ||
| namever = wf.parsed_filename.group("namever") | ||
| destination = Path(dest) / namever | ||
| print(f"Unpacking to: {destination}...", end="", flush=True) | ||
| wf.extractall(destination) | ||
| for zinfo in wf.filelist: | ||
| extracted_path = destination / zinfo.filename |
There was a problem hiding this comment.
zinfo already has the filename path, so this is duplicating the path, I think. a/b/c.txt -> a/b/a/b/c.txt.
There was a problem hiding this comment.
No, that makes no sense, as destination remains the same, only zinfo.filename keeps changing. I tested extracting Django-3.2.16-py3-none-any.whl (which happened to be lying around) with both this branch and main, and I didn't see any difference.
There was a problem hiding this comment.
You are setting destination with the filename, so now it has both. I think you want wf.extract(zinfo, destination) here. Extract extracts "a/b.txt" from argument 1 to argument 2, keeping the original relative path. Flatting this requires workarounds, see https://stackoverflow.com/a/47632134 for example. Are you sure you were using this branch?
tree Django-4.1.7/Django-4.1.7.dist-info
Django-4.1.7/Django-4.1.7.dist-info
├── AUTHORS
│ └── Django-4.1.7.dist-info
│ └── AUTHORS
├── LICENSE
│ └── Django-4.1.7.dist-info
│ └── LICENSE
├── LICENSE.python
│ └── Django-4.1.7.dist-info
│ └── LICENSE.python
├── METADATA
│ └── Django-4.1.7.dist-info
│ └── METADATA
├── RECORD
│ └── Django-4.1.7.dist-info
│ └── RECORD
├── WHEEL
│ └── Django-4.1.7.dist-info
│ └── WHEEL
├── entry_points.txt
│ └── Django-4.1.7.dist-info
│ └── entry_points.txt
└── top_level.txt
└── Django-4.1.7.dist-info
└── top_level.txt
There was a problem hiding this comment.
However, this only affects the non-package files, like Nevermind, happening to both..dist-info. The package files (for both packages) seem fine.
There was a problem hiding this comment.
On a deeper look, I saw the problem.
There was a problem hiding this comment.
I basically pushed almost the exact same fix right as you posted that comment 😄
|
Repro: diff --git a/tests/cli/test_unpack.py b/tests/cli/test_unpack.py
index e614aa9..9cd4a94 100644
--- a/tests/cli/test_unpack.py
+++ b/tests/cli/test_unpack.py
@@ -27,9 +27,9 @@ def test_unpack_executable_bit(tmp_path):
script_path.write_bytes(b"test script")
script_path.chmod(0o777)
with WheelFile(wheel_path, "w") as wf:
- wf.write(str(script_path), "script")
+ wf.write(str(script_path), "nested/script")
script_path.unlink()
- script_path = script_path.parent / "test-1.0" / "script"
+ script_path = tmp_path / "test-1.0" / "nested" / "script"
unpack(str(wheel_path), str(tmp_path))
assert stat.S_IMODE(script_path.stat().st_mode) & 0o111Fix: diff --git a/src/wheel/cli/unpack.py b/src/wheel/cli/unpack.py
index 17ef0fa..fc3ffa4 100644
--- a/src/wheel/cli/unpack.py
+++ b/src/wheel/cli/unpack.py
@@ -23,11 +23,11 @@ def unpack(path: str, dest: str = ".") -> None:
destination = Path(dest) / namever
print(f"Unpacking to: {destination}...", end="", flush=True)
for zinfo in wf.filelist:
- extracted_path = destination / zinfo.filename
- wf.extract(zinfo, extracted_path)
+ wf.extract(zinfo, destination)
# Set the executable bit if it was set in the archive
if stat.S_IMODE(zinfo.external_attr >> 16 & 0o111):
+ extracted_path = destination / zinfo.filename
extracted_path.chmod(0o777 & ~umask | 0o111)
print("OK") |
|
:D I had the fix almost immediately, getting the repro was the hard part. For some reason I was convinced I'd recommend adding the patch for the tests, though, just to cover this case. Or a second test / parametrization with a nested file. I pulled your fix, passes my test. |
|
I tried that repro patch against 933d556, but it didn't fail. |
|
Checked it on pygal (from https://cs.github.com/?scopeName=All+repos&scope=&q=path%3Asetup.py+%22scripts%3D%22): $ pip download --no-deps pygal
$ wheel unpack pygal-3.0.0-py2.py3-none-any.whl
$ ls -l pygal-3.0.0/pygal-3.0.0.data/scripts/pygal_gen.py
-rw-r--r-- 1 henryschreiner staff 2316 Mar 13 16:09 pygal-3.0.0/pygal-3.0.0.data/scripts/pygal_gen.py
$ gh pr checkout 514
$ wheel unpack pygal-3.0.0-py2.py3-none-any.whl
$ ls -l pygal-3.0.0/pygal-3.0.0.data/scripts/pygal_gen.py
-rwxr-xr-x 1 henryschreiner staff 2316 Mar 13 16:08 pygal-3.0.0/pygal-3.0.0.data/scripts/pygal_gen.py*Looks better! What about if all |
No, it checks for any |
|
What about the repro though? Can you get it to fail against 933d556? |
|
No, it's not failing. Not sure why, but I had a mistake when I first wrote the test, then didn't go back and check the original after I fixed the test after writing the patch. Not sure why it's passing, though. I know it checks for any x, but it looks like it sets all of them if one of them was set? Shouldn't it try to keep the original setting? (Pip's extraction probably doesn't care, but that's not trying to support round trips, just installing) |
|
Pip doesn't respect the original setting, but I guess |
|
Got it: add assert not script_path.is_dir()(I think maybe to the original, tested with the nesting). Directories are "executable". :) |
|
Yes, pip's goal is different, see my (maybe too slow) edit above. :) Unpack should replicate the exact original permission bits. It's just a bug in CPython that it doesn't work via extractall. |
|
Thanks for the review! It was really helpful. |
Fixes #505.