Skip to content
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

chore: remove mandatory default values for python version flag #2217

Merged
12 changes: 11 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ A brief description of the categories of changes:
### Removed
* (toolchains): Removed accidentally exposed `http_archive` symbol from
`python/repositories.bzl`.
* (toolchains): An internal _is_python_config_setting_ has been removed
from rules_python as it can now be replaced by:
```starlark
native.config_setting(
name = "my_config_setting",
flag_values = {
"@rules_python//python/config_settings:python_version_major_minor": "3.11",
},
)
```

## [0.35.0] - 2024-08-15

Expand Down Expand Up @@ -273,7 +283,7 @@ A brief description of the categories of changes:
be automatically deleted correctly. For example, if `python_generation_mode`
is set to package, when `__init__.py` is deleted, the `py_library` generated
for this package before will be deleted automatically.
* (whl_library): Use `is_python_config_setting` to correctly handle multi-python
* (whl_library): Use _is_python_config_setting_ to correctly handle multi-python
version dependency select statements when the `experimental_target_platforms`
includes the Python ABI. The default python version case within the select is
also now handled correctly, stabilizing the implementation.
Expand Down
8 changes: 8 additions & 0 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ python.toolchain(
is_default = True,
python_version = "3.11",
)
python.toolchain(
python_version = "3.12",
)
use_repo(python, "python_3_11", "python_versions", "pythons_hub")

# This call registers the Python toolchains.
Expand Down Expand Up @@ -95,6 +98,11 @@ dev_pip.parse(
python_version = "3.11",
requirements_lock = "//docs:requirements.txt",
)
dev_pip.parse(
hub_name = "dev_pip",
python_version = "3.12",
requirements_lock = "//docs:requirements.txt",
)
dev_pip.parse(
hub_name = "pypiserver",
python_version = "3.11",
Expand Down
14 changes: 12 additions & 2 deletions examples/bzlmod/MODULE.bazel.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions python/config_settings/config_settings.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,7 @@
load(
"//python/private:config_settings.bzl",
_construct_config_settings = "construct_config_settings",
_is_python_config_setting = "is_python_config_setting",
)

# This is exposed only for cases where the pip hub repo needs to use this rule
# to define hub-repo scoped config_settings for platform specific wheel
# support.
is_python_config_setting = _is_python_config_setting

# This is exposed for usage in rules_python only.
construct_config_settings = _construct_config_settings
235 changes: 71 additions & 164 deletions python/private/config_settings.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -18,149 +18,10 @@
load("@bazel_skylib//lib:selects.bzl", "selects")
load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo")
load("//python:versions.bzl", "MINOR_MAPPING", "TOOL_VERSIONS")
load(":semver.bzl", "semver")

_PYTHON_VERSION_FLAG = str(Label("//python/config_settings:python_version"))

def _ver_key(s):
major, _, s = s.partition(".")
minor, _, s = s.partition(".")
micro, _, s = s.partition(".")
return (int(major), int(minor), int(micro))

def _flag_values(*, python_versions, minor_mapping):
"""Construct a map of python_version to a list of toolchain values.

This mapping maps the concept of a config setting to a list of compatible toolchain versions.
For using this in the code, the VERSION_FLAG_VALUES should be used instead.

Args:
python_versions: {type}`list[str]` X.Y.Z` python versions.
minor_mapping: {type}`dict[str, str]` `X.Y` to `X.Y.Z` mapping.

Returns:
A `map[str, list[str]]`. Each key is a python_version flag value. Each value
is a list of the python_version flag values that should match when for the
`key`. For example:
```
"3.8" -> ["3.8", "3.8.1", "3.8.2", ..., "3.8.19"] # All 3.8 versions
"3.8.2" -> ["3.8.2"] # Only 3.8.2
"3.8.19" -> ["3.8.19", "3.8"] # The latest version should also match 3.8 so
as when the `3.8` toolchain is used we just use the latest `3.8` toolchain.
this makes the `select("is_python_3.8.19")` work no matter how the user
specifies the latest python version to use.
```
"""
ret = {}

for micro_version in sorted(python_versions, key = _ver_key):
minor_version, _, _ = micro_version.rpartition(".")

# This matches the raw flag value, e.g. --//python/config_settings:python_version=3.8
# It's private because matching the concept of e.g. "3.8" value is done
# using the `is_python_X.Y` config setting group, which is aware of the
# minor versions that could match instead.
ret.setdefault(minor_version, [minor_version]).append(micro_version)

# Ensure that is_python_3.9.8 is matched if python_version is set
# to 3.9 if minor_mapping points to 3.9.8
default_micro_version = minor_mapping[minor_version]
ret[micro_version] = [micro_version, minor_version] if default_micro_version == micro_version else [micro_version]

return ret

VERSION_FLAG_VALUES = _flag_values(python_versions = TOOL_VERSIONS.keys(), minor_mapping = MINOR_MAPPING)

def is_python_config_setting(name, *, python_version, reuse_conditions = None, **kwargs):
"""Create a config setting for matching 'python_version' configuration flag.

This function is mainly intended for internal use within the `whl_library` and `pip_parse`
machinery.

The matching of the 'python_version' flag depends on the value passed in
`python_version` and here is the example for `3.8` (but the same applies
to other python versions present in @//python:versions.bzl#TOOL_VERSIONS):
* "3.8" -> ["3.8", "3.8.1", "3.8.2", ..., "3.8.19"] # All 3.8 versions
* "3.8.2" -> ["3.8.2"] # Only 3.8.2
* "3.8.19" -> ["3.8.19", "3.8"] # The latest version should also match 3.8 so
as when the `3.8` toolchain is used we just use the latest `3.8` toolchain.
this makes the `select("is_python_3.8.19")` work no matter how the user
specifies the latest python version to use.

Args:
name: name for the target that will be created to be used in select statements.
python_version: The python_version to be passed in the `flag_values` in the
`config_setting`. Depending on the version, the matching python version list
can be as described above.
reuse_conditions: A dict of version to version label for which we should
reuse config_setting targets instead of creating them from scratch. This
is useful when using is_python_config_setting multiple times in the
same package with the same `major.minor` python versions.
**kwargs: extra kwargs passed to the `config_setting`.
"""
if python_version not in name:
fail("The name '{}' must have the python version '{}' in it".format(name, python_version))

if python_version not in VERSION_FLAG_VALUES:
fail("The 'python_version' must be known to 'rules_python', choose from the values: {}".format(VERSION_FLAG_VALUES.keys()))

python_versions = VERSION_FLAG_VALUES[python_version]
extra_flag_values = kwargs.pop("flag_values", {})
if _PYTHON_VERSION_FLAG in extra_flag_values:
fail("Cannot set '{}' in the flag values".format(_PYTHON_VERSION_FLAG))

if len(python_versions) == 1:
native.config_setting(
name = name,
flag_values = {
_PYTHON_VERSION_FLAG: python_version,
} | extra_flag_values,
**kwargs
)
return

reuse_conditions = reuse_conditions or {}
create_config_settings = {
"_{}".format(name).replace(python_version, version): {_PYTHON_VERSION_FLAG: version}
for version in python_versions
if not reuse_conditions or version not in reuse_conditions
}
match_any = list(create_config_settings.keys())
for version, condition in reuse_conditions.items():
if len(VERSION_FLAG_VALUES[version]) == 1:
match_any.append(condition)
continue

# Convert the name to an internal label that this function would create,
# so that we are hitting the config_setting and not the config_setting_group.
condition = Label(condition)
if hasattr(condition, "same_package_label"):
condition = condition.same_package_label("_" + condition.name)
else:
condition = condition.relative("_" + condition.name)

match_any.append(condition)

for name_, flag_values_ in create_config_settings.items():
native.config_setting(
name = name_,
flag_values = flag_values_ | extra_flag_values,
**kwargs
)

# An alias pointing to an underscore-prefixed config_setting_group
# is used because config_setting_group creates
# `is_{version}_N` targets, which are easily confused with the
# `is_{minor}.{micro}` (dot) targets.
selects.config_setting_group(
name = "_{}_group".format(name),
match_any = match_any,
visibility = ["//visibility:private"],
)
native.alias(
name = name,
actual = "_{}_group".format(name),
visibility = kwargs.get("visibility", []),
)
_PYTHON_MINOR_VERSION_FLAG = str(Label("//python/config_settings:python_version_major_minor"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to name the symbol after the underlying name

Suggested change
_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"))


def construct_config_settings(name = None): # buildifier: disable=function-docstring
"""Create a 'python_version' config flag and construct all config settings used in rules_python.
Expand All @@ -178,40 +39,70 @@ def construct_config_settings(name = None): # buildifier: disable=function-docs
# also prevents version-unaware targets from inadvertently matching
# a select condition when they shouldn't.
build_setting_default = "",
values = [""] + VERSION_FLAG_VALUES.keys(),
visibility = ["//visibility:public"],
)

_python_major_minor(
name = "python_version_major_minor",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name is up for bike shedding.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

build_setting_default = "",
visibility = ["//visibility:public"],
)

native.config_setting(
name = "is_python_version_unset",
flag_values = {
Label("//python/config_settings:python_version"): "",
},
flag_values = {_PYTHON_VERSION_FLAG: ""},
visibility = ["//visibility:public"],
)

for version, matching_versions in VERSION_FLAG_VALUES.items():
is_python_config_setting(
name = "is_python_{}".format(version),
python_version = version,
reuse_conditions = {
v: native.package_relative_label("is_python_{}".format(v))
for v in matching_versions
if v != version
},
# This matches the raw flag value, e.g. --//python/config_settings:python_version=3.8
# It's private because matching the concept of e.g. "3.8" value is done
# using the `is_python_X.Y` config setting group, which is aware of the
# minor versions that could match instead.
for minor in range(20):
native.config_setting(
name = "is_python_3.{}".format(minor),
flag_values = {_PYTHON_MINOR_VERSION_FLAG: "3.{}".format(minor)},
visibility = ["//visibility:public"],
)

for version in TOOL_VERSIONS.keys():
minor_version, _, _ = version.rpartition(".")
if MINOR_MAPPING[minor_version] != version:
native.config_setting(
name = "is_python_{}".format(version),
flag_values = {":python_version": version},
visibility = ["//visibility:public"],
)
continue

# Also need to match the minor version when using
name = "is_python_{}".format(version)
native.config_setting(
name = "_" + name,
flag_values = {":python_version": version},
visibility = ["//visibility:public"],
)

# An alias pointing to an underscore-prefixed config_setting_group
# is used because config_setting_group creates
# `is_{version}_N` targets, which are easily confused with the
# `is_{minor}.{micro}` (dot) targets.
selects.config_setting_group(
name = "_{}_group".format(name),
match_any = [
":_is_python_{}".format(version),
":is_python_{}".format(minor_version),
],
visibility = ["//visibility:private"],
)
native.alias(
aignas marked this conversation as resolved.
Show resolved Hide resolved
name = name,
actual = "_{}_group".format(name),
visibility = ["//visibility:public"],
)

def _python_version_flag_impl(ctx):
value = ctx.build_setting_value
if value not in ctx.attr.values:
fail((
"Invalid --python_version value: {actual}\nAllowed values {allowed}"
).format(
actual = value,
allowed = ", ".join(sorted(ctx.attr.values)),
))

return [
# BuildSettingInfo is the original provider returned, so continue to
# return it for compatibility
Expand All @@ -227,9 +118,25 @@ def _python_version_flag_impl(ctx):
_python_version_flag = rule(
implementation = _python_version_flag_impl,
build_setting = config.string(flag = True),
attrs = {},
)

def _python_major_minor_impl(ctx):
input = ctx.attr._python_version_flag[config_common.FeatureFlagInfo].value
if input:
version = semver(input)
value = "{}.{}".format(version.major, version.minor)
else:
value = ""

return [config_common.FeatureFlagInfo(value = value)]

_python_major_minor = rule(
implementation = _python_major_minor_impl,
build_setting = config.string(flag = False),
attrs = {
"values": attr.string_list(
doc = "Allowed values.",
"_python_version_flag": attr.label(
default = _PYTHON_VERSION_FLAG,
),
},
)
9 changes: 4 additions & 5 deletions python/private/pypi/generate_whl_library_build_bazel.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,13 @@ def _render_config_settings(dependencies_by_platform):
constraint_values_str = render.indent(render.list(constraint_values)).lstrip()

if abi:
if not loads:
loads.append("""load("@rules_python//python/config_settings:config_settings.bzl", "is_python_config_setting")""")

additional_content.append(
"""\
is_python_config_setting(
config_setting(
name = "is_{name}",
python_version = "3.{minor_version}",
flag_values = {{
"@rules_python//python/config_settings:python_version_major_minor": "3.{minor_version}",
}},
constraint_values = {constraint_values},
visibility = ["//visibility:private"],
)""".format(
Expand Down
Loading