Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aspects with required_providers set do not run on attributes with providers set #17214

Open
UebelAndre opened this issue Jan 13, 2023 · 6 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) type: bug

Comments

@UebelAndre
Copy link
Contributor

UebelAndre commented Jan 13, 2023

Description of the bug:

A concrete example of this can be seen here:

The brief outline of the issue is if I have an aspect that sets required_providers, it seems that prevents that aspect from running on rules which apply the aspect to certain attributes.

my_aspect = aspect(
    # ....
    required_providers = [[MyProviderAInfo], [MyProviderBInfo]],
)

my_rule rule(
    # ...
    attrs = {
        "lib": attr.label(
            providers = [[MyProviderAInfo], [MyProviderBInfo]],
            aspects = [my_aspect],
        )
    }
)

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

The best way to reproduce this issue is to apply the following patch to rules_rust at bazelbuild/rules_rust@49906eb

diff --git a/rust/private/rustfmt.bzl b/rust/private/rustfmt.bzl
index 2e4794de..a4f17674 100644
--- a/rust/private/rustfmt.bzl
+++ b/rust/private/rustfmt.bzl
@@ -197,6 +197,10 @@ Output Groups:
     toolchains = [
         str(Label("//rust/rustfmt:toolchain_type")),
     ],
+    required_providers = [
+        [rust_common.crate_info],
+        [rust_common.test_crate_info],
+    ],
 )

 def _rustfmt_test_impl(ctx):
andrebrisco@Andres-MacBook-Pro rules_rust % git diff .
diff --git a/rust/private/rustfmt.bzl b/rust/private/rustfmt.bzl
index 2e4794de..ebe930bc 100644
--- a/rust/private/rustfmt.bzl
+++ b/rust/private/rustfmt.bzl
@@ -162,6 +162,7 @@ generated source files are also ignored by this aspect.
 )

 def _rustfmt_test_manifest_aspect_impl(target, ctx):
+    print("Running _rustfmt_test_manifest_aspect for", target)
     crate_info = _get_rustfmt_ready_crate_info(target)

     if not crate_info:
@@ -197,6 +198,10 @@ Output Groups:
     toolchains = [
         str(Label("//rust/rustfmt:toolchain_type")),
     ],
+    required_providers = [
+        [rust_common.crate_info],
+        [rust_common.test_crate_info],
+    ],
 )

 def _rustfmt_test_impl(ctx):

Then run

bazel test //test/rustfmt:rust_binary_unformatted_2018_test

Which operating system are you running Bazel on?

Linux, MacOS, Windows

What is the output of bazel info release?

release 6.0.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@Wyverald Wyverald added team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Jan 17, 2023
@comius
Copy link
Contributor

comius commented Feb 14, 2023

I'm not sure I completely follow this problem.

If an aspect declares required_providers, then the targets need to declare provides, or else the aspect won't visit them. This is WAI.

@comius comius added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts labels Feb 14, 2023
@UebelAndre
Copy link
Contributor Author

UebelAndre commented Feb 14, 2023

I'm not sure I completely follow this problem.

If an aspect declares required_providers, then the targets need to declare provides, or else the aspect won't visit them. This is WAI.

If I have a rule with an attribute with that specifies aspects = [my_aspect], I would expect that to run the aspect on targets passed to that attribute. But that's not happening when required_providers is set, despite the dependency returning that provider.

edit: Updated the example in the issue

@comius
Copy link
Contributor

comius commented Feb 14, 2023

The first line of the documentation of required_providers says: "This attribute allows the aspect to limit its propagation to only the targets whose rules advertise its required providers." https://bazel.build/rules/lib/globals#parameters_4

@UebelAndre
Copy link
Contributor Author

UebelAndre commented Feb 14, 2023

I would expect that to apply to the dependency target. If foo depends on bar where bar is a target that returns BarInfo, I would expect the following to trigger the the bar_aspect.

foo_rule rule(
    # ...
    attrs = {
        "bar": attr.label(
            providers = [BarInfo],
            aspects = [bar_aspect],
        )
    }
)
load(":foo.bzl", "foo_rule")

foo_rule(
    name = "foo",
    bar = "//bar",
)

Note that foo_rule does not return BarInfo but //bar does. Based on the implementation of the rule, bar_aspect should run on //bar when foo is built.

@trungdinhth
Copy link

In our project, we have a similar use case where we need to limit the aspect propagation with providers returned from the "beforehand" propagation of a required aspect (one that is put in requires). As mentioned above by @comius, the targets have to advertise providers, otherwise those providers are not taken into account during aspect propagation even if those providers are returned by other aspects running before that.

However, I did discover that if there is a "carrier" aspect propagating vastly through every targets, let's call it A. Then A has B in requires, where B is the aspect that we want to limit propagation with required_providers = [prov_x] for example. And B also requires aspect C (this can be many), which returns prov_x during its propagation and run on the targets. In that case, the limitation will work. ⚠️ I'm not sure if this is intended or just an unwanted side-effect due to the specific implementation, but it is not documented, so we need to be cautious when using it I think.

copybara-service bot pushed a commit that referenced this issue Sep 24, 2024
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 #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: #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 #20436.

PiperOrigin-RevId: 677997144
Change-Id: Id2f75db10447b334a9d5ec9872e15f490ae44927
iancha1992 pushed a commit that referenced this issue Sep 24, 2024
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 #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: #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 #20436.

PiperOrigin-RevId: 677997144
Change-Id: Id2f75db10447b334a9d5ec9872e15f490ae44927
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) type: bug
Projects
None yet
Development

No branches or pull requests

5 participants
@Wyverald @comius @UebelAndre @trungdinhth and others