Repository: pypa/wheel

Issue:
wheel unpack doesn't preserve permissions
I expected `wheel unpack` + `wheel pack` to result in similarly functioning wheels. However, `wheel unpack` doesn't seem to preserve permissions. That is, there's nothing in `wheel` that does this kind of thing: https://github.com/pypa/pip/blob/852deddb9c14afdb780e77ea28c2fa6d29f8c2e1/src/pip/_internal/utils/unpacking.py#L143 

Is this intentional?

Discussion:
codecov[bot]: ## [Codecov](https://codecov.io/gh/pypa/wheel/pull/514?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa) Report
Patch coverage: **`100.00`**% and project coverage change: **`-0.08`** :warning:
> Comparison is base [(`26d2e0d`)](https://codecov.io/gh/pypa/wheel/commit/26d2e0d18830b31f42c67715b68504b50e13021c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa) 72.76% compared to head [(`42e342c`)](https://codecov.io/gh/pypa/wheel/pull/514?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa) 72.68%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
- Coverage   72.76%   72.68%   -0.08%     
==========================================
  Files          13       13              
  Lines        1061     1058       -3     
==========================================
- Hits          772      769       -3     
  Misses        289      289              
```


| [Impacted Files](https://codecov.io/gh/pypa/wheel/pull/514?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa) | Coverage Δ | |
|---|---|---|
| [src/wheel/cli/unpack.py](https://codecov.io/gh/pypa/wheel/pull/514?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa#diff-c3JjL3doZWVsL2NsaS91bnBhY2sucHk=) | `100.00% <100.00%> (ø)` | |

... and [1 file with indirect coverage changes](https://codecov.io/gh/pypa/wheel/pull/514/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa)

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa)

</details>

[:umbrella: View full report in Codecov by Sentry](https://codecov.io/gh/pypa/wheel/pull/514?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pypa).

henryiii: This seems to have been a zipfile bug since at least 2012, https://github.com/python/cpython/issues/59999.
agronholm: I copied this technique from `pip`. Would you say this is good for merging?
henryiii: 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.
henryiii: 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 `spyder-5.4.2/spyder-5.4.2.data/data/share/applications/spyder.desktop` is now unpacking to `spyder-5.4.2/spyder-5.4.2.data/data/share/applications/spyder.desktop/spyder-5.4.2.data/data/share/applications/spyder.desktop`.
agronholm: Can you tell me how to repro?
henryiii: Repro:

```diff
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) & 0o111
```

Fix:

```diff
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")
```
henryiii: :D I had the fix almost immediately, getting the repro was the hard part. For some reason I was convinced `wf.write(str(script_path), "script")` was writing the word "script" to `script_path` inside the wheel, I think...

I'd recommend adding the patch for the tests, though, just to cover this case. Or a second test / parametrization with a nested file.
agronholm: I tried that repro patch against 933d556f, but it didn't fail.
henryiii: Checked it on pygal (from https://cs.github.com/?scopeName=All+repos&scope=&q=path%3Asetup.py+%22scripts%3D%22):

```console
$ 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 `x`'s aren't set, though? This only checks for `111`?
agronholm: > Looks better! What about if all x's aren't set, though? This only checks for 111?

No, it checks for *any* `x` (otherwise it would check `zinfo.external_attr >> 16 & 0o111 == 0o111` and not what it does now (`zinfo.external_attr >> 16 & 0o111` ).

agronholm: What about the repro though? Can you get it to fail against 933d556f?
henryiii: 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?
agronholm: Pip doesn't respect the original setting, but I guess `wheel unpack` is used for a different purpose, so I suppose I could just forego that umask stuff and set the exact same executable pattern on output.
henryiii: Got it:

```python
assert not script_path.is_dir()
```

Directories are executable. :)
henryiii: 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.

Patch:
diff --git a/docs/news.rst b/docs/news.rst
index 4961729..2fbe136 100644
--- a/docs/news.rst
+++ b/docs/news.rst
@@ -4,6 +4,7 @@ Release Notes
 **UNRELEASED**
 
 - Updated vendored ``packaging`` to 23.0
+- ``wheel unpack`` now preserves the executable attribute of extracted files
 - Fixed spaces in platform names not being converted to underscores (PR by David Tucker)
 - Fixed ``RECORD`` files in generated wheels missing the regular file attribute
 - Fixed ``DeprecationWarning`` about the use of the deprecated ``pkg_resources`` API
diff --git a/src/wheel/cli/unpack.py b/src/wheel/cli/unpack.py
index c6409d4..d48840e 100644
--- a/src/wheel/cli/unpack.py
+++ b/src/wheel/cli/unpack.py
@@ -18,6 +18,13 @@ def unpack(path: str, dest: str = ".") -> None:
         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:
+            wf.extract(zinfo, destination)
+
+            # Set permissions to the same values as they were set in the archive
+            # We have to do this manually due to
+            # https://github.com/python/cpython/issues/59999
+            permissions = zinfo.external_attr >> 16 & 0o777
+            destination.joinpath(zinfo.filename).chmod(permissions)
 
     print("OK")


Test patch:
diff --git a/tests/cli/test_unpack.py b/tests/cli/test_unpack.py
index e6a38bb..ae584af 100644
--- a/tests/cli/test_unpack.py
+++ b/tests/cli/test_unpack.py
@@ -1,6 +1,12 @@
 from __future__ import annotations
 
+import platform
+import stat
+
+import pytest
+
 from wheel.cli.unpack import unpack
+from wheel.wheelfile import WheelFile
 
 
 def test_unpack(wheel_paths, tmp_path):
@@ -10,3 +16,21 @@ def test_unpack(wheel_paths, tmp_path):
     """
     for wheel_path in wheel_paths:
         unpack(wheel_path, str(tmp_path))
+
+
+@pytest.mark.skipif(
+    platform.system() == "Windows", reason="Windows does not support the executable bit"
+)
+def test_unpack_executable_bit(tmp_path):
+    wheel_path = tmp_path / "test-1.0-py3-none-any.whl"
+    script_path = tmp_path / "script"
+    script_path.write_bytes(b"test script")
+    script_path.chmod(0o755)
+    with WheelFile(wheel_path, "w") as wf:
+        wf.write(str(script_path), "nested/script")
+
+    script_path.unlink()
+    script_path = tmp_path / "test-1.0" / "nested" / "script"
+    unpack(str(wheel_path), str(tmp_path))
+    assert not script_path.is_dir()
+    assert stat.S_IMODE(script_path.stat().st_mode) == 0o755