Support installing requirements from inline script metadata (PEP 723) #13052
Conversation
|
Hi @SnoopJ thanks for submitting this PR. Let us know if you are willing to continue working on it, I will be willing to review this (although be aware it may be a couple of months, or more, before I can spend time doing a full review). If so the first thing you need to do is merge, or rebase, with main, resolve any conflicts, and add this additional option to the lock command, which was probably added after you raised this PR. Once you've done that I can advise on any pre-commit failures. Finally, I don't want to get into a long bike-shedding discussion, but I don't love "--script" as an option name, to me it could be confused with running a script, perhaps "--script-dependencies" or "--script-inline"? Something that makes it clear that just the inline dependencies are being taken from the script. |
Yep, I am willing to resume work on this. Thanks for having a look, it sounds like I'm on the right-enough track that I can freshen this PR and fix up whatever was failing CI when this fell fallow.
Strictly speaking, this implementation isn't "just" the inline dependencies, since it also respects
|
|
I agree with @notatallshaw that |
Yep! |
This also lays the groundwork for potentially allowing the argument to be given multiple times, but I don't yet want to think about the additional complexity of multiple possibly-contradictory `requires-python` specifiers.
SnoopJ
force-pushed
the
feature/gh12891-inline-metadata
branch
from
cab2786 to
aa6c8a2
Compare
| ) | ||
| result = script.pip("install", "--requirements-from-script", script_path) | ||
|
|
||
| # NOTE:2024-10-05:snoopj:assertions same as in test_requirements_file |
There was a problem hiding this comment.
Not meant to retain this comment but I figured I'd call it out for review that this test is more or less a duplicate of its cousin in terms of side effects, in case that's not appropriate.
| # NOTE:2024-10-05:snoopj:assertions same as in test_requirements_file |
| if options.requirements_from_scripts: | ||
| if len(options.requirements_from_scripts) > 1: | ||
| raise CommandError("--requirements-from-script can only be given once") | ||
|
|
||
| script = options.requirements_from_scripts[0] | ||
| script_metadata = pep723_metadata(script) | ||
|
|
||
| script_requires_python = script_metadata.get("requires-python", "") | ||
|
|
||
| if script_requires_python and not options.ignore_requires_python: | ||
| target_python = make_target_python(options) | ||
|
|
||
| if not check_requires_python( | ||
| requires_python=script_requires_python, | ||
| version_info=target_python.py_version_info, | ||
| ): | ||
| raise UnsupportedPythonVersion( | ||
| f"Script {script!r} requires a different Python: " | ||
| f"{target_python.py_version} not in {script_requires_python!r}" | ||
| ) | ||
|
|
||
| for req in script_metadata.get("dependencies", []): | ||
| requirements.append( # noqa: PERF401 | ||
| InstallRequirement(Requirement(req), comes_from=None) | ||
| ) |
There was a problem hiding this comment.
When first submitting this PR, I originally commented on this code:
It feels ungraceful to put all of this logic here but I'm not really sure how I would plumb it into a separate function. Suggestions welcome
I'm not sure I feel as strongly about it now, though. Worth highlighting, though.
|
@notatallshaw this should now be ready for review. There's one comment that I'm sure needs to be removed and I'm happy to squash it all down if that is desired, but I preserved the history when rebasing on |
Note that a trailing .* is necessary to make the resulting SpecifierSet broad enough to match any minor version. Co-authored-by: Paul Moore <[email protected]>
Co-authored-by: Paul Moore <[email protected]>
That's what I get for trying to take the web UI seriously.
4 participants
This changeset adds support for installing requirements declared in a script using inline metadata as defined by PEP 723
I'm sure this still needs changes, but I'm at the point where I'm ready for a maintainer to point out to me how much I've missed 😅
Closes #12891