chore: remove mandatory default values for python version flag #2217
Conversation
There was a problem hiding this comment.
I had an inkling of an idea that might make the config_setting generation we do much easier: use FeatureFlagInfo.
Basically, have a separate flag read the full-version flag. It returns the major.minor part. Because it returns the value using FeatureFlagInfo, it can then affect config_setting resolution. We can then do away with all the config setting groups.
So something like:
def _python_major_minor_impl(ctx):
full_version = ctx.attr._python_version_flag
major_minor = <parse full into X.Y>
return [FeatureFlagInfo(value=major_minor)]
python_major_minor = rule(
impl = _python_major_minor,
build_setting_value = config.string(flag=False)
attrs = {"_python_version_flag": attr.label("//python/config_settings:python_version")}
)
python_major_minor(name = "_python_major_minor")
[
config_setting(
name = "is_python_3.{}",
flag_values = {":_python_major_minor": "3.{}"}
)
for v in (5, 6, 7, 8, 9, 10, ...)
]
| if ctx.attr.values and value not in ctx.attr.values: | ||
| fail(( | ||
| "Invalid --python_version value: {actual}\nAllowed values {allowed}" | ||
| ).format( | ||
| actual = value, | ||
| allowed = ", ".join(sorted(ctx.attr.values)), | ||
| )) |
There was a problem hiding this comment.
Lets just drop the values restriction on the flag entirely. I'm just not convinced all the extra wiring is worth it, and it just artificially constrains the values, requiring extra work from users to get a value into the list.
Since the flag is no longer coupled to the known values, move the flag definition to be directly in the BUILD file, like the other flags.
Worst case, you set a value that doesn't map to a toolchain, and you end up getting the default. Surprising, but not that bad. We can also build in guard elsewhere that use information from the analysis phase instead of loading phase. The two ideas that come to mind are:
- In py_binary or equiv, make the //python:toolchain_type (fake) optional. If toolchain lookup fails, then we compare the flag to the list of known values. If the flag isn't in the list, then we print an error about the missing toolchain and mention the flag not being in the list of known values as a hint.
- In the python_version flag definition, make
valuesa label and point it to e.g.@pythons_hub//:known_versions(if bzlmod is true). Then fail at analysis time in the flag logic. - Make everything depend on an internal "check the flag value" target that does the above.
The second option in particular would make the bzlmod/workspace situation much easier -- we can just put an X if bzlmod else None clause in the attr.label value itself.
| @@ -54,6 +55,15 @@ def py_repositories(): | |||
| internal_config_repo, | |||
| name = "rules_python_internal", | |||
| ) | |||
| maybe( | |||
| hub_repo, | |||
| name = "pythons_hub", | |||
There was a problem hiding this comment.
Hrm. How do we get the list of values into pythons_hub for workspace builds?
Or is the idea: we just want the repo to exist so that load() can work?
With the suggested snippet of code, this greatly simplified the codebase
aignas
force-pushed
the
chore/remove-default-values-for-python-version
branch
from
d7a90fa
to
60dae67
Compare
|
I have pivoted the change and just removed all of the |
python/private/config_settings.bzl
Outdated
| ) | ||
|
|
||
| _python_major_minor( | ||
| name = "python_version_major_minor", |
There was a problem hiding this comment.
name is up for bike shedding.
There was a problem hiding this comment.
I think the name is fine -- just make it more internal sounding -- prefix it with "_". It's not a flag, and shouldn't be directly set, so lets just hide it as best we can so it isn't an attractive nuisance.
(or move it under private somewhere, but we have other internal names here, so i think an underscore is sufficient).
python/private/config_settings.bzl
Outdated
| ) | ||
|
|
||
| _python_major_minor( | ||
| name = "python_version_major_minor", |
There was a problem hiding this comment.
I think the name is fine -- just make it more internal sounding -- prefix it with "_". It's not a flag, and shouldn't be directly set, so lets just hide it as best we can so it isn't an attractive nuisance.
(or move it under private somewhere, but we have other internal names here, so i think an underscore is sufficient).
python/private/config_settings.bzl
Outdated
| actual = "_{}_group".format(name), | ||
| visibility = kwargs.get("visibility", []), | ||
| ) | ||
| _PYTHON_MINOR_VERSION_FLAG = str(Label("//python/config_settings:python_version_major_minor")) |
There was a problem hiding this comment.
Suggest to name the symbol after the underlying name
| _PYTHON_MINOR_VERSION_FLAG = str(Label("//python/config_settings:python_version_major_minor")) | |
| _PYTHON_MAJOR_MINOR_VERSION_FLAG = str(Label("//python/config_settings:python_version_major_minor")) |
… types as internal
…ark the types as internal
This is a different take on #2205.
Summary:
//python/config_settings:python_version.is_python_config_settingmacro and use a different implementation toretain the same functionality.
This in general will improve the situation on how the toolchains are registered
and hopefully is a step towards resolving issues for #2081.