Skip to content
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

Allow running scripts with dependencies using pipx #916

Merged
merged 21 commits into from
Apr 13, 2023

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Nov 24, 2022

Fixes #913

TODO

  • I have added an entry to docs/changelog.md
  • Documentation updates
  • Additional tests for successfully running scripts with dependencies
  • Refactor our use of the Venv class to handle app-less venvs properly
  • Clean up the implementation

Summary of changes

This PR adds functionality to the pipx run <script> command to allow running scripts that have non-stdlib dependencies.

The script is parsed for a comment block that has the form

# Requirements:
# req1
# req2

The block is terminated by a blank comment line, or by a non-comment line. Requirements may be anything that can be a top-level requirement for pip.

Before running a script, the code checks for a requirements block. If one is present, it creates a temporary virtual environment (managed the same way as the existing pip run app handles its venvs), installs the requirements there, and runs the script in that environment.

Implementation issues

The existing implementation of the Venv class in pipx assumes every environment will have a "main app". The code to write the pipx metadata file enforces this. This is not the case for temporary environments supporting scripts.

Rather than changing the implementation of Venv to allow environments without apps (which would be an extensive change, as it changes a fundamental invariant of the existing class) I plan on splitting the Venv class into two subclasses - AppVenv and ScriptVenv. All existing uses would become AppVenv instances, with ScriptVenv being for the new functionality.

One subtlety here is that the code that lists venvs simply scans directories and assumes what it finds is a venv (VenvContainer.iter_venv_dirs). In practice that isn't an issue, as ScriptVenv environments will only be found in the cache directory, which is never scanned in this way (as far as I can tell) but it is a potential risk. I will decide how to handle this when I do the refactoring.

Test plan

The PR includes tests of the functionality.

Also, it can be manually tested by creating a file test.py containing

# Requirements:
#     requests

import sys
import subprocess
print(sys.executable)
subprocess.run([sys.executable, "-m", "pip", "list"])

Then run the command

pipx run file:test.py

Output should show the executable being in a temporary virtual environment, and the pip output will show requests being present (with its dependencies).

@pfmoore
Copy link
Member Author

pfmoore commented Nov 24, 2022

I have an alternative idea for handling the venv issue. Rather than try to deal with the complexities of having two different types of environment, I can punt on the problem, and simply note that if I call

venv = Venv(venv_dir, python=python, verbose=verbose)
venv.create_venv(venv_args, pip_args)

then I have a valid Venv object. It doesn't have a pipx_metadata.json file, but as long as I don't call methods that care about this, I'm fine. So rather than making a big deal about whether it's a "proper" Venv, let's just accept that there's no necessity to have a main package, it's just that certain methods require one.

If I then add a new install_unmanaged_requirements method to the Venv class, which installs a set of requirements without trying to add them to the metadata, everything works fine. The fact that the metadata file is missing doesn't matter to pipx run, and no other command will see those environments as they are in the .cache directory.

I'm not completely happy with this solution, but it's honestly no worse than someone creating a Venv and not calling install_package with is_main_packcakge=True in any other way. So I'm inclined to say that if we do want to be stricter on the lifecycle of Venv objects, that's something that should be handled in a follow-up PR, and I'd rather keep this PR focused on the functionality.

@pfmoore
Copy link
Member Author

pfmoore commented Nov 24, 2022

Hmm, I don't think the error on the macos CI is caused by this change:

ERROR    pipx.util:util.py:310 pip seemed to fail to build package:
    lxml

@pfmoore
Copy link
Member Author

pfmoore commented Nov 24, 2022

There's probably still work to do on this, but IMO it's ready for an initial round of reviews. If anyone knows how I can fix the MacOS test failure, I'd appreciate it.

By the way, the code creates the environment name based on the requirements, not based on the script name. That's deliberate, because it means that if you (for example) use requests in your scripts a lot, the same environment can get used for multiple scripts. I don't know if it would be worth documenting this somewhere - at a minimum, I could add a comment explaining this in the code, but I couldn't find any docs where this sort of detail seemed to fit.

@pfmoore pfmoore marked this pull request as ready for review November 24, 2022 15:00
docs/examples.md Show resolved Hide resolved
docs/examples.md Outdated Show resolved Hide resolved
@dukecat0
Copy link
Member

dukecat0 commented Dec 2, 2022

If anyone knows how I can fix the MacOS test failure, I'd appreciate it.

It's fixed in #917.

@pfmoore
Copy link
Member Author

pfmoore commented Dec 6, 2022

I believe this resolves all of the outstanding issues. I had to refactor the pipx run implementation a little. The resulting code passes all the tests. It is (IMO) a little clumsy, but that's mainly because I didn't want to risk changing the behaviour of existing forms of the command, so I tried to leave as much of that code intact as I could. It might be worth considering tidying the code up a bit in a follow-up PR.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 16, 2023

Gentle ping on this. Is there anything I can do to help get this merged and into a released version?

src/pipx/venv.py Outdated Show resolved Hide resolved
@cs01
Copy link
Member

cs01 commented Jan 18, 2023

Gentle ping on this. Is there anything I can do to help get this merged and into a released version?

Hi and apologies for the lack of feedback. I really appreciate your work here (and elsewhere in the Python world), but I simply don’t have the bandwidth in my personal life to devote to open source as much as I used to. I’ll do my best to take a look soon, but can’t make any promises.

@pfmoore
Copy link
Member Author

pfmoore commented Jan 18, 2023

No worries, thanks for the update.

Comment on lines +49 to +50
pipx run https://example.com/test.py
pipx run https://example.com/test.py 1 2 3
Copy link
Member

Choose a reason for hiding this comment

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

I’m not sure we want to mention this in documentation since running arbitrary code directly from the internet is very dangerous to attacks such as MITM. This is similar to e.g. what we used to recommend (curl .../get-pip.py | python) but now split the command into two. It is OK for this to be supported (use it at your own risk if you managed to find it in --help) but I would like to see this removed from documentation. Or at least we should move it to a separate section and mention very prominently this is dangerous.

Also, while I’m writing this, I wonder if pipx run - [args] is a good thing to support.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can already run arbitrary code, see here. All this feature adds is the ability to define requirements.

Also, while I’m writing this, I wonder if pipx run - [args] is a good thing to support.

I don't have a strong opinion on this, but I do think it can be handled in a follow-up PR if you think it's worth doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@uranusjr is there anything you want me to do to address this? I could just remove the two examples with URLs, but as I said this isn't a new feature, and it's already documented in the last line of the block of examples jutst above what I added.

docs/examples.md Outdated Show resolved Hide resolved
@uranusjr uranusjr merged commit 1931132 into pypa:main Apr 13, 2023
dukecat0 added a commit to dukecat0/pipx that referenced this pull request Apr 14, 2023
dukecat0 added a commit to dukecat0/pipx that referenced this pull request Apr 14, 2023
dukecat0 added a commit that referenced this pull request Apr 14, 2023
@pfmoore pfmoore deleted the run_with_reqs branch July 18, 2023 10:18
@Flimm
Copy link

Flimm commented Nov 23, 2023

People interested in this feature and pull request may be interested to learn about PEP 723 Inline script metadata, which is currently provisionally accepted. It specifies the syntax for a special comment that specifies dependencies. Here's an example:

# /// pyproject
# [run]
# requires-python = ">=3.11"
# dependencies = [
#   "requests<3",
#   "rich",
# ]
# ///

import requests
from rich.pretty import pprint

resp = requests.get("https://peps.python.org/api/peps.json")
data = resp.json()
pprint([(k, v["title"]) for k, v in data.items()][:10])

There are still collecting feedback from the community. See this forum thread, and this one.

To be clear, no version of pipx currently implements this, or any other tool that I am aware of.

@martin-braun
Copy link

@Flimm pipx actually seems to already support Inline script metadata PEP, which is awesome. The following script launches successfully:

#!/usr/bin/env pipx run

# /// script
# requires-python = ">=3.11"
# dependencies = [
#   "requests<3",
#   "rich",
# ]
# ///

import requests
from rich.pretty import pprint

resp = requests.get("https://peps.python.org/api/peps.json")
data = resp.json()
pprint([(k, v["title"]) for k, v in data.items()][:10])

# vi: ft=python

@Flimm
Copy link

Flimm commented Sep 6, 2024

Wow, that's wonderful! I'm glad to see this PEP got approved and that pipx supports it 😁

By the way, on Linux, the shebang line you mentioned doesn't work, because Linux doesn't support shebang lines with spaces the way that one would expect. To work around this, you need to pass -S to env, like this:

#!/usr/bin/env -S pipx run

@martin-braun
Copy link

@Flimm Thanks, MacOS' env was playing nice with me. Afaik the PEP is not necessarily approved, or is it? It seems it was implemented into pipx regardless, but the API is still not final, maybe.

@pfmoore
Copy link
Member Author

pfmoore commented Sep 8, 2024

The PEP is final. It was accepted here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow running scripts with dependencies using pipx
6 participants