Skip to content

Prevent aspects from executing on incompatible targets#15426

Closed
philsc wants to merge 13 commits intobazelbuild:masterfrom
philsc:fix-incompatible-aspects-15271
Closed

Prevent aspects from executing on incompatible targets#15426
philsc wants to merge 13 commits intobazelbuild:masterfrom
philsc:fix-incompatible-aspects-15271

Conversation

@philsc
Copy link
Copy Markdown
Contributor

@philsc philsc commented May 9, 2022 •
edited
Loading

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

@sgowroji sgowroji added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels May 9, 2022
@philsc philsc marked this pull request as ready for review May 9, 2022 06:28
@philsc
Copy link
Copy Markdown
Contributor Author

philsc commented May 12, 2022

@gregestren , are you a good person to review this?

Happy to add more test cases too.

@gregestren
Copy link
Copy Markdown
Contributor

gregestren commented May 12, 2022

Sure, happy to review!

@gregestren gregestren self-requested a review May 12, 2022 16:37
gregestren
gregestren reviewed Jun 6, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@gregestren gregestren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me too long to get to this. I thought it was going to be a lot more code than it is. What a nice, concise surprise!

It basically looks good to me, including the test coverage. I just have some sanity check questions to make sure I get all the implications.

Basic question: as I understand aspects can automatically propagate down a target's dependencies. If an aspect hits an IncompatiblePlatformProvider target, it gets short-circuited, which is great.

Will it continue to propagate down that target's dependencies? If they're not also IncompatiblePlatformProvider targets, would that do something strange?

I think your test coverage shows the opposite? How when the bottom-most dependency is incompatible, everything depending on it also gets skipped?

What happens when that's reversed?

Comment thread src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@gregestren gregestren removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 6, 2022
@gregestren gregestren self-assigned this Jun 6, 2022
@philsc
Copy link
Copy Markdown
Contributor Author

philsc commented Jun 10, 2022

Will it continue to propagate down that target's dependencies? If they're not also IncompatiblePlatformProvider targets, would that do something strange?

That is a great question. I will add a test case for this and fix it if it behaves unexpectedly.

philsc
philsc commented Jun 11, 2022
View reviewed changes
Comment thread src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
Comment thread src/test/shell/integration/target_compatible_with_test.sh
@philsc philsc requested a review from gregestren June 13, 2022 18:08
gregestren
gregestren reviewed Jun 13, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@gregestren gregestren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other comments besides asking for a bit more line documentation in the test code.

Comment thread src/test/shell/integration/target_compatible_with_test.sh
Comment thread src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@philsc philsc requested a review from gregestren June 14, 2022 04:31
@gregestren
Copy link
Copy Markdown
Contributor

gregestren commented Jun 15, 2022

I'm running this by @sdtwigg for the internal push and he asked if this works as expected with aliases (which always seem to work differently than everything else). Was that something you clarified?

@philsc
Copy link
Copy Markdown
Contributor Author

philsc commented Jun 16, 2022

I'm running this by @sdtwigg for the internal push and he asked if this works as expected with aliases (which always seem to work differently than everything else). Was that something you clarified?

Haha, totally fair. I haven't validated that it works with aliases. I will add test cases and validate the behaviour.

@sdtwigg
Copy link
Copy Markdown
Contributor

sdtwigg commented Jun 16, 2022

I mostly just saw that you did the change right before the special casing for alias handling and was wondering what the proper order between those two special cases is.

@philsc
Copy link
Copy Markdown
Contributor Author

philsc commented Jun 19, 2022

I mostly just saw that you did the change right before the special casing for alias handling and was wondering what the proper order between those two special cases is.

It's a good question. As far as I can tell, the order of those two if statements doesn't matter. If you prefer, I can move it below the alias special casing.
I added some alias() targets to the test to make sure it all still works. I couldn't think of any corner cases to cover.

@philsc philsc requested a review from gregestren June 22, 2022 17:46
@gregestren
Copy link
Copy Markdown
Contributor

gregestren commented Jun 29, 2022

This looks good to me, still. Thank you.

Let me confer with @sdtwigg and then we'll get this imported. Only caveat: @sdtwigg will be out tomorrow through the weekend and I'll be out this Friday and next week.

If you're a bit patient we'll follow up. :)

@philsc
Copy link
Copy Markdown
Contributor Author

philsc commented Jun 30, 2022

Sounds great @gregestren, thank you!

@philsc
Copy link
Copy Markdown
Contributor Author

philsc commented Jul 18, 2022

Looks like this patch would help avoid work arounds like grailbio/bazel-compilation-database#100.

@gregestren
Copy link
Copy Markdown
Contributor

gregestren commented Jul 19, 2022

Thanks for the ping now (and I'm back online). I'll get back on this...

@philsc philsc mentioned this pull request Jul 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gregestren gregestren Awaiting requested review from gregestren

Assignees

@gregestren gregestren

Labels

team-Configurability platforms, toolchains, cquery, select(), config transitions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Aspects get falsely evaluated against incompatible targets

4 participants

@philsc @gregestren @sdtwigg @sgowroji