From 611eda87e48a3943808216cf4e4de875040b09aa Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Tue, 31 Dec 2024 15:46:42 -0800 Subject: [PATCH] refactor: fold per-target python version into base rules (#2541) Today, specifying the Python version for a target requires using the version-aware rules in `transition.bzl` (or the generated equivalents bound to a specific Python version). With the rules rewritten in Bazel, that functionality can be moved into the base rules themselves. Moving the logic into the base rules simplifies the implementation and avoids having to re-implement subtle behaviors in the wrappers to correctly emulate the wrapped target. For backwards compatibility, the symbols in `transition.bzl` are left as aliases to the underlying rules. --- CHANGELOG.md | 8 + python/config_settings/transition.bzl | 291 ++---------------- python/private/py_executable.bzl | 55 +++- .../transition/multi_version_tests.bzl | 6 +- 4 files changed, 90 insertions(+), 270 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad3f0a6da9..e842cf3265 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,14 @@ Unreleased changes template. not using {bzl:obj}`pip.parse.experimental_index_url_overrides`. * ({bzl:obj}`pip.parse`) Only query SimpleAPI for packages that have sha values in the `requirements.txt` file. +* (rules) The version-aware rules have been folded into the base rules and + the version-aware rules are now simply aliases for the base rules. The + `python_version` attribute is still used to specify the Python version. + +{#v0-0-0-deprecations} +#### Deprecations +* `//python/config_settings:transitions.bzl` and its `py_binary` and `py_test` + wrappers are deprecated. Use the regular rules instead. {#v0-0-0-fixed} ### Fixed diff --git a/python/config_settings/transition.bzl b/python/config_settings/transition.bzl index a7646dcda3..14e2a73dc4 100644 --- a/python/config_settings/transition.bzl +++ b/python/config_settings/transition.bzl @@ -14,266 +14,43 @@ """The transition module contains the rule definitions to wrap py_binary and py_test and transition them to the desired target platform. + +:::{versionchanged} VERSION_NEXT_PATCH +The `py_binary` and `py_test` symbols are aliases to the regular rules. Usages +of them should be changed to load the regular rules directly. +::: """ -load("@bazel_skylib//lib:dicts.bzl", "dicts") load("//python:py_binary.bzl", _py_binary = "py_binary") -load("//python:py_info.bzl", "PyInfo") -load("//python:py_runtime_info.bzl", "PyRuntimeInfo") load("//python:py_test.bzl", _py_test = "py_test") -load("//python/config_settings/private:py_args.bzl", "py_args") -load("//python/private:reexports.bzl", "BuiltinPyInfo", "BuiltinPyRuntimeInfo") - -def _transition_python_version_impl(_, attr): - return {"//python/config_settings:python_version": str(attr.python_version)} - -_transition_python_version = transition( - implementation = _transition_python_version_impl, - inputs = [], - outputs = ["//python/config_settings:python_version"], -) - -def _transition_py_impl(ctx): - target = ctx.attr.target - windows_constraint = ctx.attr._windows_constraint[platform_common.ConstraintValueInfo] - target_is_windows = ctx.target_platform_has_constraint(windows_constraint) - executable = ctx.actions.declare_file(ctx.attr.name + (".exe" if target_is_windows else "")) - ctx.actions.symlink( - is_executable = True, - output = executable, - target_file = target[DefaultInfo].files_to_run.executable, - ) - default_outputs = [] - if target_is_windows: - # NOTE: Bazel 6 + host=linux + target=windows results in the .exe extension missing - inner_bootstrap_path = _strip_suffix(target[DefaultInfo].files_to_run.executable.short_path, ".exe") - inner_bootstrap = None - inner_zip_file_path = inner_bootstrap_path + ".zip" - inner_zip_file = None - for file in target[DefaultInfo].files.to_list(): - if file.short_path == inner_bootstrap_path: - inner_bootstrap = file - elif file.short_path == inner_zip_file_path: - inner_zip_file = file - - # TODO: Use `fragments.py.build_python_zip` once Bazel 6 support is dropped. - # Which file the Windows .exe looks for depends on the --build_python_zip file. - # Bazel 7+ has APIs to know the effective value of that flag, but not Bazel 6. - # To work around this, we treat the existence of a .zip in the default outputs - # to mean --build_python_zip=true. - if inner_zip_file: - suffix = ".zip" - underlying_launched_file = inner_zip_file - else: - suffix = "" - underlying_launched_file = inner_bootstrap - - if underlying_launched_file: - launched_file_symlink = ctx.actions.declare_file(ctx.attr.name + suffix) - ctx.actions.symlink( - is_executable = True, - output = launched_file_symlink, - target_file = underlying_launched_file, - ) - default_outputs.append(launched_file_symlink) - - env = {} - for k, v in ctx.attr.env.items(): - env[k] = ctx.expand_location(v) - - providers = [ - DefaultInfo( - executable = executable, - files = depset(default_outputs, transitive = [target[DefaultInfo].files]), - runfiles = ctx.runfiles(default_outputs).merge(target[DefaultInfo].default_runfiles), - ), - # Ensure that the binary we're wrapping is included in code coverage. - coverage_common.instrumented_files_info( - ctx, - dependency_attributes = ["target"], - ), - target[OutputGroupInfo], - # TODO(f0rmiga): testing.TestEnvironment is deprecated in favour of RunEnvironmentInfo but - # RunEnvironmentInfo is not exposed in Bazel < 5.3. - # https://github.com/bazelbuild/rules_python/issues/901 - # https://github.com/bazelbuild/bazel/commit/dbdfa07e92f99497be9c14265611ad2920161483 - testing.TestEnvironment(env), - ] - if PyInfo in target: - providers.append(target[PyInfo]) - if BuiltinPyInfo != None and BuiltinPyInfo in target and PyInfo != BuiltinPyInfo: - providers.append(target[BuiltinPyInfo]) - - if PyRuntimeInfo in target: - providers.append(target[PyRuntimeInfo]) - if BuiltinPyRuntimeInfo != None and BuiltinPyRuntimeInfo in target and PyRuntimeInfo != BuiltinPyRuntimeInfo: - providers.append(target[BuiltinPyRuntimeInfo]) - return providers - -_COMMON_ATTRS = { - "deps": attr.label_list( - mandatory = False, - ), - "env": attr.string_dict( - mandatory = False, - ), - "python_version": attr.string( - mandatory = True, - ), - "srcs": attr.label_list( - allow_files = True, - mandatory = False, - ), - "target": attr.label( - executable = True, - cfg = "target", - mandatory = True, - providers = [PyInfo], - ), - # "tools" is a hack here. It should be "data" but "data" is not included by default in the - # location expansion in the same way it is in the native Python rules. The difference on how - # the Bazel deals with those special attributes differ on the LocationExpander, e.g.: - # https://github.com/bazelbuild/bazel/blob/ce611646/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java#L415-L429 - # - # Since the default LocationExpander used by ctx.expand_location is not the same as the native - # rules (it doesn't set "allowDataAttributeEntriesInLabel"), we use "tools" temporarily while a - # proper fix in Bazel happens. - # - # A fix for this was proposed in https://github.com/bazelbuild/bazel/pull/16381. - "tools": attr.label_list( - allow_files = True, - mandatory = False, - ), - # Required to Opt-in to the transitions feature. - "_allowlist_function_transition": attr.label( - default = "@bazel_tools//tools/allowlists/function_transition_allowlist", - ), - "_windows_constraint": attr.label( - default = "@platforms//os:windows", - ), -} - -_PY_TEST_ATTRS = { - # Magic attribute to help C++ coverage work. There's no - # docs about this; see TestActionBuilder.java - "_collect_cc_coverage": attr.label( - default = "@bazel_tools//tools/test:collect_cc_coverage", - executable = True, - cfg = "exec", - ), - # Magic attribute to make coverage work. There's no - # docs about this; see TestActionBuilder.java - "_lcov_merger": attr.label( - default = configuration_field(fragment = "coverage", name = "output_generator"), - executable = True, - cfg = "exec", - ), -} -_transition_py_binary = rule( - _transition_py_impl, - attrs = _COMMON_ATTRS | _PY_TEST_ATTRS, - cfg = _transition_python_version, - executable = True, - fragments = ["py"], -) - -_transition_py_test = rule( - _transition_py_impl, - attrs = _COMMON_ATTRS | _PY_TEST_ATTRS, - cfg = _transition_python_version, - test = True, - fragments = ["py"], -) - -def _py_rule(rule_impl, transition_rule, name, python_version, **kwargs): - pyargs = py_args(name, kwargs) - args = pyargs["args"] - data = pyargs["data"] - env = pyargs["env"] - srcs = pyargs["srcs"] - deps = pyargs["deps"] - main = pyargs["main"] - - # Attributes common to all build rules. - # https://bazel.build/reference/be/common-definitions#common-attributes - compatible_with = kwargs.pop("compatible_with", None) - deprecation = kwargs.pop("deprecation", None) - exec_compatible_with = kwargs.pop("exec_compatible_with", None) - exec_properties = kwargs.pop("exec_properties", None) - features = kwargs.pop("features", None) - restricted_to = kwargs.pop("restricted_to", None) - tags = kwargs.pop("tags", None) - target_compatible_with = kwargs.pop("target_compatible_with", None) - testonly = kwargs.pop("testonly", None) - toolchains = kwargs.pop("toolchains", None) - visibility = kwargs.pop("visibility", None) - - common_attrs = { - "compatible_with": compatible_with, - "deprecation": deprecation, - "exec_compatible_with": exec_compatible_with, - "exec_properties": exec_properties, - "features": features, - "restricted_to": restricted_to, - "target_compatible_with": target_compatible_with, - "testonly": testonly, - "toolchains": toolchains, - } - - # Test-specific extra attributes. - if "env_inherit" in kwargs: - common_attrs["env_inherit"] = kwargs.pop("env_inherit") - if "size" in kwargs: - common_attrs["size"] = kwargs.pop("size") - if "timeout" in kwargs: - common_attrs["timeout"] = kwargs.pop("timeout") - if "flaky" in kwargs: - common_attrs["flaky"] = kwargs.pop("flaky") - if "shard_count" in kwargs: - common_attrs["shard_count"] = kwargs.pop("shard_count") - if "local" in kwargs: - common_attrs["local"] = kwargs.pop("local") - - # Binary-specific extra attributes. - if "output_licenses" in kwargs: - common_attrs["output_licenses"] = kwargs.pop("output_licenses") - - rule_impl( - name = "_" + name, - args = args, - data = data, - deps = deps, - env = env, - srcs = srcs, - main = main, - tags = ["manual"] + (tags if tags else []), - visibility = ["//visibility:private"], - **dicts.add(common_attrs, kwargs) - ) - - return transition_rule( - name = name, - args = args, - deps = deps, - env = env, - python_version = python_version, - srcs = srcs, - tags = tags, - target = ":_" + name, - tools = data, - visibility = visibility, - **common_attrs - ) - -def py_binary(name, python_version, **kwargs): - return _py_rule(_py_binary, _transition_py_binary, name, python_version, **kwargs) - -def py_test(name, python_version, **kwargs): - return _py_rule(_py_test, _transition_py_test, name, python_version, **kwargs) +_DEPRECATION_MESSAGE = """ +The {name} symbol in @rules_python//python/config_settings:transition.bzl +is deprecated. It is an alias to the regular rule; use it directly instead: + load("@rules_python//python:{name}.bzl", "{name}") +""" -def _strip_suffix(s, suffix): - if s.endswith(suffix): - return s[:-len(suffix)] - else: - return s +def py_binary(**kwargs): + """[DEPRECATED] Deprecated alias for py_binary. + + Args: + **kwargs: keyword args forwarded onto {obj}`py_binary`. + """ + + deprecation = _DEPRECATION_MESSAGE.format(name = "py_binary") + if kwargs.get("deprecation"): + deprecation = kwargs.get("deprecation") + "\n\n" + deprecation + kwargs["deprecation"] = deprecation + _py_binary(**kwargs) + +def py_test(**kwargs): + """[DEPRECATED] Deprecated alias for py_test. + + Args: + **kwargs: keyword args forwarded onto {obj}`py_binary`. + """ + deprecation = _DEPRECATION_MESSAGE.format(name = "py_test") + if kwargs.get("deprecation"): + deprecation = kwargs.get("deprecation") + "\n\n" + deprecation + kwargs["deprecation"] = deprecation + _py_test(**kwargs) diff --git a/python/private/py_executable.bzl b/python/private/py_executable.bzl index 20af98da9d..3b063aac95 100644 --- a/python/private/py_executable.bzl +++ b/python/private/py_executable.bzl @@ -76,6 +76,7 @@ load( _py_builtins = py_internal _EXTERNAL_PATH_PREFIX = "external" _ZIP_RUNFILES_DIRECTORY_NAME = "runfiles" +_PYTHON_VERSION_FLAG = str(Label("//python/config_settings:python_version")) # Bazel 5.4 doesn't have config_common.toolchain_type _CC_TOOLCHAINS = [config_common.toolchain_type( @@ -132,16 +133,34 @@ Valid values are: target level. """, ), - # TODO(b/203567235): In Google, this attribute is deprecated, and can - # only effectively be PY3. Externally, with Bazel, this attribute has - # a separate story. "python_version": attr.string( # TODO(b/203567235): In the Java impl, the default comes from # --python_version. Not clear what the Starlark equivalent is. - default = "PY3", - # NOTE: Some tests care about the order of these values. - values = ["PY2", "PY3"], - doc = "Defunct, unused, does nothing.", + doc = """ +The Python version this target should use. + +The value should be in `X.Y` or `X.Y.Z` (or compatible) format. If empty or +unspecified, the incoming configuration's {obj}`--python_version` flag is +inherited. For backwards compatibility, the values `PY2` and `PY3` are +accepted, but treated as an empty/unspecified value. + +:::{note} +In order for the requested version to be used, there must be a +toolchain configured to match the Python version. If there isn't, then it +may be silently ignored, or an error may occur, depending on the toolchain +configuration. +::: + +:::{versionchanged} VERSION_NEXT_PATCH + +This attribute was changed from only accepting `PY2` and `PY3` values to +accepting arbitrary Python versions. +::: +""", + ), + # Required to opt-in to the transition feature. + "_allowlist_function_transition": attr.label( + default = "@bazel_tools//tools/allowlists/function_transition_allowlist", ), "_bootstrap_impl_flag": attr.label( default = "//python/config_settings:bootstrap_impl", @@ -1009,7 +1028,7 @@ def _get_build_info(ctx, cc_toolchain): return build_info_files.redacted_build_info_files.to_list() def _validate_executable(ctx): - if ctx.attr.python_version != "PY3": + if ctx.attr.python_version == "PY2": fail("It is not allowed to use Python 2") def _declare_executable_file(ctx): @@ -1696,9 +1715,26 @@ def _create_run_environment_info(ctx, inherited_environment): inherited_environment = inherited_environment, ) +def _transition_executable_impl(input_settings, attr): + settings = { + _PYTHON_VERSION_FLAG: input_settings[_PYTHON_VERSION_FLAG], + } + if attr.python_version and attr.python_version not in ("PY2", "PY3"): + settings[_PYTHON_VERSION_FLAG] = attr.python_version + return settings + +_transition_executable = transition( + implementation = _transition_executable_impl, + inputs = [ + _PYTHON_VERSION_FLAG, + ], + outputs = [ + _PYTHON_VERSION_FLAG, + ], +) + def create_executable_rule(*, attrs, **kwargs): return create_base_executable_rule( - ##attrs = dicts.add(EXECUTABLE_ATTRS, attrs), attrs = attrs, fragments = ["py", "bazel_py"], **kwargs @@ -1720,6 +1756,7 @@ def create_base_executable_rule(*, attrs, fragments = [], **kwargs): fragments = fragments + ["py"] kwargs.setdefault("provides", []).append(PyExecutableInfo) kwargs["exec_groups"] = REQUIRED_EXEC_GROUPS | (kwargs.get("exec_groups") or {}) + kwargs.setdefault("cfg", _transition_executable) return rule( # TODO: add ability to remove attrs, i.e. for imports attr attrs = dicts.add(EXECUTABLE_ATTRS, attrs), diff --git a/tests/config_settings/transition/multi_version_tests.bzl b/tests/config_settings/transition/multi_version_tests.bzl index 5805bab32d..50b4402fce 100644 --- a/tests/config_settings/transition/multi_version_tests.bzl +++ b/tests/config_settings/transition/multi_version_tests.bzl @@ -109,8 +109,7 @@ def _test_py_binary_windows_build_python_zip_false_impl(env, target): # have the "_" prefix on them (those are coming from the underlying # wrapped binary). env.expect.that_target(target).default_outputs().contains_exactly([ - "{package}/_{test_name}_subject", - "{package}/_{test_name}_subject.exe", + "{package}/{test_name}_subject.exe", "{package}/{test_name}_subject", "{package}/{test_name}_subject.py", ]) @@ -136,8 +135,7 @@ def _test_py_binary_windows_build_python_zip_true_impl(env, target): # have the "_" prefix on them (those are coming from the underlying # wrapped binary). default_outputs.contains_exactly([ - "{package}/_{test_name}_subject.exe", - "{package}/_{test_name}_subject.zip", + "{package}/{test_name}_subject.exe", "{package}/{test_name}_subject.py", "{package}/{test_name}_subject.zip", ])