gh-96867: Set zipapp execute bits from readable bits - #151970
Conversation
pfmoore
left a comment
pfmoore
left a comment
There was a problem hiding this comment.
I've added some review comments below, but honestly I don't think this is the right fix in any case.
It shouldn't be zipapp's role to implement a function to (robustly) make a file executable. If that is useful functionality, it should be available in the os or pathlib libraries. We could then simply call that function.
Furthermore, the implementation here isn't even the recommended approach from #67679 (comment). Why did you choose this approach over that one? There are issues with that implementation (temporarily changing global state via umask is a bad idea in a multi-threaded program) but it does at least claim to have the benefit of exactly matching the behaviour of chmod +x.
Maybe you could propose the _make_executable function as a public function in os or pathlib, and then revisit this PR to simply use that function?
| | (mode & stat.S_IRGRP) >> 2 | ||
| | (mode & stat.S_IROTH) >> 2 | ||
| ) | ||
| os.chmod(path, mode | executable) |
There was a problem hiding this comment.
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.
|
|
||
| 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)): |
There was a problem hiding this comment.
Thanks for spotting and fixing the discrepancy between this line and the test in _maybe_open.
| f.write(shebang) | ||
|
|
||
|
|
||
| def _make_executable(path): |
There was a problem hiding this comment.
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.)
| with os_helper.temp_umask(umask): | ||
| zipapp.create_archive(archive, target, interpreter='python') | ||
| self.assertEqual(stat.S_IMODE(target.stat().st_mode), expected_mode) | ||
|
|
There was a problem hiding this comment.
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.
| source = self.tmpdir / 'source' | ||
| source.mkdir() | ||
| (source / '__main__.py').touch() | ||
| for umask, expected_mode in ((0o022, 0o755), (0o077, 0o700)): |
There was a problem hiding this comment.
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...)
| (source / '__main__.py').touch() | ||
| archive = self.tmpdir / 'source.pyz' | ||
| zipapp.create_archive(source, archive) | ||
| for umask, expected_mode in ((0o022, 0o755), (0o077, 0o700)): |
There was a problem hiding this comment.
Same comment about octal constants here.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
2 participants
Fixes gh-96867.
This PR sets execute bits to match readable bits when adding a shebang, preserving the permissions allowed by the user's umask. It also handles
pathlib.Pathtargets when copying an existing archive.Tests cover archives created with different umask settings.