-
-
Notifications
You must be signed in to change notification settings - Fork 661
feat(pypi): pip.defaults API for customizing repo selection 2/n #2988
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
Changes from all commits
cd4dd15
4c52622
d5a0645
7ef85aa
36cd97c
c459371
3ecc713
272f884
b10afe5
3bfe839
58c7190
b723006
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -372,7 +372,7 @@ def _whl_repo(*, src, whl_library_args, is_multiple_versions, download_only, net | |
| ), | ||
| ) | ||
|
|
||
| def _configure(config, *, platform, os_name, arch_name, override = False, env = {}): | ||
| def _configure(config, *, platform, os_name, arch_name, constraint_values, env = {}, override = False): | ||
| """Set the value in the config if the value is provided""" | ||
| config.setdefault("platforms", {}) | ||
| if platform: | ||
|
|
@@ -387,6 +387,7 @@ def _configure(config, *, platform, os_name, arch_name, override = False, env = | |
| name = platform.replace("-", "_").lower(), | ||
| os_name = os_name, | ||
| arch_name = arch_name, | ||
| constraint_values = constraint_values, | ||
| env = env, | ||
| ) | ||
| else: | ||
|
|
@@ -413,6 +414,10 @@ def _create_config(defaults): | |
| arch_name = cpu, | ||
| os_name = "linux", | ||
| platform = "linux_{}".format(cpu), | ||
| constraint_values = [ | ||
| "@platforms//os:linux", | ||
| "@platforms//cpu:{}".format(cpu), | ||
| ], | ||
| env = {"platform_version": "0"}, | ||
| ) | ||
| for cpu in [ | ||
|
|
@@ -424,17 +429,25 @@ def _create_config(defaults): | |
| arch_name = cpu, | ||
| # We choose the oldest non-EOL version at the time when we release `rules_python`. | ||
| # See https://endoflife.date/macos | ||
| env = {"platform_version": "14.0"}, | ||
| os_name = "osx", | ||
| platform = "osx_{}".format(cpu), | ||
| constraint_values = [ | ||
| "@platforms//os:osx", | ||
| "@platforms//cpu:{}".format(cpu), | ||
| ], | ||
| env = {"platform_version": "14.0"}, | ||
| ) | ||
|
|
||
| _configure( | ||
| defaults, | ||
| arch_name = "x86_64", | ||
| env = {"platform_version": "0"}, | ||
| os_name = "windows", | ||
| platform = "windows_x86_64", | ||
| constraint_values = [ | ||
| "@platforms//os:windows", | ||
| "@platforms//cpu:x86_64", | ||
| ], | ||
| env = {"platform_version": "0"}, | ||
| ) | ||
| return struct(**defaults) | ||
|
|
||
|
|
@@ -500,6 +513,7 @@ You cannot use both the additive_build_content and additive_build_content_file a | |
| _configure( | ||
| defaults, | ||
| arch_name = tag.arch_name, | ||
| constraint_values = tag.constraint_values, | ||
| env = tag.env, | ||
| os_name = tag.os_name, | ||
| platform = tag.platform, | ||
|
|
@@ -679,6 +693,13 @@ You cannot use both the additive_build_content and additive_build_content_file a | |
| } | ||
| for hub_name, extra_whl_aliases in extra_aliases.items() | ||
| }, | ||
| platform_constraint_values = { | ||
| hub_name: { | ||
| platform_name: sorted([str(Label(cv)) for cv in p.constraint_values]) | ||
| for platform_name, p in config.platforms.items() | ||
| } | ||
| for hub_name in hub_whl_map | ||
| }, | ||
| whl_libraries = { | ||
| k: dict(sorted(args.items())) | ||
| for k, args in sorted(whl_libraries.items()) | ||
|
|
@@ -769,6 +790,7 @@ def _pip_impl(module_ctx): | |
| for key, values in whl_map.items() | ||
| }, | ||
| packages = mods.exposed_packages.get(hub_name, []), | ||
| platform_constraint_values = mods.platform_constraint_values.get(hub_name, {}), | ||
| groups = mods.hub_group_map.get(hub_name), | ||
| ) | ||
|
|
||
|
|
@@ -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( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, I like this suggestion. |
||
| mandatory = True, | ||
| doc = """\ | ||
| The constraint_values to use in select statements. | ||
| """, | ||
| ), | ||
| "os_name": attr.string( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be changed to
target_settingsin the next thing up the stack.However, when we will have
target_settingsI would have expected that we would need theattr.label_listbecause 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:fooand does not cause any fetching?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.