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 get falsely evaluated against incompatible targets #15271

Closed
philsc opened this issue Apr 16, 2022 · 2 comments
Closed

Aspects get falsely evaluated against incompatible targets #15271

philsc opened this issue Apr 16, 2022 · 2 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions

Comments

@philsc
Copy link
Contributor

philsc commented Apr 16, 2022

Description of the bug:

It looks like aspects run on incompatible targets when they shouldn't.

I think the answer here is that the aspects should not run and instead just provide the IncompatiblePlatformProvider to signify that it's incompatible as well.

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

The absolute minimal example:
example.bzl:

def _custom_rule_impl(ctx):
    pass

custom_rule = rule(
    implementation = _custom_rule_impl,
)

def _example_aspect_impl(target, ctx):
    print("Running aspect on " + str(target))
    return []

_example_aspect = aspect(
    implementation = _example_aspect_impl,
    attr_aspects = ["dep"],
)

def _basic_rule_with_aspect_impl(ctx):
    pass

basic_rule_with_aspect = rule(
    implementation = _basic_rule_with_aspect_impl,
    attrs = {
        "dep": attr.label(
            aspects = [_example_aspect],
        ),
    },
)

BUILD:

load(":example.bzl", "basic_rule_with_aspect", "custom_rule")

custom_rule(
    name = "target1",
    target_compatible_with = ["@platforms//:incompatible"],
)

basic_rule_with_aspect(
    name = "target2",
    dep = ":target1",
)

When you build this target, you'll see that the print() statement in the aspect gets executed:

$ bazel build //:target2
DEBUG: /home/pschrader/work/test_repo/example.bzl:9:10: Running aspect on <target //:target1>
ERROR: Target //:target2 is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    //:target2
    //:target1   <-- target platform (@local_config_platform//:host) didn't satisfy constraint @platforms//:incompatible
INFO: Elapsed time: 0.070s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded, 2 targets configured)

This can lead to undesirable effects. Largely because the aspect evaluation can generate errors before bazel gets a chance to tell the user about the incompatible target being requested.

Which operating system are you running Bazel on?

Ubuntu 18.04

What is the output of bazel info release?

release 5.1.0

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

N/A

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

N/A

Have you found anything relevant by searching the web?

No response

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

No response

@philsc
Copy link
Contributor Author

philsc commented Apr 16, 2022

@gregestren , FYI.

I'll work on this once the robotics season is over at the end of this month and after I finish with #14096.

@sgowroji sgowroji added type: bug team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels Apr 18, 2022
@gregestren
Copy link
Contributor

Makes total sense, and I agree. Thanks for the report and commitment, and good luck with robotics!

@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged type: bug labels Apr 18, 2022
philsc added a commit to philsc/bazel that referenced this issue Jul 26, 2022
This patch prevents aspect functions from being evaluated when their
associated targets are incompatible. Otherwise aspects can generate
errors that should never be printed at all. For example, an aspect
evaluating an incompatible target may generate errors about missing
providers. This is not the desired behaviour.

The implementation in this patch short-circuits the `AspectValue`
creation if the associated target is incompatible.

I had tried to add an `IncompatiblePlatformProvider` to the
corresponding `ConfiguredAspect` instance, but then Bazel
complained about having duplicate `IncompatiblePlatformProvider`
instances.

Fixes  bazelbuild#15271

(cherry picked from commit 6d71850)
ShreeM01 added a commit that referenced this issue Jul 27, 2022
* Prevent aspects from executing on incompatible targets

This patch prevents aspect functions from being evaluated when their
associated targets are incompatible. Otherwise aspects can generate
errors that should never be printed at all. For example, an aspect
evaluating an incompatible target may generate errors about missing
providers. This is not the desired behaviour.

The implementation in this patch short-circuits the `AspectValue`
creation if the associated target is incompatible.

I had tried to add an `IncompatiblePlatformProvider` to the
corresponding `ConfiguredAspect` instance, but then Bazel
complained about having duplicate `IncompatiblePlatformProvider`
instances.

Fixes  #15271

(cherry picked from commit 6d71850)

* Fix build and test

Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
dfinity-bot pushed a commit to dfinity/ic that referenced this issue Sep 1, 2022
Upgrade bazel to 5.3.0, bazelisk to 1.12.2

This release contains the fix for
bazelbuild/bazel#15271 and therefore fixes
`bazel build //...` on Mac OS.

bazel 5.3.0 release notes: https://blog.bazel.build/2022/08/23/bazel-5.3.html 

See merge request dfinity-lab/public/ic!7362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants