From b9718f53d376a0926ef001b2d98b99ffb5617842 Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Tue, 21 Nov 2023 12:32:36 -0800 Subject: [PATCH] Use rules_python implemented py_runtime --- python/private/common/providers.bzl | 7 +- python/private/common/py_runtime_rule.bzl | 26 ++--- python/private/reexports.bzl | 2 +- python/private/util.bzl | 6 ++ python/py_runtime.bzl | 5 +- python/py_runtime_info.bzl | 6 +- python/py_runtime_pair.bzl | 4 +- tests/base_rules/util.bzl | 5 +- tests/py_runtime/py_runtime_tests.bzl | 121 +++++++++++++++++----- 9 files changed, 124 insertions(+), 58 deletions(-) diff --git a/python/private/common/providers.bzl b/python/private/common/providers.bzl index e00eb86d19..38a7054602 100644 --- a/python/private/common/providers.bzl +++ b/python/private/common/providers.bzl @@ -13,14 +13,15 @@ # limitations under the License. """Providers for Python rules.""" -load("@rules_python_internal//:rules_python_config.bzl", "config") +load("//python/private:util.bzl", "IS_BAZEL_6_OR_HIGHER") # TODO: load CcInfo from rules_cc _CcInfo = CcInfo DEFAULT_STUB_SHEBANG = "#!/usr/bin/env python3" -DEFAULT_BOOTSTRAP_TEMPLATE = "@bazel_tools//tools/python:python_bootstrap_template.txt" +DEFAULT_BOOTSTRAP_TEMPLATE = Label("//python/private:python_bootstrap_template.txt") + _PYTHON_VERSION_VALUES = ["PY2", "PY3"] # Helper to make the provider definitions not crash under Bazel 5.4: @@ -31,7 +32,7 @@ _PYTHON_VERSION_VALUES = ["PY2", "PY3"] # This isn't actually used under Bazel 5.4, so just stub out the values # to get past the loading phase. def _define_provider(doc, fields, **kwargs): - if not config.enable_pystar: + if not IS_BAZEL_6_OR_HIGHER: return provider("Stub, not used", fields = []), None return provider(doc = doc, fields = fields, **kwargs) diff --git a/python/private/common/py_runtime_rule.bzl b/python/private/common/py_runtime_rule.bzl index 39434042ea..97d3dad692 100644 --- a/python/private/common/py_runtime_rule.bzl +++ b/python/private/common/py_runtime_rule.bzl @@ -16,14 +16,12 @@ load("@bazel_skylib//lib:dicts.bzl", "dicts") load("@bazel_skylib//lib:paths.bzl", "paths") load(":attributes.bzl", "NATIVE_RULES_ALLOWLIST_ATTRS") -load(":common.bzl", "check_native_allowed") load(":providers.bzl", "DEFAULT_BOOTSTRAP_TEMPLATE", "DEFAULT_STUB_SHEBANG", _PyRuntimeInfo = "PyRuntimeInfo") load(":py_internal.bzl", "py_internal") _py_builtins = py_internal def _py_runtime_impl(ctx): - check_native_allowed(ctx) interpreter_path = ctx.attr.interpreter_path or None # Convert empty string to None interpreter = ctx.file.interpreter if (interpreter_path and interpreter) or (not interpreter_path and not interpreter): @@ -44,7 +42,8 @@ def _py_runtime_impl(ctx): if ctx.attr.coverage_tool: coverage_di = ctx.attr.coverage_tool[DefaultInfo] - if _py_builtins.is_singleton_depset(coverage_di.files): + # todo: add test for this code path + if _is_singleton_depset(coverage_di.files): coverage_tool = coverage_di.files.to_list()[0] elif coverage_di.files_to_run and coverage_di.files_to_run.executable: coverage_tool = coverage_di.files_to_run.executable @@ -60,16 +59,6 @@ def _py_runtime_impl(ctx): coverage_files = None python_version = ctx.attr.python_version - if python_version == "_INTERNAL_SENTINEL": - if ctx.fragments.py.use_toolchains: - fail( - "When using Python toolchains, this attribute must be set explicitly to either 'PY2' " + - "or 'PY3'. See https://github.com/bazelbuild/bazel/issues/7899 for more " + - "information. You can temporarily avoid this error by reverting to the legacy " + - "Python runtime mechanism (`--incompatible_use_python_toolchains=false`).", - ) - else: - python_version = ctx.fragments.py.default_python_version # TODO: Uncomment this after --incompatible_python_disable_py2 defaults to true # if ctx.fragments.py.disable_py2 and python_version == "PY2": @@ -93,6 +82,13 @@ def _py_runtime_impl(ctx): ), ] +def _is_singleton_depset(files): + # Bazel 6 doesn't have this helper to optimize detecting singleton depsets. + if _py_builtins: + return _py_builtins.is_singleton_depset(files) + else: + return len(files.to_list()) == 1 + # Bind to the name "py_runtime" to preserve the kind/rule_class it shows up # as elsewhere. py_runtime = rule( @@ -189,8 +185,8 @@ For a platform runtime, this is the absolute path of a Python interpreter on the target platform. For an in-build runtime this attribute must not be set. """), "python_version": attr.string( - default = "_INTERNAL_SENTINEL", - values = ["PY2", "PY3", "_INTERNAL_SENTINEL"], + default = "PY3", + values = ["PY2", "PY3"], doc = """ Whether this runtime is for Python major version 2 or 3. Valid values are `"PY2"` and `"PY3"`. diff --git a/python/private/reexports.bzl b/python/private/reexports.bzl index af5b394275..ea39ac9f35 100644 --- a/python/private/reexports.bzl +++ b/python/private/reexports.bzl @@ -37,4 +37,4 @@ different name. Then we can load it from elsewhere. BuiltinPyInfo = PyInfo # buildifier: disable=name-conventions -internal_PyRuntimeInfo = PyRuntimeInfo +BuiltinPyRuntimeInfo = PyRuntimeInfo diff --git a/python/private/util.bzl b/python/private/util.bzl index 6c8761d5b5..71476f9a33 100644 --- a/python/private/util.bzl +++ b/python/private/util.bzl @@ -83,3 +83,9 @@ def add_tag(attrs, tag): attrs["tags"] = tags + [tag] else: attrs["tags"] = [tag] + +IS_BAZEL_7_OR_HIGHER = hasattr(native, "starlark_doc_extract") + +# Bazel 5.4 has a bug where every access of testing.ExecutionInfo is a +# different object that isn't equal to any other. This is fixed in bazel 6+. +IS_BAZEL_6_OR_HIGHER = testing.ExecutionInfo == testing.ExecutionInfo diff --git a/python/py_runtime.bzl b/python/py_runtime.bzl index ac8b090c94..d4b913df2e 100644 --- a/python/py_runtime.bzl +++ b/python/py_runtime.bzl @@ -14,12 +14,11 @@ """Public entry point for py_runtime.""" -load("@rules_python_internal//:rules_python_config.bzl", "config") -load("//python/private:util.bzl", "add_migration_tag") +load("//python/private:util.bzl", "IS_BAZEL_6_OR_HIGHER", "add_migration_tag") load("//python/private/common:py_runtime_macro.bzl", _starlark_py_runtime = "py_runtime") # buildifier: disable=native-python -_py_runtime_impl = _starlark_py_runtime if config.enable_pystar else native.py_runtime +_py_runtime_impl = _starlark_py_runtime if IS_BAZEL_6_OR_HIGHER else native.py_runtime def py_runtime(**attrs): """See the Bazel core [py_runtime](https://docs.bazel.build/versions/master/be/python.html#py_runtime) documentation. diff --git a/python/py_runtime_info.bzl b/python/py_runtime_info.bzl index 699b31d6df..c0a9288122 100644 --- a/python/py_runtime_info.bzl +++ b/python/py_runtime_info.bzl @@ -14,8 +14,8 @@ """Public entry point for PyRuntimeInfo.""" -load("@rules_python_internal//:rules_python_config.bzl", "config") -load("//python/private:reexports.bzl", "internal_PyRuntimeInfo") +load("//python/private:reexports.bzl", "BuiltinPyRuntimeInfo") +load("//python/private:util.bzl", "IS_BAZEL_6_OR_HIGHER") load("//python/private/common:providers.bzl", _starlark_PyRuntimeInfo = "PyRuntimeInfo") -PyRuntimeInfo = _starlark_PyRuntimeInfo if config.enable_pystar else internal_PyRuntimeInfo +PyRuntimeInfo = _starlark_PyRuntimeInfo if IS_BAZEL_6_OR_HIGHER else BuiltinPyRuntimeInfo diff --git a/python/py_runtime_pair.bzl b/python/py_runtime_pair.bzl index c80994c963..30df002f13 100644 --- a/python/py_runtime_pair.bzl +++ b/python/py_runtime_pair.bzl @@ -15,10 +15,10 @@ """Public entry point for py_runtime_pair.""" load("@bazel_tools//tools/python:toolchain.bzl", _bazel_tools_impl = "py_runtime_pair") -load("@rules_python_internal//:rules_python_config.bzl", "config") load("//python/private:py_runtime_pair_macro.bzl", _starlark_impl = "py_runtime_pair") +load("//python/private:util.bzl", "IS_BAZEL_6_OR_HIGHER") -_py_runtime_pair = _bazel_tools_impl if not config.enable_pystar else _starlark_impl +_py_runtime_pair = _starlark_impl if IS_BAZEL_6_OR_HIGHER else _bazel_tools_impl # NOTE: This doc is copy/pasted from the builtin py_runtime_pair rule so our # doc generator gives useful API docs. diff --git a/tests/base_rules/util.bzl b/tests/base_rules/util.bzl index 9b386ca3bd..b9ed3ae930 100644 --- a/tests/base_rules/util.bzl +++ b/tests/base_rules/util.bzl @@ -14,6 +14,7 @@ """Helpers and utilities multiple tests re-use.""" load("@bazel_skylib//lib:structs.bzl", "structs") +load("//python/private:util.bzl", "IS_BAZEL_6_OR_HIGHER") # Use this with is_windows() WINDOWS_ATTR = {"windows": attr.label(default = "@platforms//os:windows")} @@ -53,9 +54,7 @@ def _struct_with(s, **kwargs): return struct(**struct_dict) def _is_bazel_6_or_higher(): - # Bazel 5.4 has a bug where every access of testing.ExecutionInfo is a - # different object that isn't equal to any other. This is fixed in bazel 6+. - return testing.ExecutionInfo == testing.ExecutionInfo + return IS_BAZEL_6_OR_HIGHER def _is_windows(env): """Tell if the target platform is windows. diff --git a/tests/py_runtime/py_runtime_tests.bzl b/tests/py_runtime/py_runtime_tests.bzl index 662909cca2..7f0c8ec9e5 100644 --- a/tests/py_runtime/py_runtime_tests.bzl +++ b/tests/py_runtime/py_runtime_tests.bzl @@ -29,6 +29,20 @@ _SKIP_TEST = { "target_compatible_with": ["@platforms//:incompatible"], } +def _simple_binary_impl(ctx): + output = ctx.actions.declare_file(ctx.label.name) + ctx.actions.write(output, "", is_executable = True) + return [DefaultInfo( + executable = output, + runfiles = ctx.runfiles(ctx.files.data), + )] + +_simple_binary = rule( + implementation = _simple_binary_impl, + attrs = {"data": attr.label_list(allow_files = True)}, + executable = True, +) + def _test_bootstrap_template(name): # The bootstrap_template arg isn't present in older Bazel versions, so # we have to conditionally pass the arg and mark the test incompatible. @@ -123,86 +137,137 @@ def _test_cannot_specify_files_for_system_interpreter_impl(env, target): _tests.append(_test_cannot_specify_files_for_system_interpreter) -def _test_in_build_interpreter(name): +def _test_coverage_tool_executable(name): + if br_util.is_bazel_6_or_higher(): + py_runtime_kwargs = { + "coverage_tool": name + "_coverage_tool", + } + attr_values = {} + else: + py_runtime_kwargs = {} + attr_values = _SKIP_TEST + rt_util.helper_target( py_runtime, name = name + "_subject", - interpreter = "fake_interpreter", python_version = "PY3", - files = ["file1.txt"], + interpreter_path = "/bogus", + **py_runtime_kwargs + ) + rt_util.helper_target( + _simple_binary, + name = name + "_coverage_tool", + data = ["coverage_file1.txt", "coverage_file2.txt"], ) analysis_test( name = name, target = name + "_subject", - impl = _test_in_build_interpreter_impl, + impl = _test_coverage_tool_executable_impl, + attr_values = attr_values, ) -def _test_in_build_interpreter_impl(env, target): +def _test_coverage_tool_executable_impl(env, target): info = env.expect.that_target(target).provider(PyRuntimeInfo, factory = py_runtime_info_subject) - info.python_version().equals("PY3") - info.files().contains_predicate(matching.file_basename_equals("file1.txt")) - info.interpreter().path().contains("fake_interpreter") + info.coverage_tool().short_path_equals("{package}/{test_name}_coverage_tool") + info.coverage_files().contains_exactly([ + "{package}/{test_name}_coverage_tool", + "{package}/coverage_file1.txt", + "{package}/coverage_file2.txt", + ]) -_tests.append(_test_in_build_interpreter) +_tests.append(_test_coverage_tool_executable) -def _test_must_have_either_inbuild_or_system_interpreter(name): +def _test_coverage_tool_plain_files(name): if br_util.is_bazel_6_or_higher(): - py_runtime_kwargs = {} - attr_values = {} - else: py_runtime_kwargs = { - "interpreter_path": "/some/path", + "coverage_tool": name + "_coverage_tool", } + attr_values = {} + else: + py_runtime_kwargs = {} attr_values = _SKIP_TEST rt_util.helper_target( py_runtime, name = name + "_subject", python_version = "PY3", + interpreter_path = "/bogus", **py_runtime_kwargs ) + rt_util.helper_target( + native.filegroup, + name = name + "_coverage_tool", + srcs = ["coverage_tool.py"], + data = ["coverage_file1.txt", "coverage_file2.txt"], + ) analysis_test( name = name, target = name + "_subject", - impl = _test_must_have_either_inbuild_or_system_interpreter_impl, - expect_failure = True, + impl = _test_coverage_tool_plain_files_impl, attr_values = attr_values, ) -def _test_must_have_either_inbuild_or_system_interpreter_impl(env, target): - env.expect.that_target(target).failures().contains_predicate( - matching.str_matches("one of*interpreter*interpreter_path"), +def _test_coverage_tool_plain_files_impl(env, target): + info = env.expect.that_target(target).provider(PyRuntimeInfo, factory = py_runtime_info_subject) + info.coverage_tool().short_path_equals("{package}/coverage_tool.py") + info.coverage_files().contains_exactly([ + "{package}/coverage_tool.py", + "{package}/coverage_file1.txt", + "{package}/coverage_file2.txt", + ]) + +_tests.append(_test_coverage_tool_plain_files) + +def _test_in_build_interpreter(name): + rt_util.helper_target( + py_runtime, + name = name + "_subject", + interpreter = "fake_interpreter", + python_version = "PY3", + files = ["file1.txt"], + ) + analysis_test( + name = name, + target = name + "_subject", + impl = _test_in_build_interpreter_impl, ) -_tests.append(_test_must_have_either_inbuild_or_system_interpreter) +def _test_in_build_interpreter_impl(env, target): + info = env.expect.that_target(target).provider(PyRuntimeInfo, factory = py_runtime_info_subject) + info.python_version().equals("PY3") + info.files().contains_predicate(matching.file_basename_equals("file1.txt")) + info.interpreter().path().contains("fake_interpreter") + +_tests.append(_test_in_build_interpreter) -def _test_python_version_required(name): - # Bazel 5.4 will entirely crash when python_version is missing. +def _test_must_have_either_inbuild_or_system_interpreter(name): if br_util.is_bazel_6_or_higher(): py_runtime_kwargs = {} attr_values = {} else: - py_runtime_kwargs = {"python_version": "PY3"} + py_runtime_kwargs = { + "interpreter_path": "/some/path", + } attr_values = _SKIP_TEST rt_util.helper_target( py_runtime, name = name + "_subject", - interpreter_path = "/math/pi", + python_version = "PY3", **py_runtime_kwargs ) analysis_test( name = name, target = name + "_subject", - impl = _test_python_version_required_impl, + impl = _test_must_have_either_inbuild_or_system_interpreter_impl, expect_failure = True, attr_values = attr_values, ) -def _test_python_version_required_impl(env, target): +def _test_must_have_either_inbuild_or_system_interpreter_impl(env, target): env.expect.that_target(target).failures().contains_predicate( - matching.str_matches("must be set*PY2*PY3"), + matching.str_matches("one of*interpreter*interpreter_path"), ) -_tests.append(_test_python_version_required) +_tests.append(_test_must_have_either_inbuild_or_system_interpreter) def _test_system_interpreter(name): rt_util.helper_target(