bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23.29k stars 4.09k forks source link

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

Open UebelAndre opened 1 year ago

UebelAndre commented 1 year ago

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 https://github.com/bazelbuild/rules_rust/commit/49906eb29eb41475242138fca3eb0b1ee89ddc98

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

comius commented 1 year ago

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.

UebelAndre commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. :warning: 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.

github-actions[bot] commented 1 month ago

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.