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

aignas · web-flow · commit 654b4d54eeee · 2024-09-17T04:12:48.000Z
This is a different take on #2205. Summary: - Remove version constraints from the `//python/config_settings:python_version`. - Remove `is_python_config_setting` macro and use a different implementation to retain 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.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -79,6 +79,7 @@ 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_ macro has been removed.
 
 ## [0.35.0] - 2024-08-15
 
@@ -274,7 +275,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.
diff --git a/examples/bzlmod/MODULE.bazel.lock b/examples/bzlmod/MODULE.bazel.lock
diff --git a/python/config_settings/BUILD.bazel b/python/config_settings/BUILD.bazel
@@ -1,4 +1,5 @@
 load("@bazel_skylib//rules:common_settings.bzl", "string_flag")
+load("//python:versions.bzl", "MINOR_MAPPING", "TOOL_VERSIONS")
 load(
     "//python/private:flags.bzl",
     "BootstrapImplFlag",
@@ -27,6 +28,8 @@ filegroup(
 
 construct_config_settings(
     name = "construct_config_settings",
+    minor_mapping = MINOR_MAPPING,
+    versions = TOOL_VERSIONS.keys(),
 )
 
 string_flag(
diff --git a/python/config_settings/config_settings.bzl b/python/config_settings/config_settings.bzl
@@ -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
diff --git a/python/private/BUILD.bazel b/python/private/BUILD.bazel
@@ -78,8 +78,9 @@ bzl_library(
     name = "config_settings_bzl",
     srcs = ["config_settings.bzl"],
     deps = [
-        "//python:versions_bzl",
+        ":semver_bzl",
         "@bazel_skylib//lib:selects",
+        "@bazel_skylib//rules:common_settings",
     ],
 )
 
diff --git a/python/private/config_settings.bzl b/python/private/config_settings.bzl
@@ -17,201 +17,95 @@
 
 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"))
+_PYTHON_VERSION_FLAG = Label("//python/config_settings:python_version")
+_PYTHON_VERSION_MAJOR_MINOR_FLAG = Label("//python/config_settings:_python_version_major_minor")
 
-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", []),
-    )
-
-def construct_config_settings(name = None):  # buildifier: disable=function-docstring
+def construct_config_settings(*, name, versions, minor_mapping):  # buildifier: disable=function-docstring
     """Create a 'python_version' config flag and construct all config settings used in rules_python.
 
     This mainly includes the targets that are used in the toolchain and pip hub
     repositories that only match on the 'python_version' flag values.
 
     Args:
-        name(str): A dummy name value that is no-op for now.
+        name: {type}`str` A dummy name value that is no-op for now.
+        versions: {type}`list[str]` A list of versions to build constraint settings for.
+        minor_mapping: {type}`dict[str, str]` A mapping from `X.Y` to `X.Y.Z` python versions.
     """
+    _ = name  # @unused
     _python_version_flag(
-        name = "python_version",
+        name = _PYTHON_VERSION_FLAG.name,
         # TODO: The default here should somehow match the MODULE config. Until
         # then, use the empty string to indicate an unknown version. This
         # 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_version_major_minor_flag(
+        name = _PYTHON_VERSION_MAJOR_MINOR_FLAG.name,
+        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
-            },
+    _reverse_minor_mapping = {full: minor for minor, full in minor_mapping.items()}
+    for version in versions:
+        minor_version = _reverse_minor_mapping.get(version)
+        if not minor_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(
+            name = name,
+            actual = "_{}_group".format(name),
+            visibility = ["//visibility:public"],
+        )
+
+    # 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 minor_mapping.keys():
+        native.config_setting(
+            name = "is_python_{}".format(minor),
+            flag_values = {_PYTHON_VERSION_MAJOR_MINOR_FLAG: minor},
             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
@@ -227,9 +121,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_version_major_minor_flag_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_version_major_minor_flag = rule(
+    implementation = _python_version_major_minor_flag_impl,
+    build_setting = config.string(flag = False),
     attrs = {
-        "values": attr.string_list(
-            doc = "Allowed values.",
+        "_python_version_flag": attr.label(
+            default = _PYTHON_VERSION_FLAG,
         ),
     },
 )
diff --git a/python/private/pypi/generate_whl_library_build_bazel.bzl b/python/private/pypi/generate_whl_library_build_bazel.bzl
@@ -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(
diff --git a/tests/config_settings/construct_config_settings_tests.bzl b/tests/config_settings/construct_config_settings_tests.bzl
diff --git a/tests/pypi/generate_whl_library_build_bazel/generate_whl_library_build_bazel_tests.bzl b/tests/pypi/generate_whl_library_build_bazel/generate_whl_library_build_bazel_tests.bzl
