-
Notifications
You must be signed in to change notification settings - Fork 418
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
Run a command from a [pipx.run] entry point #615
Conversation
TODO: Write documentation on this. |
src/pipx/venv.py
Outdated
@@ -34,6 +40,16 @@ | |||
|
|||
logger = logging.getLogger(__name__) | |||
|
|||
# This regex was only introduced into importlib.metadata in Python 3.9. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use sys.version_info < (3, 9)
instead of the try/except above, then require importlib_metadata for < 3.9 instead of < 3.8. Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the regex pattern is well-specified and stable and requring importlib-metadata
on Python 3.8 is not worth it.
Can you give an example of what a |
This is the configuration I’m using to test this (against pypa/build): |
OK, thanks. So the idea is that one would need to make sure that the packages PyPI Package Name is the name of the pipx.run endpoint? Like:
Is there a way to eliminate |
Unfortunately, each entry point must have a group and a name, so it’s not possible to write [options.entry_points]
pipx.run = <import-path> The closest possible is something like (using [options.entry_points]
pipx =
run = <import-path> or (using an empty string as key) [options.entry_points]
pipx.run =
= <import-path> |
That's how entry points work. What you would like to do is write: [options.entry_points]
console_scripts =
build = build.__main__:entrypoint But that would be too common of a name and would break things. So now you can write: [options.entry_points]
console_scripts =
pyproject-build = build.__main__:entrypoint
pipx.run =
build = build.__main__:entrypoint Now only "pyproject-build" shows up in the console, but |
Using run or an empty string (or anything not unique to the package) would likely be a disaster. If you have a dependency that also had a pipx run entry point, the two would be merged into an unordered list (I believe - that's how Numba works). You want the similarity to |
You are also not limited to matching the package name; you could add multiple commands. For example, if a project did not want to provide console_scripts (say it only wanted to provide a |
importlib.metadata has the information which entry point comes from which package, so it's possible to choose the correct entry point when there are multiple matches. The implementation would be a bit more complicated than the current PR, but it's doable. |
I didn't know that; good to know! I still think that the current implementation is correct, since you are literally just proving a |
Yeah, I think the current approach is more balanced, simple to implement while also easy enough for the user to use. The user experience is not perfect, but improvements would also bring drawbacks that may prove problematic. |
That sounds right, I was just wondering if there was an easy way to eliminate what could be a possible gotcha for package creators. Naming it properly should probably be pointed out in the docs. |
So, just to double-check on how the interface is working... If
It will download and install I feel like we should canonicalize the package name and/or endpoint name before matching them because of this, to avoid trouble with any random capitalization or punctuation for example. |
I believe the same is true with console scripts as well though? I agree canonicalisation is the right thing to do, but it is arguably a separate enhancement. |
The difference to my mind is that console scripts are between the packager and the user only, but here pipx is part of it. And the other parts of pipx behave in a way that different capitalizations, punctuations don't matter if they are equivalent canonically. EDIT: Oh I see, you mean console scripts in conjunction with |
I filed #618 for the name normalisation issue. |
Does this mean you're working on documentation for this PR? |
I’m not actively writing the documentation for this. |
src/pipx/venv.py
Outdated
if not self.python_path.exists(): | ||
return None | ||
site_packages = get_site_packages(self.python_path) | ||
for dist in Distribution.discover(path=[str(site_packages)]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about limiting our check for the pipx.run
entrypoint to only the main package?
Otherwise if we found a dependency with a pipx.run
entrypoint having the app name, we would match it instead. This sounds like a dumb thing to happen, but I've seen dumb things in the past (like a package using two dependencies that both use the same name for console scripts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added. (Reverted, see below.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this does not really work well after I think about this more, because pipx run
does not really know what the “main package” is in a cached venv. It’s wrong to use the app
argument, that would make usages like pipx run tox4 --spec "tox>=4a0"
fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have that information in the metadata? e.g. self.main_package_name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could even use pipx.venv_inspect.get_dist
using self.main_package_name
as the first argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn’t know that! Let me try it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to work. PR updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is importlib.metadata.Distribution.discover()
documented anywhere? I'm only finding it by browsing the importlib.metadata
source. It would probably be useful to use in pipx.venv_inspect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just out of curiosity, is there a way you would get multiple dists
from from Distribution.discover()
if you searched for the name of the package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think Distribution.discover()
is documented. The author told me about it when I was investigating the possibility of using importlib.metadata in pip (to replace pkg_resources
).
Is there a way you would get multiple dists from from Distribution.discover() if you searched for the name of the package?
Yes, because it is possible to have multiple installations of a package in different sys.path
entries. For example:
pip install build
pip install --user build
would give you two build
distributions, one under the system site-packages, and the other in the per-user site-packages. Another possible case is enabling system site-packages in a virtual environment. Neither applies for pipx run
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Of course still need to fix the conflict but otherwise, nice!
Adding docs in "developing for pipx" would be nice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good
Supercedes #602.
docs/changelog.md
Summary of changes
Test plan
Tested by running