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.

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

Include directory entries when building wheel #289

Merged
merged 6 commits into from Apr 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/test_bdist_wheel.py
Expand Up @@ -8,6 +8,7 @@
from wheel.wheelfile import WheelFile

DEFAULT_FILES = {
'dummy_dist-1.0.dist-info/',
'dummy_dist-1.0.dist-info/top_level.txt',
'dummy_dist-1.0.dist-info/METADATA',
'dummy_dist-1.0.dist-info/WHEEL',
Expand Down
24 changes: 24 additions & 0 deletions tests/test_wheelfile.py
Expand Up @@ -2,6 +2,7 @@
from __future__ import unicode_literals

import sys
import operator
from zipfile import ZipFile, ZIP_DEFLATED

import pytest
Expand Down Expand Up @@ -172,3 +173,26 @@ def test_attributes(tmpdir_factory, wheel_path):
info = zf.getinfo('test-1.0.dist-info/RECORD')
permissions = (info.external_attr >> 16) & 0o777
assert permissions == 0o664


def test_directories(tmpdir, wheel_path):
"""
The WheelFile should contain entries for directories,
empty and not.
"""
build_dir = tmpdir
sub_dir = build_dir / 'sub'
sub_dir.mkdir()
(sub_dir / '__init__.py').write_text('', encoding='utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create a file in the directory? Wasn't the point to ensure that empty directories get included, or did I misunderstand?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I decided that it's probably better to match the behavior observed in other tools that generate zip files from a tree and create directory entries for every directory. But I do take from your point that the test should probably also ensure that directory entries are created for empty directories.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 0e94b10

empty_dir = build_dir / 'empty'
empty_dir.mkdir()

with WheelFile(wheel_path, 'w') as wf:
wf.write_files(str(build_dir))

with ZipFile(wheel_path, 'r') as zf:
infos = zf.infolist()

names = set(map(operator.attrgetter('filename'), infos))
assert 'sub/' in names
assert 'empty/' in names
15 changes: 14 additions & 1 deletion wheel/wheelfile.py
Expand Up @@ -111,6 +111,12 @@ def write_files(self, base_dir):
# Sort the directory names so that `os.walk` will walk them in a
# defined order on the next iteration.
dirnames.sort()

for name in dirnames:
path = os.path.normpath(os.path.join(root, name))
arcname = os.path.relpath(path, base_dir)
self.mkdir(path, arcname)

for name in sorted(filenames):
path = os.path.normpath(os.path.join(root, name))
if os.path.isfile(path):
Expand All @@ -136,12 +142,19 @@ def write(self, filename, arcname=None, compress_type=None):
zinfo.compress_type = ZIP_DEFLATED
self.writestr(zinfo, data, compress_type)

def mkdir(self, filename, arcname):
st = os.stat(filename)
zinfo = ZipInfo(arcname + '/', date_time=get_zipinfo_datetime(st.st_mtime))
zinfo.external_attr = st.st_mode << 16
zinfo.compress_type = ZIP_DEFLATED
self.writestr(zinfo, b'')

def writestr(self, zinfo_or_arcname, bytes, compress_type=None):
ZipFile.writestr(self, zinfo_or_arcname, bytes, compress_type)
fname = (zinfo_or_arcname.filename if isinstance(zinfo_or_arcname, ZipInfo)
else zinfo_or_arcname)
logger.info("adding '%s'", fname)
if fname != self.record_path:
if fname != self.record_path and not fname.endswith('/'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you take it out, the tests fail because there are RECORD entries for the directories (with a SHA hash for an empty file).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I see it's also used by the mkdir() method.

hash_ = self._default_algorithm(bytes)
self._file_hashes[fname] = hash_.name, native(urlsafe_b64encode(hash_.digest()))
self._file_sizes[fname] = len(bytes)
Expand Down