Allow to filter lists of targets by the platform compatibility.

[konste]

Description of the problem / feature request:

Newly introduced feature allows filtering the requested targets and gracefully removes those incompatible with the requested platform. All targets which depend on incompatible targets also considered incompatible and excluded from the build.
In case we have filegroup or another rule which accepts the list of targets we don't want to invalidate it only because one of the targets in the list appeared to be incompatible. We want to skip incompatible targets, but let the rest of the list proceed with the build.

Feature requests: what underlying problem are you trying to solve with this feature?

To build specific category of artifacts (like "binaries") we have something like the following in our root BUILD file:

filegroup(
    name = "binaries",
    srcs = [
            "@lego//:binaries",
            "@atrclient//:binaries",
            "@tablicense//:binaries",
            "@art-cpp//:binaries",
    ],
)

Each item in the list is defined in a corresponding external workspace as a filegroup which may pull in direct targets and more transitive filegroups. The problem is that not all targets are suitable for all possible target platforms although each target knows what platforms it is compatible with.
target_compatible_with attribute does not solve this problem completely as any incompatible target would invalidate the whole filegroup, so we need some mechanism to allow for the filtering based on that attribute.

What operating system are you running Bazel on?

Windows, Mac, Linux

3.7.0

[philsc]

I see a few options here:

1. Create a native function that filters for you.

   filegroup(
       name = "binaries",
       srcs = filter_incompatible([
               "@lego//:binaries",
               "@atrclient//:binaries",
               "@tablicense//:binaries",
               "@art-cpp//:binaries",
       ]),
   )

2. Amend attr.label() and attr.label_list() to allow people to create an auto-filtering filegroup alternative.

   # Bazel automatically filters out targets passed to "srcs" before Starlark executes.
   auto_filtering_filegroup = rule(
       implementation = ...,
       attrs = {
           "srcs": attr.label_list(filter_incompatible_targets = True, ...),
       },
   )

3. Amend attr.label() and attr.label_list() to allow Starlark rules to access the incompatible dependencies. That way they can implement their own logic to filter out targets based on the IncompatiblePlatformProvider.

   # The Starlark implementation filters out targets passed to "srcs".
   auto_filtering_filegroup = rule(
       implementation = ...,
       attrs = {
           "srcs": attr.label_list(allow_incompatible_targets = True, ...),
       },
   )

@konste , can you confirm that the three solutions above would address your concern? I.e. did I misunderstand what you're looking for?

@gregestren , @katre, what are your thoughts on this? Do you see other/better options?

[konste]

I can confirm that we are on the same page and any of the three solutions above would solve the problem at hand.
Solution #1 looks like the easiest to implement and the least impactful one. On the other hand, it is also the most limited.

It took me some effort to figure the difference between solutions #2 and #3. From what I can see #3 is more flexible and future proof. At the same time, it seems to be less work than #2.

Considering the above solution #3 seems to be the best in the longer term, although if there are any unforeseen problems with it we can fall back to #1 as a stop-gap.

Thank you for looking into it!

[gregestren]

I like the idea of 3. We already have an IncompatiblePlatformProvider, so rather than adding more hard-coded logic of what to do with that (that won't suit all needs), just let users do whatever they want with it in Starlark.

Aside from the incompatibility issue, do you you have any attachment to filegroup specifically? Would your own custom Starlark rule provide equivalent outputs, filesToBuild, runfiles, and that sort of thing?

[moroten]

With 3, would it be possible for a rule to return IncompatiblePlatformProvider? That way, the incompatibility can be dynamically calculated depending on the inputs, not just stated using select cases in BUILD files.

[konste]

@gregestren in this particular case we use filegroup as a "funnel" to gather the targets we want for the given configuration and don't use any filegroup specific features, so it would not be a problem to replace it with the custom rule which does the filtering based on the srcs providers.

@moroten in our case "funnel" rules themselves are always compatible, so we would not need the ability to return IncompatiblePlatformProvider, but when you mentioned it - it looks like natural thing to do.

[philsc]

I have a proof-of-concept up here:
https://github.com/philsc/bazel/blob/IncompatiblePlatformProvider-in-starlark/src/test/shell/integration/target_compatible_with_test.sh#L193-L264

That patch lets you filter targets with IncompatiblePlatformProviders in Starlark. It's a bit hacky, but I think it should work.
It does not let you construct IncompatiblePlatformProvider in Starlark yet.

[gregestren]

Another API variation could be to omit the allow_incompatible_targets parameter on attr and parameterize IncompatiblePlatformProvider with a boolean.

So any target with IncompatiblePlatformProvider(value=True) is considered incompatible, any with IncompatiblePlatformProvider(value=False) is considered compatible, and any without the provider is considered compatible.

That would give Starlark rules 100% flexibility in determining what makes then incompatible or not (similar to what I think @moroten is suggesting?) and not require more API surface area on attr.

[philsc]

@gregestren , could you elaborate a little bit on how a Starlark rule would access incompatible dependencies with your proposed change?
Right now the challenge is that your Starlark code won't even get executed when it depends on incompatible targets. We need a way to tell bazel to still execute the Starlark even if it depends on incompatible targets. I'm having trouble figuring out how adding a boolean parameter to IncompatiblePlatformProvider helps bazel determine that.

[konste]

@philwo beat me to the punch. I have the same question.

I also noticed that you use this construct: incompatible_provider = src[platform_common.IncompatiblePlatformProvider] and the part with platform_common confuses me. I never saw it used as a prefix to the provider name. Could you please elaborate a little about it?

[gregestren]

@philsc you're right! I didn't think of that.

[philsc]

@gregestren , would the next step here be to create a proposal?

[gregestren]

Start up a doc at https://www.bazel.build/designs/index.html (maybe with @konste as co-author if interested?) Try to summarize the concerns expressed here and suggest what inspires you!

If this involves a change to the Starlark API, which it sounds like it will whatever the exact proposal, just note that's going to require its own review consideration. i.e. we need to clearly argue how this would grow the API in a coherent, non-cluttered, maintainable way.

[gregestren]

Setting as P1 since @philsc is actively paying attention to this now. Please feel free to adjust priority any time if this is inaccurate.

[philsc]

Sounds good, thanks @gregestren!

[konste]

I will gladly help with reviewing the document, testing the prototypes and anything I can technically handle.

[philsc]

Cool, thanks! I started a doc here: https://docs.google.com/document/d/1c1wnev-8mBHdOGQuom9hdXhfuBPlT9O8wplY-HuvXZc/edit?usp=sharing

I haven't actually put in any useful information yet.

[philsc]

@konste , could you please take a look at the doc when you get a chance? You should have full edit access.
I'd love to hear what you're thinking.

[konste]

Thanks @philsc ! I will try to get to it soon.

[konste]

@philsc I see you requested 84fadcf for cherry-pick into 4.0 and wonder if it is related to this issue?

[philsc]

That patch is unrelated to this issue. That patch is to fix an issue where creating a filegroup with incompatible dependencies would cause FailAction actions to get executed instead of being skipped.

[konste]

@philsc @gregestren I just published my attempt at the alternative design here. Hoping to make it simpler, less invasive and with that hopefully implemented sooner.

[AustinSchuh]

@konste , mind making the doc viewable publicly?

[konste]

@AustinSchuh, of course! Please try now. I am new to Google Docs and did not know how to do it initially.

[philsc]

Apologies @konste . I've been in full vacation mode for the past few weeks. I'll take a look soon.

[konste]

@philsc I wonder if the feature you presumably working on would help to resolve this issue?

[philsc]

I'm not working on any filtering features. I'm not seeing how filtering would help with the linked issue. It sounds to me like that linked issue wants bazel to skip an explicitly requested target. That seems very different from automatically filtering dependencies.