Add basic incompatible target skipping #10945

philsc · 2020-03-11T04:52:20Z

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 · 2020-03-11T04:54:20Z

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 · 2020-03-11T04:56:20Z

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 · 2020-03-16T21:43:05Z

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

gregestren · 2020-04-03T22:44:30Z

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

gregestren

First pass. Will do another pass tomorrow.

src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java

src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java

...ain/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

...ain/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java

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.

src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

...ain/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java

philsc

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

...ain/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java

src/test/shell/bazel/target_compatible_with_test.sh

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

gregestren · 2020-07-06T18:02:48Z

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

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

src/test/shell/bazel/target_compatible_with_test.sh

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

gregestren

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

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java

...ava/com/google/devtools/build/lib/analysis/configuredtargets/OutputFileConfiguredTarget.java

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

philsc

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

Added more TODOs to remind myself to add documentation.

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

...ava/com/google/devtools/build/lib/analysis/configuredtargets/OutputFileConfiguredTarget.java

AustinSchuh · 2020-10-22T20:21:34Z

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 · 2020-10-22T20:28:56Z

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> .

The upcoming Incompatible Target Skipping patch (bazelbuild#10945) introduces a new `target_compatible_with` attribute which clashes with one of the custom attributes in the bazel-toolchains repo. This patch fixes the clash so we can merge the Incompatible Target Skipping path. This is essentially a backport of bazelbuild/bazel-toolchains#913.

philsc · 2020-10-23T02:09:33Z

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

The upcoming Incompatible Target Skipping patch (bazelbuild#10945) introduces a new `target_compatible_with` attribute which clashes with one of the custom attributes in the bazel-toolchains repo. This patch fixes the clash so we can merge the Incompatible Target Skipping path. This is essentially a backport of bazelbuild/bazel-toolchains#913.

The upcoming Incompatible Target Skipping patch (#10945) introduces a new `target_compatible_with` attribute which clashes with one of the custom attributes in the bazel-toolchains repo. This patch fixes the clash so we can merge the Incompatible Target Skipping path. This is essentially a backport of bazelbuild/bazel-toolchains#913.

philsc · 2020-10-23T16:06:30Z

@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.

site/docs/platforms.md

...ava/com/google/devtools/build/docgen/templates/attributes/common/target_compatible_with.html

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java

...ain/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java

src/test/shell/bazel/target_compatible_with_test.sh

AustinSchuh · 2020-10-23T21:46:04Z

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.

src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java

@AustinSchuh

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.

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 · 2020-10-26T15:39:25Z

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...

ARM-only target with --cpu=x86. Support work for #10945. PiperOrigin-RevId: 339113675

…rbe_config This essentially the same patch as 9a2fbe2 except that it deals with `target_compatible_with` instead of `exec_properties`. The PR bazelbuild/bazel#10945 adds a new `target_compatible_with` attribute to all rules. That includes repository rules. Currently the patch fails in `//src/test/shell/bazel:bazel_bootstrap_distfile_test`. The error complains about: Error in repository_rule: There is already a built-in attribute 'target_compatible_with' which cannot be overridden Since the repository rule is abstracted by a macro, it should be fairly safe to rename the attribute to `internal_target_compatible_with`.

peakschris · 2022-09-08T15:52:39Z

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 · 2022-09-08T16:06:08Z

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.

philsc requested a review from lberki as a code owner March 11, 2020 04:52

googlebot added the cla: yes label Mar 11, 2020

gregestren self-requested a review March 16, 2020 21:42

aiuto assigned gregestren Mar 24, 2020

aiuto added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 24, 2020

gregestren reviewed Apr 6, 2020

View reviewed changes

gregestren reviewed Apr 14, 2020

View reviewed changes

gregestren mentioned this pull request May 11, 2020

Gracefully filter targets not compatible with the current Platform #3780

Closed

philsc commented May 20, 2020

View reviewed changes

shs96c mentioned this pull request May 21, 2020

test_suite tests should be configurable #11455

Closed

philsc commented May 26, 2020

View reviewed changes

...ain/java/com/google/devtools/build/lib/analysis/constraints/TopLevelConstraintSemantics.java Outdated Show resolved Hide resolved

philsc force-pushed the incompatible-target-skipping branch from 4a6f40d to 6c26d99 Compare July 5, 2020 22:11

philsc commented Jul 5, 2020

View reviewed changes

src/test/shell/bazel/target_compatible_with_test.sh Outdated Show resolved Hide resolved

philsc commented Jul 5, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

illicitonion mentioned this pull request Jul 8, 2020

exec_compatible_with targets which can be built but don't match --platforms cause errors #11729

Closed

gregestren reviewed Jul 8, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/analysis/BuildView.java Outdated Show resolved Hide resolved

philsc force-pushed the incompatible-target-skipping branch 2 times, most recently from c88047b to dd2e775 Compare July 12, 2020 03:58

philsc commented Jul 12, 2020

View reviewed changes

src/test/shell/bazel/target_compatible_with_test.sh Outdated Show resolved Hide resolved

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java Outdated Show resolved Hide resolved

gregestren reviewed Jul 14, 2020

View reviewed changes

philsc force-pushed the incompatible-target-skipping branch from dd2e775 to 21b8d4d Compare August 5, 2020 04:26

philsc commented Aug 5, 2020

View reviewed changes

gregestren reviewed Aug 13, 2020

View reviewed changes

philsc force-pushed the incompatible-target-skipping branch from 21b8d4d to e4df959 Compare August 31, 2020 03:27

philsc mentioned this pull request Oct 23, 2020

Prepare to make bazel-toolchains compatible with Incompatible Target Skipping #12336

Closed

philsc force-pushed the incompatible-target-skipping branch from 66f85e2 to 9c9905c Compare October 23, 2020 15:40

katre requested changes Oct 23, 2020

View reviewed changes

AustinSchuh force-pushed the incompatible-target-skipping branch from 9c9905c to 22818fa Compare October 23, 2020 21:43

AustinSchuh force-pushed the incompatible-target-skipping branch 4 times, most recently from 2915f50 to 397f3bc Compare October 23, 2020 23:57

katre reviewed Oct 24, 2020

View reviewed changes

src/main/java/com/google/devtools/build/lib/analysis/IncompatiblePlatformProvider.java Outdated Show resolved Hide resolved

AustinSchuh force-pushed the incompatible-target-skipping branch from 397f3bc to c853ee9 Compare October 24, 2020 01:05

katre approved these changes Oct 24, 2020

View reviewed changes

bazel-io pushed a commit that referenced this pull request Oct 26, 2020

Add "incompatible target" messages, for example when building an

b78f2ef

ARM-only target with --cpu=x86. Support work for #10945. PiperOrigin-RevId: 339113675

bazel-io closed this in 22b4dbc Oct 27, 2020

keith mentioned this pull request Oct 28, 2020

target_compatible_with failing with analysis_test #12366

Closed

philsc mentioned this pull request Nov 2, 2020

Bazel's shell tests on Windows are noisy unless you use "bazel shutdown" #12397

Closed

philsc deleted the incompatible-target-skipping branch November 3, 2020 04:13

sitaktif mentioned this pull request Sep 2, 2022

Allow filtering of expanded targets according to the available exec configuration (like target_compatible_with but for exec) #16206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic incompatible target skipping #10945

Add basic incompatible target skipping #10945

philsc commented Mar 11, 2020 •

edited

Loading

philsc commented Mar 11, 2020

philsc commented Mar 11, 2020

gregestren commented Mar 16, 2020

gregestren commented Apr 3, 2020

gregestren left a comment

philsc left a comment

philsc left a comment •

edited

Loading

gregestren commented Jul 6, 2020

This comment has been minimized.

gregestren left a comment

philsc left a comment

AustinSchuh commented Oct 22, 2020

katre commented Oct 22, 2020 via email

philsc commented Oct 23, 2020

philsc commented Oct 23, 2020

katre left a comment

AustinSchuh commented Oct 23, 2020

katre left a comment

gregestren commented Oct 26, 2020

peakschris commented Sep 8, 2022

aiuto commented Sep 8, 2022

Add basic incompatible target skipping #10945

Add basic incompatible target skipping #10945

Conversation

philsc commented Mar 11, 2020 • edited Loading

philsc commented Mar 11, 2020

philsc commented Mar 11, 2020

gregestren commented Mar 16, 2020

gregestren commented Apr 3, 2020

gregestren left a comment

Choose a reason for hiding this comment

philsc left a comment

Choose a reason for hiding this comment

philsc left a comment • edited Loading

Choose a reason for hiding this comment

gregestren commented Jul 6, 2020

This comment has been minimized.

gregestren left a comment

Choose a reason for hiding this comment

philsc left a comment

Choose a reason for hiding this comment

AustinSchuh commented Oct 22, 2020

katre commented Oct 22, 2020 via email

philsc commented Oct 23, 2020

philsc commented Oct 23, 2020

katre left a comment

Choose a reason for hiding this comment

AustinSchuh commented Oct 23, 2020

katre left a comment

Choose a reason for hiding this comment

gregestren commented Oct 26, 2020

peakschris commented Sep 8, 2022

aiuto commented Sep 8, 2022

philsc commented Mar 11, 2020 •

edited

Loading

philsc left a comment •

edited

Loading