gh-96867: Set zipapp execute bits from readable bits

[yuanx749]

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.Path targets when copying an existing archive.

Tests cover archives created with different umask settings.

[pfmoore]

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?

[pfmoore]

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.

[pfmoore]

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

[pfmoore]

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.)

[pfmoore]

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.

[pfmoore]

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...)

[pfmoore]

Same comment about octal constants here.

[bedevere-app]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[yuanx749]

Thanks for the review. _make_executable is a private zipapp helper, not a proposed public API. It exists only to avoid duplicating this operation in the two output paths, and is called only after zipapp writes a shebang for a supplied interpreter.

Directly executing a shebang archive requires both read and execute permissions. This PR therefore maps each read bit to its corresponding execute bit, rather than adding execute bits for permission classes that still cannot run the archive.

umask	after creation	before PR	this PR (read to execute)	chmod +x	practical effect
Usual (0022)	rw-r--r-- (0644)	rwxr--r-- (0744)	rwxr-xr-x (0755)	rwxr-xr-x (0755)	Both this PR and chmod +x let all readers execute the archive.
Mask group/other execute (0011)	rw-rw-rw- (0666)	rwxrw-rw- (0766)	rwxrwxrwx (0777)	rwxrw-rw- (0766)	chmod +x leaves group/other readers unable to execute it directly.
Mask group/other read (0044)	rw--w--w- (0622)	rwx-w--w- (0722)	rwx-w--w- (0722)	rwx-wx-wx (0733)	chmod +x adds group/other execute bits without matching read bits, so those users still cannot run it.
Mask owner execute (0100)	rw-rw-rw- (0666)	rwxrw-rw- (0766)	rwxrwxrwx (0777)	rw-rwxrwx (0677)	chmod +x differs from the pre-PR behavior: 0766 lets the owner execute it, while 0677 gives a non-root owner EACCES.
Mask all execute (0111)	rw-rw-rw- (0666)	rwxrw-rw- (0766)	rwxrwxrwx (0777)	rw-rw-rw- (0666)	chmod +x leaves the archive non-executable.

Because chmod +x does not specify owner, group, or others, the umask controls which execute bits it adds. The rule here is instead a consistent shebang-specific rule: it extends the previous owner-only behavior to every permission class that can read the archive. It works for new and existing path targets, and avoids obtaining the umask via a temporary process-wide change, which would introduce a race with other threads.

I agree that the current name and bit shifts do not communicate this well. If the approach is acceptable, I will rename the helper, use pathlib.Path and explicit S_IR*/S_IX* checks, make the POSIX scope explicit, and update the tests.

Does this address your concern about the overall approach?

[pfmoore]

Thanks for the review. _make_executable is a private zipapp helper, not a proposed public API.

You're missing my point. I don't think we should have a private helper here, I think we should use a public API from one of the modules dedicated to filesystem operations. That public API doesn't exist yet, so it should be created before we do anything further here.

Regarding the rest of your comment, I understand the details of permission bits and umasks. The current behaviour is to just set the user execute bit - that's simplistic and easy to explain, but I can see why it's not ideal (as per #96867). But if we're changing the behaviour, I think the only reasonable thing to do is follow what chmod +x would do, as that's how someone would naturally make a zipapp executable if they had to do it manually. And as I say, I don't want code in the zipapp module that's trying to emulate chmod +x - if CPython is to have code to do that, it should be in an appropriate stdlib module (os, shutil, or pathlib, I don't have a strong preference).

So the long and short of this is that I think this PR should be put on hold until a decision is made on where and if a suitable chmod +x API is added to the stdlib. Once that exists, we can use it here. And if the decision is (for some reason) that we shouldn't have such an API, I'm going to reject #96867 on the basis that the stdlib has decided that chmod +x functionality won't be provided, and zipapp will follow that policy.