Gracefully filter targets not compatible with the current Platform

[mjs-sx]

There was a discussion recently on bazel-discuss about selectively building targets based on a platform: https://groups.google.com/forum/#!topic/bazel-discuss/U6sFbPWiXGM

Greg Estren suggested:

Ultimately we should integrate this with Bazel's upcoming platform semantics (which builds on the concepts above) to provide as much granularity and flexibility as you want.

This is a request to do exactly that. I'd like to be able to say "build for this platform, but gracefully exclude targets that aren't compatible" (where gracefully means not failing the build). Perhaps a --filter_unsupported_platforms flag? This would let a CI script do something conceptually like for platform in [list of supported platforms]; bazel build --platform=$platform --filter_unsupported_platforms //...; done, while maintaining targets in the repository that aren't compatible with all platforms.

A suggested alternative in that thread is to use --build_tag_filters/--test_tag_filters. I've found this to be clunky because those flags don't accumulate when passed multiple times - if you have configurations in tools/bazel.rc that set these flags, you can't subsequently add another copy of --{build/test}_tag_filters on the command-line, or the value from bazel.rc will be overwritten.

[damienmg]

(definitely +1 for the bazel perspective :))

[gregestren]

I've had a few detailed email conversations with @katre about this, to the point where I'm interested in speccing up a brainstorm proposal of what it might look like.

With Bazel's platform integration steadily humming along, now may be a good time to follow through on this. If there's still interest. There are still some unresolved design considerations, hence the desire for the brainstorm proposal.

If anyone currently wants this, please ping here.

If anyone wants to be part of the design discussion, also ping here.

[scottmin0r]

I would definitely like this; +1 for being part of the design discussion as well. Knowing what to build in presubmits/CI for each platform is currently one of the major pain points in our build infrastructure.

[mjs-sx]

I'm still interested in this feature, for the reasons originally described.

[gregestren]

Thanks for the quick feedback.

I'll try to seed a brainstorm proposal by EOY (I'll be on vacation two weeks in the middle of December) and we can iterate from there.

[scottmin0r]

I'm excited to read it! I'm wondering if we can clarify what the problem is though, because I think the title of this issue is one possible solution to an underlying problem, which is defining "what should build" in a given bazel workspace.

Knowing "what should build" is useful for:

• creating/maintaining presubmit/CI systems
• being able to gain 100% confidence that local changes break no targets in any build

Concretely, when you only have one platform, with a sufficiently small codebase, your organization can decree that a trivial bazel build //... and bazel test //... should always work. The same is true with multiple platforms, as long as all targets are valid for both platforms; the set of commands to validate the repo "is green" is bazel build //... and bazel test //... for each platform. The issue arises (as stated in the original bazel-discuss thread) when some targets are not valid on at least one platform. Now what is the set of commands that validates that the repo is green? You have to know: 1. what the platforms are, and 2. which targets should build/test for each platform.

For our team, the lists of valid targets for each platform get sprinkled into the CI bash/batch scripts, which is not an obvious place to look/maintain for the non-build-engineers on the team. Furthermore, it becomes harder for people to exhaustively test their changes locally, because they have to remember that it's bazel build --config=foo //bar/baz/... //quux:xyzzy (especially since it can change over time!).

I'm looking to answer the question "what should build on each platform in our workspace", and to do it in such a way that non-build-engineers can maintain it, the same way they can maintain BUILD rules. (I was looking to do this with a home-grown tool, but if bazel wants to bake it in then that's even better!)

My questions are, for any given workspace with multiple platforms that support different sets of targets

• Can there be a common set of bazel commands that mean "build everything"/"test everything"?
• If so, what is the bazel-y way to define what "everything" means for a workspace?

I'm open to a solution that says that

"everything" for a given platform is defined by //... minus (the set of targets that aren't compatible with that platform)
as implied above for for platform in [list of supported platforms]; bazel build --platform=$platform --filter_unsupported_platforms //...; done; however, I don't think that's the only possible approach (which brings me full-circle to trying to clarify the root issue).

Are these the same problems/goals that you guys see, or am I totally off the mark here?

[gregestren]

Quick response before I go on vacation for two weeks:

1. Happy to discuss more as long as we need. I didn't want to imply I'd just write up a proposal in place of collaborative discussion.
2. I'd like to understand better how platform settings are embedded in your CI / external scripts. I think it's reasonable for bazel / BUILD files to fully handle this.
3. I agree testing / presubmits / CI / user builds should all be easy to make consistent. I think that's a high-level goal even beyond this specific issue.

So overall I think we're seeing the same challenges.

[gregestren]

@scottmin0r - getting back up to speed on this thread. I read thoroughly over your comment and my take of the basic problem is pretty much what you're suggesting.

In my mind, we can use the platform API to let rules declare which platforms they are and aren't compatible with. Then bazel build //... --config=whatever_platform automatically filters out whatever targets don't work for that platform (and communicates which ones were filtered out so they're not just silently dropped).

So for non-build engineers, things should "just work".

I'm curious - did you see different implications in the first comment on this thread? That reads to me the same as I read your message.

[gregestren]

Re: moving forward: whoever's interested in collaborating, how would you like to move forward?

1. Just continue discussion here?
2. I can seed a shared Google doc or start up a Markdown proposal at https://github.com/bazelbuild/proposals that we can all iterate on?
3. I can do my best guess attempt at a rough proposal and we'll continue discussion on it in a dedicated thread?

[nlopezgi]

fyi @agoulti

[dclarkNV]

Love the proposals here, is this still on anyone's radar?

[katre]

@dclarkNV Yes, it's absolutely still active, just that @gregestren is out for the next week or so.

[AustinSchuh]

And @gregestren and I are like 20 pages and a dozen emails into a proposal. I think we have converged on an idea that solves all our problems. I'm going to be taking another stab at writing it up next week while Greg is out to see if we are actually in agreement or not.

[jtattermusch]

gRPC is also running into this problem (we need to run tests on multiple platforms and the some tests need to be configured differently on different platforms; some tests only support a subset of platforms and should not run elsewhere). Is the any known workarounds?

[gregestren]

Jan - the former (configuring differently for different platforms) should be possible today.

One workaround for not supporting a given platform is to alias the test's logic to a no-op for that platform (or for //conditions:default if you want to explicitly whitelist which platforms are legit).

[jtattermusch]

@gregestren I feel that defaulting a test to no-op if not supported a given platform is not a good solution. We do have thousands of tests and it's dangerous to make tests appear as if they were run (they show up as green in the test reports) while they are basically being skipped. Maybe this would be acceptable for very small projects that have a small number of tests and it's obvious which ones are supposed to be run e.g. on windows and which ones are not - but for most large real world project, the proposed solutions seems unacceptable because it's only going to lead to confusion and difficult to reveal mistakes.

[AustinSchuh]

@jtattermusch , we are working on a real solution. It is proving to be hard, so progress is slower than we'd like. Greg is proposing above a workaround until this lands. In all 3 of the decently sized Bazel codebases I've used, all 3 have needed target skipping. https://docs.google.com/document/d/1cjR8oUwGAtokKbpBJR-jp26l2HxfOrKRJe20VZ7Rbvw/edit?ts=5d9c8924 has our attempt to document the problem statement. We would appreciate any feedback on it. gRPC is one of the projects that I used as an example in that document. Using gRPC has also helped me refine my views on how expressive the interface needs to be to work in the real world.

There is a follow-on proposal that is being worked on to propose an implementation which will allow for platform and toolchain based skipping and selecting. I can't promise a timeframe, but it is being worked on. We'll share it on bazel-dev when it is ready for feedback.

[jtattermusch]

Thanks @AustinSchuh I was just trying to explain why the proposed workaround doesn't really work for us (and for others). I'll try to provide some feedback on the design once I have a bit of time.

Thanks for the updates!

[jtattermusch]

I've read through https://docs.google.com/document/d/12n5QNHmFSkuh5yAbdEex64ot4hRgR-moL1zRimU7wHQ/edit#heading=h.5mcn15i0e1ch and the proposed way of skipping incompatible targets works for us IMHO. Thanks for sharing the design!

…

On Tue, Oct 8, 2019 at 5:18 PM Austin Schuh ***@***.***> wrote: @jtattermusch <https://github.com/jtattermusch> , we are working on a real solution. It is proving to be hard, so progress is slower than we'd like. Greg is proposing above a workaround until this lands. In all 3 of the decently sized Bazel codebases I've used, all 3 have needed target skipping. https://docs.google.com/document/d/1cjR8oUwGAtokKbpBJR-jp26l2HxfOrKRJe20VZ7Rbvw/edit?ts=5d9c8924 has our attempt to document the problem statement. We would appreciate any feedback on it. gRPC is one of the projects that I used as an example in that document. Using gRPC has also helped me refine my views on how expressive the interface needs to be to work in the real world. There is a follow-on proposal that is being worked on to propose an implementation which will allow for platform and toolchain based skipping and selecting. I can't promise a timeframe, but it is being worked on. We'll share it on bazel-dev when it is ready for feedback. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3780?email_source=notifications&email_token=ACL2VZDGOMPXW6KTLYQQDIDQNSQETA5CNFSM4D323MQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAURMDY#issuecomment-539563535>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACL2VZERLBEOEPMRC72EJXTQNSQETANCNFSM4D323MQA> .

-- Jan

[gregestren]

Just a quick update: @philsc is working on implementation now!

[gregestren]

Update: PR is in review at #10945. @philsc emailed me recently saying he'll do the next round when he can but it's not on the top of his plate yet.

[philsc]

Yeah, sorry. Tomorrow is the day I'll resume working on it. Apologies for the delay.

[xiay-nv]

Any update on this? This is really a much desired feature.

[philsc]

It's been slow progress, apologies @xiay-nv. We're getting close to finishing it though.
There are a couple of outstanding issues regarding artifact generation. And then there are the doc updates and unit tests. Will spend more time on that this weekend.

[xiay-nv]

@philsc Thanks for the update!

[gregestren]

@philsc 's PR just merged: #10945. Congrats @philsc!

As for deployment, it looks like this'll just make the cut to get into the next release?

We're especially interested in your experiences and feedback using this once available. Also note @philsc and @AustinSchuh are delivering a virtual tech talk on this at BazelCon on November 13. Check that out for a good overview.

[konste]

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.

@philsc when your feature is released would it help here? When filegroup depends on the targets some of which are not compatible with the requested platform - will those targets be skipped from filegroup or the whole filegroup is going to be deemed incompatible and skipped?

[philsc]

@konste, Incompatible Target Skipping on its own is not sufficient for your purposes.

You'd have to filter out the targets manually:

filegroup(
    name = "binaries",
    srcs = select({
            "@platforms//cpu:x86": [":my_x86_binaries"],
            "//conditions:default": [],
    }) + select({
            "@platforms//cpu:arm": [":my_arm_binaries"],
            "//conditions:default": [],
    }),
)

It's not great, but I think that's better behaviour than bazel auto-filtering srcs.

That being said, I wonder if we could expose the IncompatiblePlatformProvider to Starlark and filter there. I.e. maybe we could create something like this:

filegroup(
    name = "binaries",
    srcs = constraints.filter([
            ":my_x86_binaries",
            ":my_arm_binaries",
    ]),
)

WDYT about this, @gregestren ? I feel like auto-filtering is not the answer, but explicit filtering via a helper may be simple and readable.

[konste]

Yes, manual filtering of filegroup srcs using select is what we are doing at this time. What I don't like about it is that the knowledge which target is compatible with which platform has to be expressed at the filegroup level (instead of the target itself) which may be in the different workspace, different repo, etc.

target_compatible_with is still going to be beneficial as it would prevent people from accidental attempts to build incompatible targets directly. For this reason we plan to add target_compatible_with attribute where applicable anyway.

I was thinking about replacing built-in filegroup rule in that "funnel" role with the custom rule, such as "target_group" which would somehow look at its srcs and do the filtering based on their target_compatible_with attributes. If I understand you right the IncompatiblePlatformProvider you proposed should help a great deal with that.

Of course if filegroup would be able to do that and custom rule is not necessary it would be better, but frankly I don't understand the idea with constraints.filter([]). It is not obvious for the reader that the filtering is done based on target_compatible_with attributes. Maybe name it to express that relation, if I understand the idea right?

[gregestren]

@philsc - that's an interesting proposal. It has some non-trivial API implications, but it's certainly something I'm happy to think about.

Re: today, can the targets self-select, i.e. apply a select() on themselves that becomes an empty target if the wrong condition matches? Or the targets are alias()es that do the same thing?

[konste]

@gregestren I understand your idea and will try to implement it as a stop-gap workaround.

[philsc]

Hmm. I forgot that accessing the IncompatiblePlatformProvider from Starlark is not feasible as-is because any target that depends on an incompatible target will by definition be skipped. So any Starlark code that tries to look for the provider never gets executed. That's probably what @gregestren was referring to with "non-trivial API implications". I'll think about it some more.

[konste]

Oh, drats! Indeed! So the real problem as I see it now is not to filter out incompatible targets - this is happening all right, but rather to let the targets which depend on the mix of compatible and incompatible targets to proceed with the compatible subset.

[konste]

Here is a specific problem we are trying to solve with the `target_compatible_with" attribute:

We have thousands of “output” targets (binaries, test_binaries, packages) most of which are compatible with the tree standard platforms: Linux, Darwin, Windows_64. I would rather not visit them all and add them standard “target_compatible_with” attribute – too much hassle and code bloat.
Few of those targets are special and compatible to Windows only – they should be skipped on any other platform and we want to mark them individually.
Very few of these targets are special in the other direction – they are compatible with Emscripten target platform, additionally to the tree standard platforms. Again we don’t mind marking them individually.
What I would like to avoid though is the explicit marking of the bulk of the targets in exactly the same way. Maybe we can introduce a configurable default value for “target_compatible_with” for the package, workspace, or simply in .bazelrc which we should be able to change on the target level. I would hate to visit each and every target to add that it is not compatible with Emscripten.

We need filegroup to accept a mix of the targets compatible and incompatible with the currently requested pattern and let ignore incompatible targets and collect compatible ones.
But there is more to it – we also have “test_suite” rules, which accept the collection of the test target. Extra complexity here stems from the fact that for “test_suite” srcs attribute is not configurable!

Based on all that custom filtering rule does not look like a feasible path forward. It looks like we need Bazel built-in function to do the filtering and dependency on incompatible target through that function should not be considered invalidating dependency itself.

[AustinSchuh]

We should probably split your requests/thoughts out into separate issues so they don't get lost.

The initial proposal had package level defaults. @katre and @gregestren (If my memory holds) proposed we come back to that, and that buildozer can be used today to automate most of the labor of adding them by hand. Not a particularly elegant solution, but functional. We tried to strip down the feature as far as possible so we could actually get something submitted.

[konste]

@AustinSchuh We tried to strip down the feature as far as possible so we could actually get something submitted. I totally understand and support that motivation! I am gladly splitting it into smaller tracking issues:

Here is the first one: Allow to filter lists of targets by the platform compatibility.
Here is the second: Allow for a bulk assignment of the platform compatibility attribute.

[konste]

I wonder if at least Allow to filter lists of targets by the platform compatibility could be addressed in time for 4.0? Otherwise, the feature has rather limited usability.

[AustinSchuh]

Since 4.0 is looking like tomorrow is the cut, I don't think it is going to happen in time :( Thanks for the feedback though, and keep it coming.