From f6766565f7830ff900990d0100cec3ad54b22eaa Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Tue, 21 Nov 2023 12:55:33 -0800 Subject: [PATCH] pystar: support builtin providers for compatibility (#1573) This makes the rules_python Starlark implementation accept and return the builtin providers. This allows depending on, and being depended on by, the builtin rules, which enables the two rule sets to interoperate better. Work towards #1069 --- python/defs.bzl | 6 +-- python/private/BUILD.bazel | 3 +- python/private/common/BUILD.bazel | 2 + python/private/common/attributes.bzl | 7 ++- python/private/common/common.bzl | 28 +++++++--- python/private/common/py_executable.bzl | 3 +- python/private/common/py_library.bzl | 3 +- python/private/reexports.bzl | 23 +++----- python/py_info.bzl | 4 +- tests/base_rules/base_tests.bzl | 72 ++++++++++++++++++++----- tests/base_rules/py_info_subject.bzl | 2 +- 11 files changed, 106 insertions(+), 47 deletions(-) diff --git a/python/defs.bzl b/python/defs.bzl index 3fb6b5bb65..bd89f5b1f2 100644 --- a/python/defs.bzl +++ b/python/defs.bzl @@ -15,7 +15,7 @@ load("@bazel_tools//tools/python:srcs_version.bzl", _find_requirements = "find_requirements") load("//python:py_binary.bzl", _py_binary = "py_binary") -load("//python:py_info.bzl", internal_PyInfo = "PyInfo") +load("//python:py_info.bzl", _PyInfo = "PyInfo") load("//python:py_library.bzl", _py_library = "py_library") load("//python:py_runtime.bzl", _py_runtime = "py_runtime") load("//python:py_runtime_info.bzl", internal_PyRuntimeInfo = "PyRuntimeInfo") @@ -26,9 +26,7 @@ load(":py_import.bzl", _py_import = "py_import") # Patching placeholder: end of loads -# Exports of native-defined providers. - -PyInfo = internal_PyInfo +PyInfo = _PyInfo PyRuntimeInfo = internal_PyRuntimeInfo diff --git a/python/private/BUILD.bazel b/python/private/BUILD.bazel index d5b170e5b9..04c7af6da8 100644 --- a/python/private/BUILD.bazel +++ b/python/private/BUILD.bazel @@ -169,8 +169,7 @@ bzl_library( name = "reexports_bzl", srcs = ["reexports.bzl"], visibility = [ - "//docs:__pkg__", - "//python:__pkg__", + "//:__subpackages__", ], deps = [":bazel_tools_bzl"], ) diff --git a/python/private/common/BUILD.bazel b/python/private/common/BUILD.bazel index f20e682e26..b180e3d068 100644 --- a/python/private/common/BUILD.bazel +++ b/python/private/common/BUILD.bazel @@ -31,6 +31,7 @@ bzl_library( ":providers_bzl", ":py_internal_bzl", ":semantics_bzl", + "//python/private:reexports_bzl", ], ) @@ -59,6 +60,7 @@ bzl_library( ":providers_bzl", ":py_internal_bzl", ":semantics_bzl", + "//python/private:reexports_bzl", ], ) diff --git a/python/private/common/attributes.bzl b/python/private/common/attributes.bzl index b1c54a0973..b26d02cb39 100644 --- a/python/private/common/attributes.bzl +++ b/python/private/common/attributes.bzl @@ -13,6 +13,7 @@ # limitations under the License. """Attributes for Python rules.""" +load("//python/private:reexports.bzl", "BuiltinPyInfo") load(":common.bzl", "union_attrs") load(":providers.bzl", "PyInfo") load(":py_internal.bzl", "py_internal") @@ -127,7 +128,11 @@ COMMON_ATTRS = union_attrs( PY_SRCS_ATTRS = union_attrs( { "deps": attr.label_list( - providers = [[PyInfo], [_CcInfo]], + providers = [ + [PyInfo], + [_CcInfo], + [BuiltinPyInfo], + ], # TODO(b/228692666): Google-specific; remove these allowances once # the depot is cleaned up. allow_rules = DEPS_ATTR_ALLOW_RULES, diff --git a/python/private/common/common.bzl b/python/private/common/common.bzl index 84b2aa5388..75c117f5cd 100644 --- a/python/private/common/common.bzl +++ b/python/private/common/common.bzl @@ -13,6 +13,7 @@ # limitations under the License. """Various things common to Bazel and Google rule implementations.""" +load("//python/private:reexports.bzl", "BuiltinPyInfo") load(":cc_helper.bzl", "cc_helper") load(":providers.bzl", "PyInfo") load(":py_internal.bzl", "py_internal") @@ -265,6 +266,10 @@ def collect_imports(ctx, semantics): dep[PyInfo].imports for dep in ctx.attr.deps if PyInfo in dep + ] + [ + dep[BuiltinPyInfo].imports + for dep in ctx.attr.deps + if BuiltinPyInfo in dep ]) def collect_runfiles(ctx, files): @@ -355,8 +360,8 @@ def create_py_info(ctx, *, direct_sources, imports): transitive_sources_files = [] # list of Files for target in ctx.attr.deps: # PyInfo may not be present e.g. cc_library rules. - if PyInfo in target: - info = target[PyInfo] + if PyInfo in target or BuiltinPyInfo in target: + info = _get_py_info(target) transitive_sources_depsets.append(info.transitive_sources) uses_shared_libraries = uses_shared_libraries or info.uses_shared_libraries has_py2_only_sources = has_py2_only_sources or info.has_py2_only_sources @@ -384,8 +389,8 @@ def create_py_info(ctx, *, direct_sources, imports): for target in ctx.attr.data: # TODO(b/234730058): Remove checking for PyInfo in data once depot # cleaned up. - if PyInfo in target: - info = target[PyInfo] + if PyInfo in target or BuiltinPyInfo in target: + info = _get_py_info(target) uses_shared_libraries = info.uses_shared_libraries else: files = target.files.to_list() @@ -396,9 +401,7 @@ def create_py_info(ctx, *, direct_sources, imports): if uses_shared_libraries: break - # TODO(b/203567235): Set `uses_shared_libraries` field, though the Bazel - # docs indicate it's unused in Bazel and may be removed. - py_info = PyInfo( + py_info_kwargs = dict( transitive_sources = depset( transitive = [deps_transitive_sources, direct_sources], ), @@ -410,7 +413,16 @@ def create_py_info(ctx, *, direct_sources, imports): has_py3_only_sources = has_py3_only_sources, uses_shared_libraries = uses_shared_libraries, ) - return py_info, deps_transitive_sources + + # TODO(b/203567235): Set `uses_shared_libraries` field, though the Bazel + # docs indicate it's unused in Bazel and may be removed. + py_info = PyInfo(**py_info_kwargs) + builtin_py_info = BuiltinPyInfo(**py_info_kwargs) + + return py_info, deps_transitive_sources, builtin_py_info + +def _get_py_info(target): + return target[PyInfo] if PyInfo in target else target[BuiltinPyInfo] def create_instrumented_files_info(ctx): return _coverage_common.instrumented_files_info( diff --git a/python/private/common/py_executable.bzl b/python/private/common/py_executable.bzl index d188b3ad98..a24bad6788 100644 --- a/python/private/common/py_executable.bzl +++ b/python/private/common/py_executable.bzl @@ -765,7 +765,7 @@ def _create_providers( PyCcLinkParamsProvider(cc_info = cc_info), ) - py_info, deps_transitive_sources = create_py_info( + py_info, deps_transitive_sources, builtin_py_info = create_py_info( ctx, direct_sources = depset(direct_sources), imports = imports, @@ -780,6 +780,7 @@ def _create_providers( ) providers.append(py_info) + providers.append(builtin_py_info) providers.append(create_output_group_info(py_info.transitive_sources, output_groups)) extra_legacy_providers, extra_providers = semantics.get_extra_providers( diff --git a/python/private/common/py_library.bzl b/python/private/common/py_library.bzl index 8d09c51092..28ee7bf4b6 100644 --- a/python/private/common/py_library.bzl +++ b/python/private/common/py_library.bzl @@ -61,7 +61,7 @@ def py_library_impl(ctx, *, semantics): runfiles = collect_runfiles(ctx = ctx, files = output_sources) cc_info = semantics.get_cc_info_for_library(ctx) - py_info, deps_transitive_sources = create_py_info( + py_info, deps_transitive_sources, builtins_py_info = create_py_info( ctx, direct_sources = depset(direct_sources), imports = collect_imports(ctx, semantics), @@ -78,6 +78,7 @@ def py_library_impl(ctx, *, semantics): return [ DefaultInfo(files = output_sources, runfiles = runfiles), py_info, + builtins_py_info, create_instrumented_files_info(ctx), PyCcLinkParamsProvider(cc_info = cc_info), create_output_group_info(py_info.transitive_sources, extra_groups = {}), diff --git a/python/private/reexports.bzl b/python/private/reexports.bzl index a300a20365..af5b394275 100644 --- a/python/private/reexports.bzl +++ b/python/private/reexports.bzl @@ -12,20 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Internal re-exports of built-in symbols. +"""Internal re-exports of builtin symbols. -Currently the definitions here are re-exports of the native rules, "blessed" to -work under `--incompatible_load_python_rules_from_bzl`. As the native rules get -migrated to Starlark, their implementations will be removed from here. +We want to use both the PyInfo defined by builtins and the one defined by +rules_python. Because the builtin symbol is going away, the rules_python +PyInfo symbol is given preference. Unfortunately, that masks the builtin, +so we have to rebind it to another name and load it to make it available again. -We want to re-export a built-in symbol as if it were defined in a Starlark -file, so that users can for instance do: - -``` -load("@rules_python//python:defs.bzl", "PyInfo") -``` - -Unfortunately, we can't just write in defs.bzl +Unfortunately, we can't just write: ``` PyInfo = PyInfo @@ -33,15 +27,14 @@ PyInfo = PyInfo because the declaration of module-level symbol `PyInfo` makes the builtin inaccessible. So instead we access the builtin here and export it under a -different name. Then we can load it from defs.bzl and export it there under -the original name. +different name. Then we can load it from elsewhere. """ # Don't use underscore prefix, since that would make the symbol local to this # file only. Use a non-conventional name to emphasize that this is not a public # symbol. # buildifier: disable=name-conventions -internal_PyInfo = PyInfo +BuiltinPyInfo = PyInfo # buildifier: disable=name-conventions internal_PyRuntimeInfo = PyRuntimeInfo diff --git a/python/py_info.bzl b/python/py_info.bzl index cbf145d07d..0af35ac320 100644 --- a/python/py_info.bzl +++ b/python/py_info.bzl @@ -15,7 +15,7 @@ """Public entry point for PyInfo.""" load("@rules_python_internal//:rules_python_config.bzl", "config") -load("//python/private:reexports.bzl", "internal_PyInfo") +load("//python/private:reexports.bzl", "BuiltinPyInfo") load("//python/private/common:providers.bzl", _starlark_PyInfo = "PyInfo") -PyInfo = _starlark_PyInfo if config.enable_pystar else internal_PyInfo +PyInfo = _starlark_PyInfo if config.enable_pystar else BuiltinPyInfo diff --git a/tests/base_rules/base_tests.bzl b/tests/base_rules/base_tests.bzl index 99a35f9e5e..fb95c15017 100644 --- a/tests/base_rules/base_tests.bzl +++ b/tests/base_rules/base_tests.bzl @@ -17,17 +17,37 @@ load("@rules_testing//lib:analysis_test.bzl", "analysis_test") load("@rules_testing//lib:truth.bzl", "matching") load("@rules_testing//lib:util.bzl", "PREVENT_IMPLICIT_BUILDING_TAGS", rt_util = "util") load("//python:defs.bzl", "PyInfo") +load("//python/private:reexports.bzl", "BuiltinPyInfo") # buildifier: disable=bzl-visibility load("//tests/base_rules:py_info_subject.bzl", "py_info_subject") load("//tests/base_rules:util.bzl", pt_util = "util") _tests = [] +_PRODUCES_PY_INFO_ATTRS = { + "imports": attr.string_list(), + "srcs": attr.label_list(allow_files = True), +} + +def _create_py_info(ctx, provider_type): + return [provider_type( + transitive_sources = depset(ctx.files.srcs), + imports = depset(ctx.attr.imports), + )] + +def _produces_builtin_py_info_impl(ctx): + return _create_py_info(ctx, BuiltinPyInfo) + +_produces_builtin_py_info = rule( + implementation = _produces_builtin_py_info_impl, + attrs = _PRODUCES_PY_INFO_ATTRS, +) + def _produces_py_info_impl(ctx): - return [PyInfo(transitive_sources = depset(ctx.files.srcs))] + return _create_py_info(ctx, BuiltinPyInfo) _produces_py_info = rule( implementation = _produces_py_info_impl, - attrs = {"srcs": attr.label_list(allow_files = True)}, + attrs = _PRODUCES_PY_INFO_ATTRS, ) def _not_produces_py_info_impl(ctx): @@ -38,30 +58,58 @@ _not_produces_py_info = rule( implementation = _not_produces_py_info_impl, ) -def _test_consumes_provider(name, config): +def _py_info_propagation_setup(name, config, produce_py_info_rule, test_impl): rt_util.helper_target( config.base_test_rule, name = name + "_subject", - deps = [name + "_produces_py_info"], + deps = [name + "_produces_builtin_py_info"], ) rt_util.helper_target( - _produces_py_info, - name = name + "_produces_py_info", + produce_py_info_rule, + name = name + "_produces_builtin_py_info", srcs = [rt_util.empty_file(name + "_produce.py")], + imports = ["custom-import"], ) analysis_test( name = name, target = name + "_subject", - impl = _test_consumes_provider_impl, + impl = test_impl, ) -def _test_consumes_provider_impl(env, target): - env.expect.that_target(target).provider( - PyInfo, +def _py_info_propagation_test_impl(env, target, provider_type): + info = env.expect.that_target(target).provider( + provider_type, factory = py_info_subject, - ).transitive_sources().contains("{package}/{test_name}_produce.py") + ) + + info.transitive_sources().contains("{package}/{test_name}_produce.py") + info.imports().contains("custom-import") + +def _test_py_info_propagation_builtin(name, config): + _py_info_propagation_setup( + name, + config, + _produces_builtin_py_info, + _test_py_info_propagation_builtin_impl, + ) + +def _test_py_info_propagation_builtin_impl(env, target): + _py_info_propagation_test_impl(env, target, BuiltinPyInfo) + +_tests.append(_test_py_info_propagation_builtin) + +def _test_py_info_propagation(name, config): + _py_info_propagation_setup( + name, + config, + _produces_py_info, + _test_py_info_propagation_impl, + ) + +def _test_py_info_propagation_impl(env, target): + _py_info_propagation_test_impl(env, target, PyInfo) -_tests.append(_test_consumes_provider) +_tests.append(_test_py_info_propagation) def _test_requires_provider(name, config): rt_util.helper_target( diff --git a/tests/base_rules/py_info_subject.bzl b/tests/base_rules/py_info_subject.bzl index 20185e55e4..b23308c388 100644 --- a/tests/base_rules/py_info_subject.bzl +++ b/tests/base_rules/py_info_subject.bzl @@ -70,7 +70,7 @@ def _py_info_subject_imports(self): Method: PyInfoSubject.imports """ return subjects.collection( - self.actual.imports, + self.actual.imports.to_list(), meta = self.meta.derive("imports()"), )