feat(pypi): pip.defaults API for customizing repo selection 2/n #2988
Conversation
Summary: - Allow switching to the Starlark implementation of the marker evaluation function. - Add a way for users to modify the `env` for the marker evaluation when parsing the requirements. This can only be done by `rules_python` or the root module. - Limit the platform selection when parsing the requirements files. Work towards bazel-contrib#2747 Work towards bazel-contrib#2949 Split out from bazel-contrib#2909
|
Haven't looked at code, just saw the PR description. Passing target settings will be better overall. They're more flexible and easier to customize for users. I suppose if they can pass a flag setting (the name at least), then it's just as flexible. But if a custom name can be passed, then a custom target_setting should also be passable. |
|
Will do this later, because of how much code I would have to change in one go. Hopefully this can be done in 3/n or 4/n. My plan is to roughly spike out the implementation with almost passing tests, little docs and then tidy up each and every one by one. EDIT: done in #2990 |
aignas
force-pushed
the
feat/pypi/default-2
branch
from
9a13cec to
f61b111
Compare
aignas
force-pushed
the
feat/pypi/default-2
branch
from
f61b111 to
4c52622
Compare
Stacked on bazel-contrib#2988 With this PR we can support arbitrary target settings instead of just plain `constraint_values`. We still have custom logic to ensure that all of the tests pass. However, the plan is to remove those tests once we have simplified the wheel selection mechanisms and the `pkg_aliases` macro. I.e. if we have at most 1 wheel per platform that the `pypi` bzlmod extension passes to the `pkg_aliases` macro, then we can just have a simple `selects.with_or` where we list out all of the target platform values. This PR may result in us creating more targets but that is the price that we have to pay if we want to do this incrementally. Work towards bazel-contrib#2747 Work towards bazel-contrib#2548 Work towards bazel-contrib#260
Stacked on bazel-contrib#2988 With this PR we can support arbitrary target settings instead of just plain `constraint_values`. We still have custom logic to ensure that all of the tests pass. However, the plan is to remove those tests once we have simplified the wheel selection mechanisms and the `pkg_aliases` macro. I.e. if we have at most 1 wheel per platform that the `pypi` bzlmod extension passes to the `pkg_aliases` macro, then we can just have a simple `selects.with_or` where we list out all of the target platform values. This PR may result in us creating more targets but that is the price that we have to pay if we want to do this incrementally. Work towards bazel-contrib#2747 Work towards bazel-contrib#2548 Work towards bazel-contrib#260
| ::: | ||
| """, | ||
| ), | ||
| "constraint_values": attr.label_list( |
There was a problem hiding this comment.
I think this should be a string_list, not label list?
If it's a label, it'll trigger repo-evaluation, even if it goes unused.
There was a problem hiding this comment.
This will be changed to target_settings in the next thing up the stack.
However, when we will have target_settings I would have expected that we would need the attr.label_list because we want to ensure that the users don't need to use canonical representation of the labels. The fetch would not matter that much, because this affects only the hub repo and the extension itself, both of which are reasonably cheap.
What do you mean about repo-evaluation? I thought that we can use @platforms//os:foo and does not cause any fetching?
There was a problem hiding this comment.
I thought it does cause repo fetching? Though, when I think back, maybe I'm confusing label_list() in repo/bzlmod with rctx.path(<some label>).
In any case, lets double check to verify.
Co-authored-by: Richard Levasseur <[email protected]>
| ::: | ||
| """, | ||
| ), | ||
| "constraint_values": attr.label_list( |
There was a problem hiding this comment.
I thought it does cause repo fetching? Though, when I think back, maybe I'm confusing label_list() in repo/bzlmod with rctx.path(<some label>).
In any case, lets double check to verify.
| @@ -788,6 +810,12 @@ The CPU architecture name to be used. | |||
| :::{note} | |||
| Either this or {attr}`env` `platform_machine` key should be specified. | |||
| ::: | |||
| """, | |||
| ), | |||
| "constraint_values": attr.label_list( | |||
There was a problem hiding this comment.
Followup thought for subsequent PR: replace constraint_values and target_settings with simply "config_settings" ?
The custom platform stuff in the toolchains has target_compatible_with (constraints) and target_settings because that's what the toolchain() rule accepts. However, for the pip stuff, it just needs something valid for a select() key.
There was a problem hiding this comment.
Correct, I like this suggestion. config_settings does sound more natural.
Co-authored-by: Richard Levasseur <[email protected]>
WIP: stacked on #2987
This is adding
constraint_valuesattribute topip.configureand isthreading it all the way down to the generation of
BUILD.bazelfile of forconfig settings used in the hub repository.
Out of scope:
flag_valuesor target settings. I am torn about it - doing it inthis PR would flesh out the design more, but at the same time it might become
harder to review.
whl_target_platformsandselect_whlsis still unchanged, not sure if itis related to this attribute addition.
Work towards #2747
Work towards #2548
Work towards #260