install-wheel: add support for specifying python startup flags by eli-schwartz · Pull Request #13 · gentoo/gpep517

eli-schwartz · 2023-11-21T04:03:10Z

gpep517 install-wheel --shebang-flags='-s'

will cause all /usr/bin scripts to pass -s to the interpreter in its shebang. Useful for ensuring that system commands do not import mismatched modules from pip install --user.

Also useful in theory to produce programs that run via -OO, something the python ecosystem rarely remembers is possible because scripts cannot be easily fine-tuned and short of changing the entrypoint script you cannot activate -O. Perhaps we could change that.

mgorny

Please don't forget to handle updates to your "prerequirement" upstream PR and finally file this as a PR for upstream.

mgorny · 2023-12-02T10:14:12Z

    from installer.sources import WheelFile
    from installer.utils import get_launcher_kind

+    from .scheme import Gpep517WheelDestination


I don't use relative imports. Be consistent.

mgorny · 2023-12-02T10:16:37Z

+from __future__ import annotations
+
+import contextlib, io, os, shlex, zipfile
+import typing as T


Inconsistent.

Again, all this does is make files longer and more annoying to read, since most type annotations suddenly gain an additional 5 characters of overhead towards the max line length.

Either that or you spend more time managing individual from typing import Foo, Bar, Baz imports than you spend coding.

In preparation for rebasing... I'd still like an answer about this.

I've found when reading and contributing to many python codebases that usage of symbols from the typing module tend to shift around a lot.

Importing typing and referencing typing.Iterable leads to overly long lines that are cramped and hard to read, especially if a line has more than one typing symbol on a single line (multiple parameters to a function definition, use of Union, container types such as Dict and List and Mapping that have other symbols inside). Tonaid in readability, I was introduced to, and gratefully started using myself, the convention to import typing as T which saves a lot of space. "T" is very recognizable and there aren't a lot of use cases for a one-character symbol other than that. The main other use case is for loop iteration, but that is usually "i" or "k, v". And anyways, the use of upper case is a well established convention for global constants, and the typing module is global and contains constants, so... :)

I think it's a good convention and it aids in readability, so if possible I'd like to be able to come to an agreement that this "as" import can stay and perhaps be used in other files as well.

I've never done it in this project, or any of the other of my projects, and I never had a real need to. Hence, I don't see the point of suddenly changing this now.

mgorny · 2023-12-02T10:17:47Z

+# with minor tweaks where classes don't have hooks to control their
+# internals.
+
+from __future__ import annotations


Is this really needed?

It is required to make if T.TYPE_CHECKING imports not be runtime errors. It is also a small performance improvement for all code that has type annotations, period.

Type annotations are not needed at runtime unless you're using something like beartype or pydantic. Building them into code objects and discarding them is a bit wasteful.

The import is equivalent to surrounding all type annotations with quotes. I find the future import to be more ergonomic, and could add it to other files if you like... ?

Yeah, feel free to add them everywhere.

``` gpep517 install-wheel --shebang-flags='-s' ``` will cause all /usr/bin scripts to pass `-s` to the interpreter in its shebang. Useful for ensuring that system commands do not import mismatched modules from `pip install --user`. Also useful in theory to produce programs that run via -OO, something the python ecosystem rarely remembers is possible because scripts cannot be easily fine-tuned and short of changing the entrypoint script you cannot activate -O. Perhaps we could change that.

eli-schwartz · 2023-12-13T01:30:58Z

Updated.

eli-schwartz · 2024-05-03T15:46:18Z

Any update on what I should do here? Upstream PyPA/installer hasn't merged any PRs in months other than pre-commit.ci spam and isn't responsive to requests for review.

Are we still waiting on them or is it fine to reimplement functionality locally?

Are there any responses to the answers I gave on review comments where I disagreed with the review?

arthurzam · 2025-04-29T17:00:44Z

@mgorny mind explaining why this PR was closed?

I see a request for further comments from the author, with no replies. A little bit of more explanation would be nice.

mgorny · 2025-04-29T17:40:40Z

OP was requested to submit the pull request upstream. Nothing new has happened.

thesamesam · 2025-04-29T17:46:32Z

It took ages for pypa/installer#200 to be merged and no answer was given to the questions directed to you above, including whether there's a need to wait for installer to merge every single change (which may take years) or not. Nor the other parts.

mgorny · 2025-04-29T17:49:14Z

The PR was merged eventually.

thesamesam · 2025-07-23T18:01:10Z

Yes, eventually. pypa/installer#276 seems stuck now too.

Let's please not wait on installer (where we've had various issues before, like with --prefix). Can you review this again?

arthurzam · 2025-07-23T18:46:53Z

I'm also in support for not waiting for upstream, I suspect it would be easier to drop support when they do accept it (in some years).

I would recommend to mention in the script docs, and the special argument passed, that they are temporary until upstream merges, so non-Gentoo projects that use gpep517 know this might break in the future.

mgorny

Sure, I can live with it. Still, it needs to be rebased and the new code needs to follow the same coding style as the existing code.

mgorny · 2025-07-24T19:08:44Z

+
+    if forlauncher or simple:
+        shebang = '#!' + executable + post_interp_
+    else:


Do we actually need this? I don't think we really expect Gentoo Python executables that long, or support spaces in EPREFIX. I get that it's upstream code, but since we're duplicating it, better duplicate less of it.

I think so. There are people outside of Gentoo that use gpep517, e.g. on the distro side Alpine. But also I've mentioned it a few times to people in #python and similar when the topic of installing wheels without pip has come up, and they have found it interesting and useful.

mgorny · 2025-07-24T19:10:22Z

+        # gpep517: use build_shebang
+        new_stream.write(build_shebang(interpreter, False, flags) + b'\n')
+        # copy the rest of the stream
+        stream.seek(0)


Is this really necessary? Logically saying, we've just read #!python without the newline, so readline() without seeking should yield the same result.

Indeed, I think you're correct. It's a small saving but correct to avoid. I suppose the installer sources copied it from the else branch (where it is necessary to rewind to the beginning) but I'm still not sure why. :D

mgorny · 2025-07-24T19:11:33Z

+        while True:
+            buf = stream.read(_COPY_BUFSIZE)
+            if not buf:
+                break
+            new_stream.write(buf)


This looks like reinventing shutil.copyfileobj().

Yes... I quite agree. It appears to be blindly copied from copyfileobj_with_hashing, another function from the installer codebase which at least had a good reason to exist.

eli-schwartz · 2025-07-25T05:23:04Z

Still, it needs to be rebased

Yup, I see that since 4d9f6e9 the code I modified has made other changes that lead to merge conflicts. (Same basic idea though -- applying our own scheme handler.)

After the weekend I will see about finding time to sort that out, and update the PR to base itself off of that.

I would recommend to mention in the script docs, and the special argument passed, that they are temporary until upstream merges, so non-Gentoo projects that use gpep517 know this might break in the future.

@arthurzam,

I'm not sure what you mean by that. We don't -- never have and probably never will -- import upstream's __main__, all we do is use the library APIs to build out our own CLI. We already add other optional features that upstream isn't interested in for their CLI, and we also rename the option for enabling optimized bytecode (which upstream insists on using two separate arguments for).

I don't see any reason to mark a gpep517 CLI argument as temporary. Upstream changes will only affect our ability to reuse new library APIs instead of designing them ourselves.

eli-schwartz force-pushed the script-flags branch from 2d14514 to 8c7405b Compare November 21, 2023 04:11

eli-schwartz mentioned this pull request Nov 21, 2023

Migrate several classes over to dataclasses pypa/installer#200

Merged

thesamesam requested a review from mgorny November 21, 2023 06:33

mgorny requested changes Dec 2, 2023

View reviewed changes

eli-schwartz force-pushed the script-flags branch 2 times, most recently from 3e79825 to 2b65308 Compare December 13, 2023 01:28

eli-schwartz force-pushed the script-flags branch from 2b65308 to 07241af Compare December 13, 2023 01:30

thesamesam requested a review from mgorny December 26, 2023 20:21

Mustafaalqaysy approved these changes Apr 16, 2024

View reviewed changes

mgorny closed this Apr 29, 2025

arthurzam reopened this Apr 29, 2025

mgorny requested changes Jul 24, 2025

View reviewed changes

Conversation

eli-schwartz commented Nov 21, 2023

Uh oh!

mgorny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eli-schwartz commented Dec 13, 2023

Uh oh!

eli-schwartz commented May 3, 2024

Uh oh!

arthurzam commented Apr 29, 2025

Uh oh!

mgorny commented Apr 29, 2025

Uh oh!

thesamesam commented Apr 29, 2025

Uh oh!

mgorny commented Apr 29, 2025

Uh oh!

thesamesam commented Jul 23, 2025

Uh oh!

arthurzam commented Jul 23, 2025

Uh oh!

mgorny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-schwartz commented Jul 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants