From 910e5769224b5d06392eec1abd684612642ca4aa Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Mon, 23 Sep 2024 17:10:29 -0700 Subject: [PATCH] python: Add `provides = [PyInfo]` to py_library/py_binary/py_test This is required for aspects to propagate along python libraries (and binaries) with `required_providers` set. Otherwise the aspect terminates on the top-level target. This follows from/was inspired by https://github.com/bazelbuild/bazel/issues/19609 where the `CcInfo` was added to `cc_binary` targets. The `cc_library` does provide `CcInfo` so an aspect can successfully propagate the `cc_library` tree. Also related: https://github.com/bazelbuild/bazel/issues/17214
Reproduction example to show the problem with py targets ``` #!/bin/sh set -eu dir=${1:-$(mktemp -d)} # "$dir"/BUILD.bazel {{{ cat > "$dir"/BUILD.bazel <<'EOF' # stack a high dependency tree py_binary( name = "base_py", srcs = ["base.py"], main = "base.py", ) py_library( name = "stack_0_py", srcs = ["extra.py"], deps = ["//:base_py"], ) [ py_library( name = "stack_{}_py".format(index), srcs = ["extra.py"], deps = [":stack_{}_py".format(index - 1)], ) for index in range(1, 2) ] cc_library( name = "base_cc", srcs = ["base.c"], ) cc_library( name = "stack_0_cc", srcs = ["extra.c"], deps = ["//:base_cc"], ) [ cc_library( name = "stack_{}_cc".format(index), srcs = ["extra.c"], deps = [":stack_{}_cc".format(index - 1)], ) for index in range(1, 2) ] EOF # }}} # "$dir"/file_count.bzl {{{ cat > "$dir"/file_count.bzl <<'EOF' FileCountInfo = provider( 'count', fields = { 'count' : 'number of files' } ) def _file_count_aspect_impl(_, ctx): name = ctx.rule.attr.name count = 0 # Make sure the rule has a srcs attribute. if hasattr(ctx.rule.attr, 'srcs'): # Iterate through the sources counting files for src in ctx.rule.attr.srcs: for _ in src.files.to_list(): count = count + 1 # Get the counts from our dependencies. for dep in ctx.rule.attr.deps: if FileCountInfo in dep: count = count + dep[FileCountInfo].count print(name, count) return [FileCountInfo(count = count)] ok = aspect( implementation = _file_count_aspect_impl, attr_aspects = ['deps'], ) required = aspect( implementation = _file_count_aspect_impl, required_providers = [ [PyInfo], [CcInfo], ], attr_aspects = ['deps'], ) EOF # }}} touch "$dir"/WORKSPACE touch "$dir"/base.py touch "$dir"/extra.py touch "$dir"/base.c touch "$dir"/extra.c echo Reproducing in "$dir" cd "$dir" || exit 1 bazelisk --version echo "[Python] First print the succesfull traversal with transient target, and accumulating count" ( set -x bazelisk build \ --ui_event_filters=-info --noshow_progress --noshow_loading_progress \ --show_result=0 \ --aspects=//:file_count.bzl%ok //:stack_1_py set +x ) echo "[Python] Now add 'required_providers', and the transitive dependencies disappear." ( set -x bazelisk build \ --ui_event_filters=-info --noshow_progress --noshow_loading_progress \ --show_result=0 \ --aspects=//:file_count.bzl%required //:stack_1_py set +x ) echo "[CC] Both works for cc_library" ( set -x bazelisk build \ --ui_event_filters=-info --noshow_progress --noshow_loading_progress \ --show_result=0 \ --aspects=//:file_count.bzl%ok //:stack_1_cc bazelisk build \ --ui_event_filters=-info --noshow_progress --noshow_loading_progress \ --show_result=0 \ --aspects=//:file_count.bzl%required //:stack_1_cc set +x ) ``` Output: ``` $ env USE_BAZEL_VERSION=7.0.0rc5 sh reproduction-aspect-required-provider. sh aspect-required-provider/ Reproducing in aspect-required-provider/ bazel 7.0.0rc5 [Python] First print the succesfull traversal with transient target, and accumulating count + bazelisk build --ui_event_filters=-info --noshow_progress --noshow_loading_progress --aspects=//:file_count.bzl%ok //:stack_1_py Starting local Bazel server and connecting to it... DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: base_py 1 DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: stack_0_py 2 DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: stack_1_py 3 + set +x [Python] Now add 'required_providers', and the transitive dependencies disappear. + bazelisk build --ui_event_filters=-info --noshow_progress --noshow_loading_progress --aspects=//:file_count.bzl%required //:stack_1_py DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: stack_1_py 1 + set +x [CC] Both works for cc_library + bazelisk build --ui_event_filters=-info --noshow_progress --noshow_loading_progress --aspects=//:file_count.bzl%ok //:stack_1_cc DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: base_cc 1 DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: stack_0_cc 2 DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: stack_1_cc 3 + bazelisk build --ui_event_filters=-info --noshow_progress --noshow_loading_progress --aspects=//:file_count.bzl%required //:stack_1_cc DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: base_cc 1 DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: stack_0_cc 2 DEBUG: /home/nils/task/reproductions/aspect-required-provider/file_count.bzl:22:10: stack_1_cc 3 ```
Closes #20436. PiperOrigin-RevId: 677997144 Change-Id: Id2f75db10447b334a9d5ec9872e15f490ae44927 --- .../builtins_bzl/common/python/py_executable_bazel.bzl | 7 ++++++- .../starlark/builtins_bzl/common/python/py_library.bzl | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/main/starlark/builtins_bzl/common/python/py_executable_bazel.bzl b/src/main/starlark/builtins_bzl/common/python/py_executable_bazel.bzl index 5a9a22cdae4896..2c57667efd7d81 100644 --- a/src/main/starlark/builtins_bzl/common/python/py_executable_bazel.bzl +++ b/src/main/starlark/builtins_bzl/common/python/py_executable_bazel.bzl @@ -23,7 +23,11 @@ load( "union_attrs", ) load(":common/python/common_bazel.bzl", "collect_cc_info", "get_imports", "maybe_precompile") -load(":common/python/providers.bzl", "DEFAULT_STUB_SHEBANG") +load( + ":common/python/providers.bzl", + "DEFAULT_STUB_SHEBANG", + "PyInfo", +) load( ":common/python/py_executable.bzl", "create_base_executable_rule", @@ -89,6 +93,7 @@ def create_executable_rule(*, attrs, **kwargs): return create_base_executable_rule( attrs = BAZEL_EXECUTABLE_ATTRS | attrs, fragments = ["py", "bazel_py"], + provides = [PyInfo], **kwargs ) diff --git a/src/main/starlark/builtins_bzl/common/python/py_library.bzl b/src/main/starlark/builtins_bzl/common/python/py_library.bzl index 1d071c37b6c100..bf2c1257fc39eb 100644 --- a/src/main/starlark/builtins_bzl/common/python/py_library.bzl +++ b/src/main/starlark/builtins_bzl/common/python/py_library.bzl @@ -35,7 +35,11 @@ load( "filter_to_py_srcs", "union_attrs", ) -load(":common/python/providers.bzl", "PyCcLinkParamsProvider") +load( + ":common/python/providers.bzl", + "PyCcLinkParamsProvider", + "PyInfo", +) _py_builtins = _builtins.internal.py_builtins @@ -106,6 +110,7 @@ def create_py_library_rule(*, attrs = {}, **kwargs): attrs = create_library_attrs() | attrs, # TODO(b/253818097): fragments=py is only necessary so that # RequiredConfigFragmentsTest passes + provides = [PyInfo], fragments = ["py"], **kwargs )