Prevent aspects from executing on incompatible targets

[philsc]

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

[philsc]

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

Happy to add more test cases too.

[gregestren]

Sure, happy to review!

[gregestren]

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?

[philsc]

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.

[gregestren]

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

[gregestren]

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]

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]

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]

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.

[gregestren]

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]

Sounds great @gregestren, thank you!

[philsc]

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

[gregestren]

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