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

feat(config_settings): allow matching minor version of python_version flag #1555

Merged
merged 9 commits into from
Jan 16, 2024
6 changes: 4 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ A brief description of the categories of changes:
`python_register_toolchains`.
Note that this only available on the Starlark implementation of the provider.

* (config_settings) Added `//python/config_settings:is_python_X.Y` config
settings to match on minor Python version. These settings match any `X.Y`
version instead of just an exact `X.Y.Z` version.

## [0.28.0] - 2024-01-07

[0.28.0]: https://github.com/bazelbuild/rules_python/releases/tag/0.28.0
Expand Down Expand Up @@ -234,8 +238,6 @@ Breaking changes:
* (utils) Added a `pip_utils` struct with a `normalize_name` function to allow users
to find out how `rules_python` would normalize a PyPI distribution name.

[0.27.0]: https://github.com/bazelbuild/rules_python/releases/tag/0.27.0

## [0.26.0] - 2023-10-06

### Changed
Expand Down
5 changes: 4 additions & 1 deletion python/config_settings/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ filegroup(
visibility = ["//python:__pkg__"],
)

construct_config_settings(python_versions = TOOL_VERSIONS.keys())
construct_config_settings(
name = "construct_config_settings",
python_versions = TOOL_VERSIONS.keys(),
)
59 changes: 51 additions & 8 deletions python/config_settings/config_settings.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,69 @@
"""This module is used to construct the config settings in the BUILD file in this same package.
"""

load("@bazel_skylib//lib:selects.bzl", "selects")
load("@bazel_skylib//rules:common_settings.bzl", "string_flag")

# buildifier: disable=unnamed-macro
def construct_config_settings(python_versions):
def construct_config_settings(name, python_versions):
"""Constructs a set of configs for all Python versions.

Args:
python_versions: The Python versions supported by rules_python.
name: str, unused; only specified to satisfy buildifier lint checks
and allow programatic modification of the target.
python_versions: list of all (x.y.z) Python versions supported by rules_python.
"""

# Maps e.g. "3.8" -> ["3.8.1", "3.8.2", etc]
minor_to_micro_versions = {}
allowed_flag_values = []
for micro_version in python_versions:
minor, _, _ = micro_version.rpartition(".")
minor_to_micro_versions.setdefault(minor, []).append(micro_version)
allowed_flag_values.append(micro_version)

string_flag(
name = "python_version",
# TODO: The default here should somehow match the MODULE config
build_setting_default = python_versions[0],
Copy link
Contributor Author

@dizzy57 dizzy57 Nov 13, 2023

Choose a reason for hiding this comment

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

Using build_setting_default = python_versions[0] here looks like a minor bug, BTW.

It enables a config setting, and if user manages to configure a toolchain that matches this full version, this toolchain will be selected by default, instead of the one marked by the is_default attr in python.toolchain(...).

I think a better approach would be to have something like build_setting_default = "default" and values = ["default"] + allowed_flag_values passed to the string_flag(...).

Copy link
Contributor

Choose a reason for hiding this comment

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

"default" is a bit overloaded here.

What python.toolchain's is_default=True affects is what //conditions:default points to, essentially. If that is hit, it basically means either the version-unaware rules are being used, or there is no matching version-aware toolchain configured.

The build_setting_default value here affects what the version-aware rules use if there isn't something overriding the value (i.e. command line flag, or the target-specific override).

I agree that it would be best if the version set with is_default=True in the module config should match what this flag defaults to. I think this is possible? By putting the value in a generated repo and loading it here, similar to how rules_python_internal works. But lets have that as part of a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a comment to note this

values = python_versions,
values = sorted(allowed_flag_values),
visibility = ["//visibility:public"],
)

for python_version in python_versions:
python_version_constraint_setting = "is_python_" + python_version
for minor_version, micro_versions in minor_to_micro_versions.items():
# 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.
rickeylev marked this conversation as resolved.
Show resolved Hide resolved
equals_minor_version_name = "_python_version_flag_equals_" + minor_version
native.config_setting(
name = python_version_constraint_setting,
flag_values = {":python_version": python_version},
name = equals_minor_version_name,
flag_values = {":python_version": minor_version},
)

matches_minor_version_names = [equals_minor_version_name]

for micro_version in micro_versions:
is_micro_version_name = "is_python_" + micro_version
native.config_setting(
name = is_micro_version_name,
flag_values = {":python_version": micro_version},
visibility = ["//visibility:public"],
)
matches_minor_version_names.append(is_micro_version_name)

# This is prefixed with an underscore to prevent confusion due to how
# config_setting_group is implemented and how our micro-version targets
# are named. config_setting_group will generate targets like
# "is_python_3.10_1" (where the `_N` suffix is len(match_any).
# Meanwhile, the micro-version tarets are named "is_python_3.10.1" --
# just a single dot vs underscore character difference.
selects.config_setting_group(
name = "_is_python_" + minor_version,
match_any = matches_minor_version_names,
)

native.alias(
name = "is_python_" + minor_version,
actual = "_is_python_" + minor_version,
visibility = ["//visibility:public"],
)
19 changes: 19 additions & 0 deletions tests/config_settings/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Copyright 2022 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

load(":construct_config_settings_tests.bzl", "construct_config_settings_test_suite")

construct_config_settings_test_suite(
name = "construct_config_settings_tests",
)
81 changes: 81 additions & 0 deletions tests/config_settings/construct_config_settings_tests.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# Copyright 2024 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
"""Tests for construction of Python version matching config settings."""

load("@rules_testing//lib:analysis_test.bzl", "analysis_test")
load("@rules_testing//lib:test_suite.bzl", "test_suite")
load("@rules_testing//lib:truth.bzl", "subjects")
load("@rules_testing//lib:util.bzl", rt_util = "util")

_tests = []

def _subject_impl(ctx):
_ = ctx # @unused
return [DefaultInfo()]

_subject = rule(
implementation = _subject_impl,
attrs = {
"match_micro": attr.string(),
"match_minor": attr.string(),
"no_match": attr.string(),
},
)

def _test_minor_version_matching(name):
rt_util.helper_target(
_subject,
name = name + "_subject",
match_minor = select({
"//python/config_settings:is_python_3.11": "matched-3.11",
"//conditions:default": "matched-default",
}),
match_micro = select({
"//python/config_settings:is_python_3.11": "matched-3.11",
"//conditions:default": "matched-default",
}),
no_match = select({
"//python/config_settings:is_python_3.12": "matched-3.12",
"//conditions:default": "matched-default",
}),
)

analysis_test(
name = name,
target = name + "_subject",
impl = _test_minor_version_matching_impl,
config_settings = {
str(Label("//python/config_settings:python_version")): "3.11.1",
},
)

def _test_minor_version_matching_impl(env, target):
target = env.expect.that_target(target)
target.attr("match_minor", factory = subjects.str).equals(
"matched-3.11",
)
target.attr("match_micro", factory = subjects.str).equals(
"matched-3.11",
)
target.attr("no_match", factory = subjects.str).equals(
"matched-default",
)

_tests.append(_test_minor_version_matching)

def construct_config_settings_test_suite(name):
test_suite(
name = name,
tests = _tests,
)
Loading