Add basic incompatible target skipping

[philsc]

This patch aims to implement a basic version of incompatible target
skipping outlined here:
https://docs.google.com/document/d/12n5QNHmFSkuh5yAbdEex64ot4hRgR-moL1zRimU7wHQ/edit?usp=sharing

The implementation in this patch supports target skipping based
on the target platform. In a BUILD file you can now add constraints
that the target platform must satisfy in order for the target to be
built and/or tested. For example, use the following snippet to declare
a target to be compatible with Windows platforms only:

cc_binary(
    name = "bin",
    srcs = ["bin.cc"],
    target_compatible_with = [
        "@platforms//os:windows",
    ],
)

Builds triggered with :all or ... on a non-Windows platform will
simply skip the incompatible target. An appropriate note is shown on
the command line if the --show_result threshold is high enough.
Targets that transitively depend on incompatible targets are
themselves considered incompatible and will also be skipped.

Explicitly requesting an incompatible target on the command line is an
error and will cause the build to fail. Bazel will print out an
appropriate error message and inform the user what constraint could
not be satisfied.

See the new documentation in this patch for more information. In
particular, https://docs.bazel.build/versions/master/platforms.html
should be a good bit more informative.

This implementation does not make any effort to support expressing
compatibility with toolchains. It is possible that using select()
(commented on below) already makes this possible, but it's not
validated or explicitly supported in this patch.

During implementation we noticed that select() can be quite powerful
in combination with target_compatible_with. A basic summary of this
is also documented on the Platforms page.

It may be useful to create helper functions in, say, skylib to help
make complex select() statements more readable. For example, we
could replace the following:

target_compatible_with = select({
    "@platforms//os:linux": [],
    "@platforms//os:macos": [],
    "//conditions:default": [":not_compatible"],
})

with something like:

target_compatible_with = constraints.any_of([
    "@platforms//os:linux",
    "@platforms//os:macos",
])

That, however, is work for follow-up patches.

Many thanks to Austin Schuh (@AustinSchuh) and Greg Estren
(@gregestren) for working on the proposal and helping a ton on this
patch itself. Also thanks to many others who provided feedback on the
implementation.

RELNOTES: Bazel skips incompatible targets based on target platform
and target_compatible_with contents. See
https://docs.bazel.build/versions/master/platforms.html for more
details.

[philsc]

I'm posting this review to get some feedback from @gregestren (and anyone else interested) regarding implementation, functionality, etc.

There is various documentation missing for starters. Also, the code could use some auto-formatting love.

[philsc]

Ah, also looks like I need to update various unit tests. I'll get around to that also.
Again, my main interest in the short term is to get some feedback on the implementation and such.

[gregestren]

Acknowledged. Please have patience but I'll look through as soon as I can.

[gregestren]

@philsc thanks for your patience. I'm going to look at this Monday.

[gregestren]

First pass. Will do another pass tomorrow.

[philsc]

It's been longer than I'd like since I last looked at this. Looks like I have to look at a lot of the pieces again.
I'll add some comments to keep future me on track.

[philsc]

I'm almost done incorporating your feedback. I also have to resolve a couple of conflicts when rebasing on 3.1.

[gregestren]

Just as a sanity check, the commit on this PR is still old, right?

[gregestren]

All my nitty comments aside, looking fairly good to me. The status reporting logic look also looks really nice.

[philsc]

A minor update to address some of @gregestren 's feedback.

Added more TODOs to remind myself to add documentation.

[AustinSchuh]

Starting the merge process...

Woah! I feel like we should frame that message. Nice job Phil on doing the heavy lifting and getting this over the finish line and thanks Greg for all your help and wisdom.

[katre]

It needs to be in a separate PR, not just a separate commit in this PR. Will the change work standalone or does it require the rest of these changes? Either way we can merge it, I can help with the third_party side (as well as reviewing the non-third_party side).

…

On Thu, Oct 22, 2020 at 4:25 PM Greg ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In WORKSPACE <#10945 (comment)>: > @@ -422,6 +422,8 @@ http_archive( "https://mirror.bazel.build/github.com/bazelbuild/bazel-toolchains/releases/download/3.1.0/bazel-toolchains-3.1.0.tar.gz", "https://github.com/bazelbuild/bazel-toolchains/releases/download/3.1.0/bazel-toolchains-3.1.0.tar.gz", ], + patches = ["@//third_party:bazel_toolchains/0001-Rename-target_compatible_with-to-internal_target_com.patch"], Merge update: third_party changes have to be in their own dedicated commit. Can we move the .patch file to its own commit and get that submitted first? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10945 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACPW7Y532ULTJ2LPIWTNLDSMCIMXANCNFSM4LFNQWKA> .

[philsc]

It needs to be in a separate PR, not just a separate commit in this PR. Will the change work standalone or does it require the rest of these changes? Either way we can merge it, I can help with the third_party side (as well as reviewing the non-third_party side).

@katre , @gregestren , here's the separate PR for the third-party portion: #12336

[philsc]

@katre , this is now rebased on top of the separate third-party and WORKSPACE patches.

[katre]

Hi @philsc! This is a great change and I'm amazed at the progress you've made.

The large number of comments is my fault: I wasn't keeping up with the status of this PR and so am coming in late with suggestions and comments. Most of them are fairly minor, and are aimed at making the code be more inline with our internal style guide.

The only major change I'd like to propose is in IncompatiblePlatformProvider, and I've left comments there explaining my concerns and suggestions. If these have already been raised during this review, I apologize, and will happily defer to the previous reviewers. If you decide to leave that class as-is, however, could you add comments explaining the rationale for the decision?

Thanks, and sorry for my tardiness.

[AustinSchuh]

Since I have the day off, I volunteered to help Phil out addressing comments. The easy ones should be done. Pushed to get some CI feedback. Now time to look at the harder ones.

[katre]

This looks great, thanks so much.

Greg, it's ready to be re-imported. let me know if you want me to help.

[gregestren]

This looks great, thanks so much.

Greg, it's ready to be re-imported. let me know if you want me to help.

Got it. I'll re-import today. Stay tuned all...

[peakschris]

Does this feature work with general rules? I'm finding that the presence of this rule continues to trigger load of py3_image in WORKSPACE when building on windows, and since io_bazel_rules_docker are not supported on Windows, this causes a build error.

py3_image(
name = "myscript",
srcs = ["myscript.py"],
data = [":somedeps"],
deps = [requirement("flask")],
target_compatible_with = ["@platforms//os:linux"],
)

[aiuto]

Skipping the target happens after we have analyzed enough to evaluate the rule, so all the workspace loads have to have happened already. If you want py3_image to essentially be a no-op when you are building from windows, then you'll have to do workspace gymnastics to load fake docker rules.