Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 33 additions & 0 deletions Lib/test/test_zipapp.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Test harness for the zipapp module."""

import io
import os
import pathlib
import stat
import sys
Expand Down Expand Up @@ -366,6 +367,38 @@ def test_shebang_is_executable(self):
zipapp.create_archive(str(source), str(target), interpreter='python')
self.assertTrue(target.stat().st_mode & stat.S_IEXEC)

@unittest.skipIf(sys.platform == 'win32',
'Windows does not support an executable bit')
@unittest.skipUnless(hasattr(os, 'umask'), 'test needs os.umask()')
@os_helper.skip_unless_working_chmod
def test_shebang_executable_bits_match_readable_bits(self):
source = self.tmpdir / 'source'
source.mkdir()
(source / '__main__.py').touch()
for umask, expected_mode in ((0o022, 0o755), (0o077, 0o700)):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't use octal constants here. They aren't understandable to readers who aren't familiar with the octal representation of Unix permissions (e.g., me...)

with self.subTest(umask=umask):
target = self.tmpdir / f'source-{umask:o}.pyz'
with os_helper.temp_umask(umask):
zipapp.create_archive(source, target, interpreter='python')
self.assertEqual(stat.S_IMODE(target.stat().st_mode), expected_mode)

@unittest.skipIf(sys.platform == 'win32',
'Windows does not support an executable bit')
@unittest.skipUnless(hasattr(os, 'umask'), 'test needs os.umask()')
@os_helper.skip_unless_working_chmod
def test_copied_shebang_executable_bits_match_readable_bits(self):
source = self.tmpdir / 'source'
source.mkdir()
(source / '__main__.py').touch()
archive = self.tmpdir / 'source.pyz'
zipapp.create_archive(source, archive)
for umask, expected_mode in ((0o022, 0o755), (0o077, 0o700)):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment about octal constants here.

with self.subTest(umask=umask):
target = self.tmpdir / f'target-{umask:o}.pyz'
with os_helper.temp_umask(umask):
zipapp.create_archive(archive, target, interpreter='python')
self.assertEqual(stat.S_IMODE(target.stat().st_mode), expected_mode)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't the existing tests also need fixing? At the very least, test_shebang_is_executable should assert that the newly created archive is readable, as well as executable, because if it's not, the test will fail.

@unittest.skipIf(sys.platform == 'win32',
'Windows does not support an executable bit')
def test_no_shebang_is_not_executable(self):
Expand Down
16 changes: 13 additions & 3 deletions Lib/zipapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ def _write_file_prefix(f, interpreter):
f.write(shebang)


def _make_executable(path):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rather this function required that path is a pathlib.Path object, and used its methods, rather than using raw os calls.

That requires changing the call in _copy_archive, as the argument there could currently be a str. (Longer term, I'd prefer it if we consistently used Path objects rather than repeatedly testing for "str or PathLike", but that's something to handle in a future PR, not here.)

mode = os.stat(path).st_mode
executable = (
(mode & stat.S_IRUSR) >> 2
| (mode & stat.S_IRGRP) >> 2
| (mode & stat.S_IROTH) >> 2
)
os.chmod(path, mode | executable)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not feel at all understandable. Why are we doing >> 2 bit operations instead of using the ST_IX* constants? Also, why are we checking S_IR* anyway? The docs just say that we make the file executable, not that we only make it executable when it's readable. I'm not totally opposed to this, I just want to see some justification - I'm not a Unix user, so I don't know what the typical expectation is when something is documented as "making a file executable".

Also, while I know this also applies to the existing code, the concept of an "executable bit" doesn't exist on Windows, so this code should be Unix-only. In fact the documentation explicitly says that this only happens on POSIX. We should fix this at the same time.



def _copy_archive(archive, new_archive, interpreter=None):
"""Copy an application archive, modifying the shebang line."""
with _maybe_open(archive, 'rb') as src:
Expand All @@ -69,8 +79,8 @@ def _copy_archive(archive, new_archive, interpreter=None):
dst.write(first_2)
shutil.copyfileobj(src, dst)

if interpreter and isinstance(new_archive, str):
os.chmod(new_archive, os.stat(new_archive).st_mode | stat.S_IEXEC)
if interpreter and isinstance(new_archive, (str, os.PathLike)):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for spotting and fixing the discrepancy between this line and the test in _maybe_open.

_make_executable(new_archive)


def create_archive(source, target=None, interpreter=None, main=None,
Expand Down Expand Up @@ -169,7 +179,7 @@ def create_archive(source, target=None, interpreter=None, main=None,
z.writestr('__main__.py', main_py.encode('utf-8'))

if interpreter and not hasattr(target, 'write'):
target.chmod(target.stat().st_mode | stat.S_IEXEC)
_make_executable(target)


def get_interpreter(archive):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix :mod:`zipapp` to set executable bits on archives with shebangs based on
their readable permission bits. Contributed by Xiao Yuan.
Loading