Parse pyproject.toml statically where possible #1964

hauntsaninja · 2023-08-10T21:26:46Z

Contributor checklist

Included tests for the changes.
PR title is short, clear, and ready to be included in the user-facing changelog.

Maintainer checklist

Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

webknjaz · 2023-08-11T01:41:38Z

Plz use pathlib everywhere instead of low-level os calls.

hauntsaninja · 2023-08-11T01:49:39Z

Sure, was just matching style to preexisting code

hauntsaninja · 2023-08-18T21:26:10Z

Just to be explicit: atugushev suggested I don't work on this PR until #1681 is merged. This makes sense to me, since the two conflict.

webknjaz · 2023-12-03T03:41:08Z

Hey @hauntsaninja, that PR was merged about a month ago. So it seems like this one can be considered unblocked.

for more information, see https://pre-commit.ci

hauntsaninja · 2023-12-03T07:02:20Z

Thanks for the nudge, I've rebased the changes on top of main

webknjaz · 2023-12-03T07:16:52Z

Ah, I was just cleaning up the old notification emails and noticed this..

Since this looks up the [build-system] section, I think @apljungquist should be invited to review too.

webknjaz

Note that such changes require tests. Could you add some?

piptools/build.py

webknjaz · 2023-12-03T08:58:55Z

piptools/build.py

@@ -41,6 +48,67 @@ class ProjectMetadata:
    build_requirements: tuple[InstallRequirement, ...]


+def maybe_statically_parse_project_metadata(
+    src_file: pathlib.Path,
+) -> ProjectMetadata | None:


I think it's better not to use None as a magic sentinel value that the caller shouldn't forget to check for.
Instead, how about raising a LookupError. The handling would be cleaner, then.

Given that we're using a static type checker, I think that would actually be less safe. mypy will complain if you do not handle None, but it will not complain if you do not handle the exception

webknjaz · 2023-12-03T09:01:15Z

piptools/build.py

+    comes_from = f"{package_name} ({src_file}::build-system.requires)"
+    build_requirements = [
+        InstallRequirement(Requirement(req), comes_from)
+        for req in pyproject_contents.get("build-system", {}).get("requires", [])


This doesn't return complete build deps. It should call the corresponding hooks or not inject the build deps at all. An incomplete list would be harmful, expectations-wise.

webknjaz · 2023-12-03T09:04:44Z

Could you also include more information regarding the motivation, use case, possible side effects, usage examples etc. into the PR description? Make sure all of those are also present as tests.

webknjaz · 2023-12-03T09:06:37Z

Oh, and it looks like the contributor checklist might've been overlooked, could you verify?

for more information, see https://pre-commit.ci

hauntsaninja · 2024-01-07T10:09:24Z

Thanks for the initial review! I pushed a bunch of changes; this should now be much closer to shippable

hauntsaninja · 2024-01-07T10:12:32Z

piptools/build.py

    :param isolated: Whether to run invoke the backend in the current
                     environment or to create an isolated one and invoke it
                     there.
    :param quiet: Whether to suppress the output of subprocesses.
    """

+    if attempt_static_parse:
+        if build_targets:


Let me know if you prefer to do this implicitly based on the value of build_targets, instead of having a separate param. I did it this way because it feels a little easier to test / harder to mess up

I like how you did it.

piptools/build.py

tests/test_build.py

piptools/build.py

Co-authored-by: chrysle <fritzihab@posteo.de>

chrysle

LGTM!

chrysle · 2024-03-09T08:06:34Z

@webknjaz Could you review again? This is a larger change, so I wouldn't like to merge this on my own.

chrysle

I have got two minor suggestions, and a larger design question. If @webknjaz does not respond in the near future, I think I'll merge.

piptools/build.py

chrysle · 2024-03-24T19:00:08Z

piptools/build.py

+        for req in project_table.get("dependencies", [])
+    ]
+    for extra, reqs in (
+        pyproject_contents.get("project", {}).get("optional-dependencies", {}).items()


Should we support Poetry for static metadata parsing?

If someone is interested in that, it can be a follow-up PR. It's been tough enough getting this one in, so I want to just stick with the standards based thing in this one

piptools/build.py

Co-authored-by: chrysle <fritzihab@posteo.de>

hauntsaninja · 2024-03-24T19:53:46Z

Thank you for the reviews!

chrysle · 2024-03-25T19:35:35Z

Thanks, I was about to suggest the same! Could you rebase properly?

hauntsaninja · 2024-03-25T19:44:25Z

fixed the test and ungoofed the git!

chrysle · 2024-03-25T20:10:04Z

Great, thank you for the contribution!

charliermarsh · 2024-03-25T20:49:47Z

Just as a heads up, since we struggle with a similar thing in uv, I think this will error on Hatch projects that use Hatch's context formatting. Here's an example: https://github.com/astral-sh/uv/blob/ae35a215c67a161e759769b832b99740f4ecad69/scripts/packages/root_editable/pyproject.toml.

If I run from main, I get:

❯ pip-compile ../uv/scripts/packages/root_editable/pyproject.toml
Traceback (most recent call last):
  File "/Users/crmarsh/workspace/pip-tools/.venv/bin/pip-compile", line 8, in <module>
    sys.exit(cli())
             ^^^^^
  File "/Users/crmarsh/workspace/pip-tools/.venv/lib/python3.12/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/.venv/lib/python3.12/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/.venv/lib/python3.12/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/.venv/lib/python3.12/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/.venv/lib/python3.12/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/piptools/scripts/compile.py", line 365, in cli
    metadata = build_project_metadata(
               ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/piptools/build.py", line 155, in build_project_metadata
    project_metadata = maybe_statically_parse_project_metadata(src_file)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/piptools/build.py", line 92, in maybe_statically_parse_project_metadata
    InstallRequirement(Requirement(req), comes_from)
                       ^^^^^^^^^^^^^^^^
  File "/Users/crmarsh/workspace/pip-tools/.venv/lib/python3.12/site-packages/pip/_vendor/packaging/requirements.py", line 117, in __init__
    raise InvalidRequirement(f"Invalid URL: {req.url}")
pip._vendor.packaging.requirements.InvalidRequirement: Invalid URL: {root:uri}/../black_editable

hauntsaninja · 2024-03-25T20:56:48Z

Hmm that's a violation of standards. It should probably declare dependencies as dynamic, or not use PEP 621

charliermarsh · 2024-03-25T20:58:23Z

Yeah, there's a discussion on it here if you'd like to chime in: pypa/hatch#1331

statically parse pyproject

15de8f0

hauntsaninja force-pushed the static-pyproj branch from 1fcb6c9 to 15de8f0 Compare December 3, 2023 07:01

[pre-commit.ci] auto fixes from pre-commit.com hooks

7724b4b

for more information, see https://pre-commit.ci

.

5bd37aa

hauntsaninja marked this pull request as ready for review December 3, 2023 07:41

webknjaz reviewed Dec 3, 2023

View reviewed changes

webknjaz added enhancement Improvements to functionality pyproject.toml Related to pyproject.toml support labels Dec 3, 2023

chrysle added the awaiting response Awaiting response from a contributor label Jan 7, 2024

hauntsaninja and others added 6 commits January 7, 2024 01:03

Avoid static branch if build deps are required

25c3420

Fix self-recursive extras and add tests

81cff63

[pre-commit.ci] auto fixes from pre-commit.com hooks

155ffa9

for more information, see https://pre-commit.ci

pragma

4b27fd3

fix on windows

1d4c02e

another test

1d95efd

hauntsaninja force-pushed the static-pyproj branch from 14502f4 to 1d95efd Compare January 7, 2024 09:59

test

0dc0221

use as_uri

7232275

hauntsaninja commented Jan 7, 2024

View reviewed changes

coverage

5107511

chrysle removed the awaiting response Awaiting response from a contributor label Jan 19, 2024

chrysle reviewed Jan 19, 2024

View reviewed changes

piptools/build.py Show resolved Hide resolved

piptools/build.py Show resolved Hide resolved

piptools/build.py Outdated Show resolved Hide resolved

code review

6308202

chrysle reviewed Jan 20, 2024

View reviewed changes

tests/test_build.py Show resolved Hide resolved

piptools/build.py Outdated Show resolved Hide resolved

piptools/build.py Outdated Show resolved Hide resolved

piptools/build.py Outdated Show resolved Hide resolved

hauntsaninja and others added 3 commits January 28, 2024 21:56

Apply suggestions from code review

5bb3202

Co-authored-by: chrysle <fritzihab@posteo.de>

Update piptools/build.py

c99514e

Co-authored-by: chrysle <fritzihab@posteo.de>

split up test a little

472c13b

chrysle requested a review from webknjaz January 29, 2024 14:30

chrysle approved these changes Jan 29, 2024

View reviewed changes

hauntsaninja mentioned this pull request Mar 22, 2024

Consider reading pyproject.toml metadata directly astral-sh/uv#2629

Closed

chrysle reviewed Mar 24, 2024

View reviewed changes

Apply suggestions from code review

7ca2879

Co-authored-by: chrysle <fritzihab@posteo.de>

hauntsaninja added 2 commits March 25, 2024 12:42

Merge remote-tracking branch 'upstream/main' into static-pyproj

4e8f59f

constraint

2098b99

hauntsaninja force-pushed the static-pyproj branch from 1b85599 to 2098b99 Compare March 25, 2024 19:42

chrysle enabled auto-merge March 25, 2024 19:48

chrysle added this pull request to the merge queue Mar 25, 2024

Merged via the queue into jazzband:main with commit b692edb Mar 25, 2024
36 checks passed

hauntsaninja deleted the static-pyproj branch March 25, 2024 20:06

hauntsaninja mentioned this pull request Mar 25, 2024

Context formatting of project.dependencies is not standard compliant? pypa/hatch#1331

Open

chrysle added this to the 7.4.2 milestone Mar 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse pyproject.toml statically where possible #1964

Parse pyproject.toml statically where possible #1964

hauntsaninja commented Aug 10, 2023 •

edited

webknjaz commented Aug 11, 2023

hauntsaninja commented Aug 11, 2023

hauntsaninja commented Aug 18, 2023

webknjaz commented Dec 3, 2023

hauntsaninja commented Dec 3, 2023

webknjaz commented Dec 3, 2023

webknjaz left a comment

webknjaz Dec 3, 2023

hauntsaninja Jan 7, 2024

webknjaz Dec 3, 2023

hauntsaninja Jan 7, 2024

webknjaz commented Dec 3, 2023

webknjaz commented Dec 3, 2023

hauntsaninja commented Jan 7, 2024

hauntsaninja Jan 7, 2024 •

edited

chrysle Jan 19, 2024

chrysle left a comment

chrysle commented Mar 9, 2024

chrysle left a comment

chrysle Mar 24, 2024

hauntsaninja Mar 24, 2024 •

edited

hauntsaninja commented Mar 24, 2024

chrysle commented Mar 25, 2024

hauntsaninja commented Mar 25, 2024

chrysle commented Mar 25, 2024

charliermarsh commented Mar 25, 2024 •

edited

hauntsaninja commented Mar 25, 2024 •

edited

charliermarsh commented Mar 25, 2024

Parse pyproject.toml statically where possible #1964

Parse pyproject.toml statically where possible #1964

Conversation

hauntsaninja commented Aug 10, 2023 • edited

Contributor checklist

Maintainer checklist

webknjaz commented Aug 11, 2023

hauntsaninja commented Aug 11, 2023

hauntsaninja commented Aug 18, 2023

webknjaz commented Dec 3, 2023

hauntsaninja commented Dec 3, 2023

webknjaz commented Dec 3, 2023

webknjaz left a comment

Choose a reason for hiding this comment

webknjaz Dec 3, 2023

Choose a reason for hiding this comment

hauntsaninja Jan 7, 2024

Choose a reason for hiding this comment

webknjaz Dec 3, 2023

Choose a reason for hiding this comment

hauntsaninja Jan 7, 2024

Choose a reason for hiding this comment

webknjaz commented Dec 3, 2023

webknjaz commented Dec 3, 2023

hauntsaninja commented Jan 7, 2024

hauntsaninja Jan 7, 2024 • edited

Choose a reason for hiding this comment

chrysle Jan 19, 2024

Choose a reason for hiding this comment

chrysle left a comment

Choose a reason for hiding this comment

chrysle commented Mar 9, 2024

chrysle left a comment

Choose a reason for hiding this comment

chrysle Mar 24, 2024

Choose a reason for hiding this comment

hauntsaninja Mar 24, 2024 • edited

Choose a reason for hiding this comment

hauntsaninja commented Mar 24, 2024

chrysle commented Mar 25, 2024

hauntsaninja commented Mar 25, 2024

chrysle commented Mar 25, 2024

charliermarsh commented Mar 25, 2024 • edited

hauntsaninja commented Mar 25, 2024 • edited

charliermarsh commented Mar 25, 2024

hauntsaninja commented Aug 10, 2023 •

edited

hauntsaninja Jan 7, 2024 •

edited

hauntsaninja Mar 24, 2024 •

edited

charliermarsh commented Mar 25, 2024 •

edited

hauntsaninja commented Mar 25, 2024 •

edited