Skip to content

Commit

Permalink
python: Add provides = [PyInfo] to py_library/py_binary/py_test
Browse files Browse the repository at this point in the history
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 bazelbuild#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: bazelbuild#17214

<details>
<summary>Reproduction example to show the problem with py targets</summary>

```
#!/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
```
</details>

Closes bazelbuild#20436.

PiperOrigin-RevId: 677997144
Change-Id: Id2f75db10447b334a9d5ec9872e15f490ae44927
  • Loading branch information
stagnation authored and copybara-github committed Sep 24, 2024
1 parent e53831f commit 910e576
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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
)

Expand Down
7 changes: 6 additions & 1 deletion src/main/starlark/builtins_bzl/common/python/py_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
)

0 comments on commit 910e576

Please sign in to comment.