feat(pypi): support env markers in requirements files #2059

aignas · 2024-07-12T08:22:41Z

Before this change the all_requirements and related constants will have
packages that need to be installed only on specific platforms and will mean
that users relying on those constants (e.g. gazelle) will need to do extra
work to exclude platform-specific packages. The package managers that that
support outputting such files now include uv and pdm. This might be also
useful in cases where we attempt to handle non-requirements lock files.

Note, that the best way to handle this would be to move all of the requirements
parsing code to Python, but that may cause regressions as it is a much bigger
change. This is only changing the code so that we are doing extra processing
for the requirement lines that have env markers. The lines that have no markers
will not see any change in the code execution paths and the python interpreter
will not be downloaded.

We also use the *_ctx.watch API where available to correctly re-evaluate the
markers if the packaging Python sources for this change.

Extra changes that are included in this PR:

Extend the repo_utils to have a method for arch getting from the ctx.
Change the local_runtime_repo to perform the validation not relying on the
implementation detail of the get_platforms_os_name.
Add $(UV) make variable for the uv:current_toolchain so that we can
generate the requirements for sphinx using uv.
Swap the requirement generation using genrule and uv for sphinx and co
so that we can test the requirement marker code. Note, the requirement
markers are not working well with the requirement_cycles.

Fixes #1105.
Fixes #1868.
Work towards #260, #1975.
Related #1663.

aignas · 2024-07-16T06:32:32Z

Since #2000 is introducing useful helpers, I am gonna wait until that is merged and this PR is getting a little big, so I'm gonna need to split it into smaller portions.

This is just a small PR to reduce the scope of bazelbuild#2059. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.

…around This is extra preparation needed for bazelbuild#2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.

aignas · 2024-07-16T07:37:36Z

currently stacked on #2069

…around This is extra preparation needed for bazelbuild#2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.

…around (#2069) This is extra preparation needed for #2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards #260, #1105, #1868.

…2068) This is just a small PR to reduce the scope of #2059. This just moves some code from one python file to a separate one. Work towards #260, #1105, #1868.

Move more things to repo_utils and stop using watch_tree, because it also starts watching __pycache__ files, that is causing excesive restarts. What is more, we previously where using uname to get the OS, but it does not make sense to do that, we can use rctx.os.name instead and common utilities. What is more, be more explicit about what OSes are supported in the local_runtime_repo.

…s ready for final review a better test should be added

python/private/pypi/evaluate_markers.bzl

… added to the bzlmod lockfile to re-evaluate the files if the sources change

aignas

Self review.

MODULE.bazel

docs/sphinx/BUILD.bazel

python/private/pypi/evaluate_markers.bzl

python/private/pypi/extension.bzl

python/private/pypi/pip_repository.bzl

python/private/pypi/requirements_parser/resolve_target_platforms.py

aignas · 2024-08-06T11:34:16Z

python/private/pypi/requirements_parser/resolve_target_platforms.py

+    with args.input_path.open() as f:
+        reqs = json.load(f)
+
+    response = {}
+    for requirement_line, target_platforms in reqs.items():
+        entry, prefix, hashes = requirement_line.partition("--hash")
+        hashes = prefix + hashes
+
+        req = Requirement(entry)
+        for p in target_platforms:
+            (platform,) = Platform.from_string(p)
+            if not req.marker or req.marker.evaluate(platform.env_markers("")):
+                response.setdefault(requirement_line, []).append(p)
+
+    with args.output_path.open("w") as f:
+        json.dump(response, f)


Currently there are no unit tests, but if we agree on the approach I could add a few.

I think it's fine. Whether we run a python program, uv, or Starlark, how we evaluate the markers is an internal detail, right?

python/private/repo_utils.bzl

tests/pypi/evaluate_markers/BUILD.bazel

groodt

LGTM

…o feat/support-requirement-markers

rickeylev · 2024-08-15T19:04:42Z

I pushed a couple doc fixes, otherwise LGTM

aignas requested review from groodt and rickeylev as code owners July 12, 2024 08:22

aignas marked this pull request as draft July 12, 2024 13:36

aignas mentioned this pull request Jul 16, 2024

internal: repos to create a toolchain from a locally installed Python #2000

Merged

aignas added a commit to aignas/rules_python that referenced this pull request Jul 16, 2024

refactor(pypi): split out code for env marker evaluation for reuse

87fd3f5

This is just a small PR to reduce the scope of bazelbuild#2059. Work towards bazelbuild#260, bazelbuild#1105, bazelbuild#1868.

aignas force-pushed the feat/support-requirement-markers branch from d93f1ae to 69a296a Compare July 16, 2024 07:33

This was referenced Jul 16, 2024

refactor(pypi): split out code for env marker evaluation for reuse #2068

Merged

refactor(pypi): split out more utils and start passing 'abi_os_arch' around #2069

Merged

aignas added 5 commits July 17, 2024 15:36

feat(pypi): support env markers in python requirements files

fbc6a88

chore: use watch_tree and watch from repo_utils

ffcde83

chore: use uv to create a universal lock file for sphinx

969a43b

reset the .bazelrc

3e691b9

clarify logs

2106402

aignas force-pushed the feat/support-requirement-markers branch from 69a296a to 2106402 Compare July 17, 2024 07:22

aignas added 5 commits July 17, 2024 16:35

use dev_dependency for dev_dependency toolchains

fe69b9d

mark the requirements updating incompatible

d102ea1

disable sandbox for the requirements compilation

3fa1379

revert examples/bzlmod changes

eb75a85

aignas marked this pull request as ready for review July 17, 2024 13:55

aignas added 4 commits July 17, 2024 22:55

use a simpler way to define sphinx requirements

8e8e400

add marker eval to workspace

afb25f9

fix a typo

751006c

tidy up the requirement generation

3749199

aignas added 12 commits August 2, 2024 12:13

comment: docstring

d53e04a

add a comment about the runtime cost

3d2f314

fixup tests

2522ccd

default logger to None

56c1fdc

update changelog to clarify wording

04447ee

remove shebang

b25e62d

pass pythonpath entries as a default attribute to the rule or tag_class

99e401b

just hardcode the sources for the marker evaluation for now

282592b

just hardcode the sources for the marker evaluation for now

3521c4d

add a build_test for marker evaluation as a stop gap, before the PR i…

ec44390

…s ready for final review a better test should be added

fixup tests

44c46d2

bump the sphinx deps

26d9d77

aignas commented Aug 2, 2024

View reviewed changes

python/private/pypi/evaluate_markers.bzl Outdated Show resolved Hide resolved

aignas added 2 commits August 6, 2024 14:11

create a utility method to run python scripts and ensure the srcs are…

51424cf

… added to the bzlmod lockfile to re-evaluate the files if the sources change

distil the interface

8f14806

aignas commented Aug 6, 2024

View reviewed changes

Apply suggestions from self code review

20c643c

aignas marked this pull request as ready for review August 6, 2024 11:37

buildifier

824491b

aignas requested a review from rickeylev August 7, 2024 11:53

groodt approved these changes Aug 9, 2024

View reviewed changes

rickeylev added 3 commits August 15, 2024 11:38

clarify changelog doc

c454a7d

typo: bazel -> Bazel

b424d36

Merge branch 'main' of https://github.com/bazelbuild/rules_python int…

3515466

…o feat/support-requirement-markers

rickeylev approved these changes Aug 15, 2024

View reviewed changes

rickeylev enabled auto-merge August 15, 2024 19:04

rickeylev added this pull request to the merge queue Aug 15, 2024

Merged via the queue into bazelbuild:main with commit 519574c Aug 15, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pypi): support env markers in requirements files #2059

feat(pypi): support env markers in requirements files #2059

feat(pypi): support env markers in requirements files #2059

feat(pypi): support env markers in requirements files #2059

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment