From 7db60b1d1fc066c9c3ef7e47a962fd00eda81f41 Mon Sep 17 00:00:00 2001 From: Ignas Anikevicius <240938+aignas@users.noreply.github.com> Date: Fri, 14 Jun 2024 19:33:06 +0900 Subject: [PATCH] fix(toolchain): do not force users to depend on optional toolchains With this change we set the `target_settings` to something that is not going to be ever matched if the `precompile` flag is not enabled. Users wanting to use the `precompile` feature will need to set the config_setting value to `enabled` instead of `auto`. Fixes #1967 --- CHANGELOG.md | 8 +++- python/config_settings/BUILD.bazel | 43 +++++++++++++++++++ python/private/BUILD.bazel | 1 + python/private/autodetecting_toolchain.bzl | 3 +- python/private/flags.bzl | 2 +- python/private/py_toolchain_suite.bzl | 5 ++- .../precompile/precompile_tests.bzl | 5 +++ 7 files changed, 63 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ba7045a70..0ab9e3e9ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,13 @@ A brief description of the categories of changes: * `protobuf`/`com_google_protobuf` dependency bumped to `v24.4` ### Fixed -* Nothing yet +* The targets would depend on the `exec_tools_toolchain_type` toolchain if it + was registered even though the pre-compilation was not enabled. This has been + fixed to not be the case anymore. Fixes + [#1967](https://github.com/bazelbuild/rules_python/issues/1967). Users + wanting to use the `precompile` feature may have to set the config setting to + always do the precompilation and per-target pre-compilation is for now + disabled. ### Removed * Nothing yet diff --git a/python/config_settings/BUILD.bazel b/python/config_settings/BUILD.bazel index 2742d18977..039b239290 100644 --- a/python/config_settings/BUILD.bazel +++ b/python/config_settings/BUILD.bazel @@ -1,3 +1,4 @@ +load("@bazel_skylib//lib:selects.bzl", "selects") load("@bazel_skylib//rules:common_settings.bzl", "string_flag") load( "//python/private:flags.bzl", @@ -128,3 +129,45 @@ string_flag( ) for flag in INTERNAL_FLAGS ] + +config_setting( + name = "_precompile_enabled", + flag_values = { + ":precompile": PrecompileFlag.ENABLED, + }, + # Only public because it is used in toolchain definition + visibility = ["//visibility:public"], +) + +config_setting( + name = "_precompile_enabled_generated_source", + flag_values = { + ":precompile": PrecompileFlag.IF_GENERATED_SOURCE, + }, + # Only public because it is used in toolchain definition + visibility = ["//visibility:public"], +) + +config_setting( + name = "_precompile_enabled_force", + flag_values = { + ":precompile": PrecompileFlag.FORCE_ENABLED, + }, + # Only public because it is used in toolchain definition + visibility = ["//visibility:public"], +) + +# FIXME @aignas 2024-06-14: this currently does not handle the `auto` value of +# the flag correctly. Right now the behaviour is to disable the pre-compilation +# via the _precompile_flag_get_effective_value function default to disabled if +# it the flag is "AUTO", however we may need to revisit this. +selects.config_setting_group( + name = "is_precompile_enabled", + match_any = [ + ":_precompile_enabled", + ":_precompile_enabled_force", + ":_precompile_enabled_generated_source", + ], + # Only public because it is used in toolchain definition + visibility = ["//visibility:public"], +) diff --git a/python/private/BUILD.bazel b/python/private/BUILD.bazel index 422ed9c7c2..e2a2bc01a2 100644 --- a/python/private/BUILD.bazel +++ b/python/private/BUILD.bazel @@ -59,6 +59,7 @@ bzl_library( name = "autodetecting_toolchain_bzl", srcs = ["autodetecting_toolchain.bzl"], deps = [ + ":toolchain_types_bzl", "//python:py_runtime_bzl", "//python:py_runtime_pair_bzl", ], diff --git a/python/private/autodetecting_toolchain.bzl b/python/private/autodetecting_toolchain.bzl index 55c95699c9..75535bf5d6 100644 --- a/python/private/autodetecting_toolchain.bzl +++ b/python/private/autodetecting_toolchain.bzl @@ -16,6 +16,7 @@ load("//python:py_runtime.bzl", "py_runtime") load("//python:py_runtime_pair.bzl", "py_runtime_pair") +load("//python/private:toolchain_types.bzl", "TARGET_TOOLCHAIN_TYPE") def define_autodetecting_toolchain(name): """Defines the autodetecting Python toolchain. @@ -65,6 +66,6 @@ def define_autodetecting_toolchain(name): native.toolchain( name = name, toolchain = ":_autodetecting_py_runtime_pair", - toolchain_type = ":toolchain_type", + toolchain_type = TARGET_TOOLCHAIN_TYPE, visibility = ["//visibility:public"], ) diff --git a/python/private/flags.bzl b/python/private/flags.bzl index d141f72eee..b09df06f63 100644 --- a/python/private/flags.bzl +++ b/python/private/flags.bzl @@ -34,7 +34,7 @@ BootstrapImplFlag = enum( def _precompile_flag_get_effective_value(ctx): value = ctx.attr._precompile_flag[BuildSettingInfo].value if value == PrecompileFlag.AUTO: - value = PrecompileFlag.DISABLED + value = PrecompileFlag.IF_GENERATED_SOURCE return value # Determines if Python source files should be compiled at build time. diff --git a/python/private/py_toolchain_suite.bzl b/python/private/py_toolchain_suite.bzl index 174c36f782..dfca3643c9 100644 --- a/python/private/py_toolchain_suite.bzl +++ b/python/private/py_toolchain_suite.bzl @@ -107,7 +107,10 @@ def py_toolchain_suite(*, prefix, user_repository_name, python_version, set_pyth ), toolchain_type = EXEC_TOOLS_TOOLCHAIN_TYPE, # The target settings capture the Python version - target_settings = target_settings, + target_settings = select({ + Label("//python/config_settings:is_precompile_enabled"): target_settings, + "//conditions:default": [Label("//python/config_settings:is_precompile_enabled")], + }), exec_compatible_with = kwargs.get("target_compatible_with"), ) diff --git a/tests/base_rules/precompile/precompile_tests.bzl b/tests/base_rules/precompile/precompile_tests.bzl index 02ab4ab19c..8ea3ac927a 100644 --- a/tests/base_rules/precompile/precompile_tests.bzl +++ b/tests/base_rules/precompile/precompile_tests.bzl @@ -61,6 +61,7 @@ def _test_precompile_enabled_setup(name, py_rule, **kwargs): target = name + "_subject", config_settings = { "//command_line_option:extra_toolchains": _TEST_TOOLCHAINS, + PRECOMPILE: "enabled", }, ) @@ -119,6 +120,7 @@ def _test_pyc_only(name): config_settings = { "//command_line_option:extra_toolchains": _TEST_TOOLCHAINS, ##PRECOMPILE_SOURCE_RETENTION: "omit_source", + PRECOMPILE: "enabled", }, target = name + "_subject", ) @@ -161,6 +163,7 @@ def _test_precompile_if_generated(name): target = name + "_subject", config_settings = { "//command_line_option:extra_toolchains": _TEST_TOOLCHAINS, + PRECOMPILE: "enabled", }, ) @@ -203,6 +206,7 @@ def _test_omit_source_if_generated_source(name): config_settings = { "//command_line_option:extra_toolchains": _TEST_TOOLCHAINS, PRECOMPILE_SOURCE_RETENTION: "omit_if_generated_source", + PRECOMPILE: "enabled", }, ) @@ -288,6 +292,7 @@ def _test_precompiler_action(name): target = name + "_subject", config_settings = { "//command_line_option:extra_toolchains": _TEST_TOOLCHAINS, + PRECOMPILE: "enabled", }, )