Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed wheel unpack not setting the executable bit #514

Merged
merged 5 commits into from
Mar 13, 2023
Merged

Fixed wheel unpack not setting the executable bit #514

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42e342c
Fixed `wheel unpack` not setting the executable bit for executable fi…
agronholm Mar 13, 2023
933d556
Fixed EncodingWarning
agronholm Mar 13, 2023
aa24fc4
Fixed unpack() unpacking files to wrong paths
agronholm Mar 13, 2023
ee19d11
Set the same permissions on output as in the archive
agronholm Mar 13, 2023
9d0249e
Link to the CPython bug
agronholm Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/news.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 11 additions & 1 deletion src/wheel/cli/unpack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations

import os
import stat
from pathlib import Path

from ..wheelfile import WheelFile
Expand All @@ -14,10 +16,18 @@ def unpack(path: str, dest: str = ".") -> None:
:param path: The path to the wheel.
:param dest: Destination directory (default to current directory).
"""
umask = os.umask(0)
os.umask(umask)
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
Copy link
Contributor

@henryiii henryiii Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zinfo already has the filename path, so this is duplicating the path, I think. a/b/c.txt -> a/b/a/b/c.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@henryiii henryiii Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@henryiii henryiii Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this only affects the non-package files, like .dist-info. The package files (for both packages) seem fine. Nevermind, happening to both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a deeper look, I saw the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically pushed almost the exact same fix right as you posted that comment 😄

wf.extract(zinfo, extracted_path)

# Set the executable bit if it was set in the archive
if stat.S_IMODE(zinfo.external_attr >> 16 & 0o111):
extracted_path.chmod(0o777 & ~umask | 0o111)

print("OK")
23 changes: 23 additions & 0 deletions tests/cli/test_unpack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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):
Expand All @@ -10,3 +16,20 @@ 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(0o777)
with WheelFile(wheel_path, "w") as wf:
wf.write(str(script_path), "script")

script_path.unlink()
script_path = script_path.parent / "test-1.0" / "script"
unpack(str(wheel_path), str(tmp_path))
assert stat.S_IMODE(script_path.stat().st_mode) & 0o111