Refactor Incompatible Target Skipping #14096
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
philsc
commented
Nov 23, 2021
src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
Outdated
Show resolved
Hide resolved
gregestren
reviewed
Nov 23, 2021
src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
Outdated
Show resolved
Hide resolved
.../java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback @gregestren , I really appreciate it! |
|
No worries. Thanks for the update! Just FYI there's been some unrelated commits on |
|
I'd love to see this moving again! We hit this in the swift rules today |
|
FYI @keith : #15271 (comment) |
|
@keith , yeah. Sorry about the delay. Now that robotics season is over, I'll get back to it. |
|
Fair enough. I spent a couple of hours trying a couple of things, but didn't really get anywhere. The visibility code is more complicated than I expected. I would like to spend another couple hours on it to see if I can figure it out. But otherwise I am okay with creating a ticket for fixing the visibility/incompatibility problem and merging this as-is. |
Sounds good to me. |
|
I won't have time this week to experiment further with moving visibility checking. So I'll do that next week. That being said, there is a question you could answer for me in case I still don't get anywhere. |
|
I think that's good for a start. We could always expend as needed, but no need to overdo documentation at first. |
|
I will keep looking into fixing the visibility issue, but as a separate, future effort. I filed #16044 and referenced it in the code and in the docs. Let me know if that looks reasonable. |
katre
approved these changes
Aug 4, 2022
gregestren
added
awaiting-PR-merge
philsc
added a commit
to philsc/bazel
that referenced
this pull request
Sep 14, 2022
The latest refactor unintentionally made it so you can list the same constraint multiple times in the `target_compatible_with` attribute. It was unintentional, but actually greatly simplifies a discussion point on bazelbuild/bazel-skylib#381. That skylib patch aims to make it easier to write non-trivial `target_compatible_with` expressions. For example, to express that something is compatible with everything except for Windows, one can use the following: foo_binary( name = "bin", target_compatible_with = select({ "@platforms//os:windows": ["@platforms//:incomaptible"], "//conditions:default: [], }), ) The skylib patch aims to reduce that to: foo_binary( name = "bin", target_compatible_with = compatibility.none_of("@platforms//os:windows"), ) This works fine on its own, but a problem arises when these expressions are composed. For example, a macro author might write the following: def foo_wrapped_binary(name, target_compatible_with = [], **kwargs): foo_binary( name = name, target_compatible_with = target_compatible_with + select({ "@platforms//os:none": ["@platforms//:incompatible"], "//conditions:default": [], }), ) A macro author should be able to express their own incompatibility while also honouring user-defined incompatibility. It turns out that my latest refactor (bazelbuild#14096) unintentionally made this work. This happened because we now check for incompatibility before we perform a lot of sanity checks. That's also one of the reasons that visibility restrictions are not honoured for incomaptible targets at the moment (bazelbuild#16044). Without the deduplicating behaviour, macro authors are stuck with either not allowing composition, or having to create unique incompatible constraints for every piece in a composed `target_compatible_with` expression. This patch adds a test to validate the deduplicating behaviour to cement it as a feature. Unfortunately, this means that `target_compatible_with` behaves differently from other label list attributes. For other label list attributes, bazel complains when labels are duplicated. For example, the following BUILD file: py_library( name = "lib", ) py_binary( name = "bin", srcs = ["bin.py"], deps = [ | ":lib", | ":lib", ], ) will result in the following error: $ bazel build :bin INFO: Invocation ID: 3d8345a4-2e76-493e-8343-c96a36185b52 ERROR: /home/jenkins/repos/test_repo/BUILD:41:10: Label '//:lib' is duplicated in the 'deps' attribute of rule 'bin' ERROR: error loading package '': Package '' contains errors INFO: Elapsed time: 2.514s INFO: 0 processes. FAILED: Build did NOT complete successfully (1 packages loaded)
copybara-service bot
pushed a commit
that referenced
this pull request
Sep 16, 2022
The latest refactor unintentionally made it so you can list the same incompatible constraint multiple times in the `target_compatible_with` attribute. It was unintentional, but actually greatly simplifies a discussion point on bazelbuild/bazel-skylib#381. That skylib patch aims to make it easier to write non-trivial `target_compatible_with` expressions. For example, to express that something is compatible with everything except for Windows, one can use the following: foo_binary( name = "bin", target_compatible_with = select({ "@platforms//os:windows": ["@platforms//:incomaptible"], "//conditions:default: [], }), ) The skylib patch aims to reduce that to: foo_binary( name = "bin", target_compatible_with = compatibility.none_of("@platforms//os:windows"), ) This works fine on its own, but a problem arises when these expressions are composed. For example, a macro author might write the following: def foo_wrapped_binary(name, target_compatible_with = [], **kwargs): foo_binary( name = name, target_compatible_with = target_compatible_with + select({ "@platforms//os:none": ["@platforms//:incompatible"], "//conditions:default": [], }), ) A macro author should be able to express their own incompatibility while also honouring user-defined incompatibility. It turns out that my latest refactor (#14096) unintentionally made this work. This happened because we now check for incompatibility before we perform a lot of sanity checks. That's also one of the reasons that visibility restrictions are not honoured for incomaptible targets at the moment (#16044). Without the deduplicating behaviour, macro authors are stuck with either not allowing composition, or having to create unique incompatible constraints for every piece in a composed `target_compatible_with` expression. This patch adds a test to validate the deduplicating behaviour to cement it as a feature. Unfortunately, this means that `target_compatible_with` behaves differently from other label list attributes. For other label list attributes, bazel complains when labels are duplicated. For example, the following BUILD file: py_library( name = "lib", ) py_binary( name = "bin", srcs = ["bin.py"], deps = [ | ":lib", | ":lib", ], ) will result in the following error: $ bazel build :bin INFO: Invocation ID: 3d8345a4-2e76-493e-8343-c96a36185b52 ERROR: /home/jenkins/repos/test_repo/BUILD:41:10: Label '//:lib' is duplicated in the 'deps' attribute of rule 'bin' ERROR: error loading package '': Package '' contains errors INFO: Elapsed time: 2.514s INFO: 0 processes. FAILED: Build did NOT complete successfully (1 packages loaded) Closes #16274. PiperOrigin-RevId: 474747867 Change-Id: Iacc4de12ce90176d803e0bc9e695d4ec14603449
aiuto
pushed a commit
to aiuto/bazel
that referenced
this pull request
Oct 12, 2022
The latest refactor unintentionally made it so you can list the same incompatible constraint multiple times in the `target_compatible_with` attribute. It was unintentional, but actually greatly simplifies a discussion point on bazelbuild/bazel-skylib#381. That skylib patch aims to make it easier to write non-trivial `target_compatible_with` expressions. For example, to express that something is compatible with everything except for Windows, one can use the following: foo_binary( name = "bin", target_compatible_with = select({ "@platforms//os:windows": ["@platforms//:incomaptible"], "//conditions:default: [], }), ) The skylib patch aims to reduce that to: foo_binary( name = "bin", target_compatible_with = compatibility.none_of("@platforms//os:windows"), ) This works fine on its own, but a problem arises when these expressions are composed. For example, a macro author might write the following: def foo_wrapped_binary(name, target_compatible_with = [], **kwargs): foo_binary( name = name, target_compatible_with = target_compatible_with + select({ "@platforms//os:none": ["@platforms//:incompatible"], "//conditions:default": [], }), ) A macro author should be able to express their own incompatibility while also honouring user-defined incompatibility. It turns out that my latest refactor (bazelbuild#14096) unintentionally made this work. This happened because we now check for incompatibility before we perform a lot of sanity checks. That's also one of the reasons that visibility restrictions are not honoured for incomaptible targets at the moment (bazelbuild#16044). Without the deduplicating behaviour, macro authors are stuck with either not allowing composition, or having to create unique incompatible constraints for every piece in a composed `target_compatible_with` expression. This patch adds a test to validate the deduplicating behaviour to cement it as a feature. Unfortunately, this means that `target_compatible_with` behaves differently from other label list attributes. For other label list attributes, bazel complains when labels are duplicated. For example, the following BUILD file: py_library( name = "lib", ) py_binary( name = "bin", srcs = ["bin.py"], deps = [ | ":lib", | ":lib", ], ) will result in the following error: $ bazel build :bin INFO: Invocation ID: 3d8345a4-2e76-493e-8343-c96a36185b52 ERROR: /home/jenkins/repos/test_repo/BUILD:41:10: Label '//:lib' is duplicated in the 'deps' attribute of rule 'bin' ERROR: error loading package '': Package '' contains errors INFO: Elapsed time: 2.514s INFO: 0 processes. FAILED: Build did NOT complete successfully (1 packages loaded) Closes bazelbuild#16274. PiperOrigin-RevId: 474747867 Change-Id: Iacc4de12ce90176d803e0bc9e695d4ec14603449
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch reworks how Incompatible Target Skipping is implemented.
The biggest change is that incompatibility is now checked earlier. This
allows us to avoid evaluating dependencies (such as toolchains) in
situations where a target is "directly" incompatible. "Direct incompatibility"
refers to incompatibility due to a target's
target_compatible_withvalue.Moving the incompatibility checking earlier had the undesired effect of
visibility restrictions being ignored on incompatible targets. This is
tracked in #16044. As per
#14096 (comment),
we will fix it in a separate patch.
Fixes #12897.