Fix a couple of bugs with Incompatible Target Skipping#12560
Closed
philsc wants to merge 1 commit intobazelbuild:masterfrom
Closed
Fix a couple of bugs with Incompatible Target Skipping#12560philsc wants to merge 1 commit intobazelbuild:masterfrom
philsc wants to merge 1 commit intobazelbuild:masterfrom
Conversation
aiuto
added
the
team-Configurability
Contributor
Author
|
Okay, this is ready for review. I'm still not happy about making |
frc971-automation
pushed a commit
to frc971/971-Robot-Code
that referenced
this pull request
Dec 5, 2020
This is the same version as 4.0.0rc2 except with 2 cherry-picks: a3c94ec2ed Fix builds for filegroup targets with incompatible dependencies b3c9d7381c Fix a couple of bugs with Incompatible Target Skipping The patch for the filegroup fix has already merged on master: bazelbuild/bazel#12601 The other patch is still under review: bazelbuild/bazel#12560 This should enable us to convert the code base to use platforms. Change-Id: I35f078c2576f5268d5e3fb33e9bd86732529f744
gregestren
reviewed
Dec 9, 2020
Contributor
gregestren
left a comment
gregestren
left a comment
There was a problem hiding this comment.
This overall looks good to me. I also don't think it's that huge a deal with the new dependency from RuleContext, although I appreciate the general concern about that.
Just the code placing comment as my only nit.
gregestren
approved these changes
Dec 9, 2020
gregestren
reviewed
Dec 9, 2020
While adding `target_compatible_with` attributes in the FRC team 971's code base I came across bug bazelbuild#12553. It wasn't possible to make a Python target depend on another incompatible Python target. This patch fixes that issue by teaching `RuleContext` about incompatible prerequisites. This also fixes an issue with the validation of file extensions. It's possible that `validateDirectPrerequisite()` skips a bit too much validation. It was unfortunately the cleanest way I could think of. I struggled a bit to come up with what ended up becoming `RuleContextConstraintSemantics.IncompatibleCheckResult`. The purpose of that class is to centralize the logic for checking for `OutputFileConfiguredTarget` dependencies. Such dependencies need a bit of special logic in order to find `IncompatiblePlatformProvider` instances. When I implemented this patch, I realized that the `TopLevelConstraintSemantics` logic had a very similar problem. It didn't deal with the `OutputFileConfiguredTarget` problem at all. I took the liberty of fixing that issue in this patch also because it nicely re-used the same new helper. I could not figure out a good way to avoid making `RuleContext` depend on `RuleContextConstraintSemantics`. Fixes bazelbuild#12553
frc971-automation
pushed a commit
to frc971/971-Robot-Code
that referenced
this pull request
Dec 13, 2020
This is the upstream rc6 branch with two cherry-picks: Fix a couple of bugs with Incompatible Target Skipping bazelbuild/bazel#12560 Add --{no,}autodetect_server_javabase bazelbuild/bazel#12542 This patch also uses the new --noautodetect_server_javabase flag. Change-Id: I7a397e4a9f17b942d0f81c7affb829d2de385a30
meisterT
pushed a commit
that referenced
this pull request
Dec 15, 2020
While adding `target_compatible_with` attributes in the FRC team 971's code base I came across bug #12553. It wasn't possible to make a Python target depend on another incompatible Python target. This patch fixes that issue by teaching `RuleContext` about incompatible prerequisites. This also fixes an issue with the validation of file extensions. It's possible that `validateDirectPrerequisite()` skips a bit too much validation. It was unfortunately the cleanest way I could think of. I struggled a bit to come up with what ended up becoming `RuleContextConstraintSemantics.IncompatibleCheckResult`. The purpose of that class is to centralize the logic for checking for `OutputFileConfiguredTarget` dependencies. Such dependencies need a bit of special logic in order to find `IncompatiblePlatformProvider` instances. When I implemented this patch, I realized that the `TopLevelConstraintSemantics` logic had a very similar problem. It didn't deal with the `OutputFileConfiguredTarget` problem at all. I took the liberty of fixing that issue in this patch also because it nicely re-used the same new helper. I could not figure out a good way to avoid making `RuleContext` depend on `RuleContextConstraintSemantics`. Fixes #12553 Closes #12560. PiperOrigin-RevId: 346796174
ulfjack
pushed a commit
to EngFlow/bazel
that referenced
this pull request
Mar 5, 2021
While adding `target_compatible_with` attributes in the FRC team 971's code base I came across bug bazelbuild#12553. It wasn't possible to make a Python target depend on another incompatible Python target. This patch fixes that issue by teaching `RuleContext` about incompatible prerequisites. This also fixes an issue with the validation of file extensions. It's possible that `validateDirectPrerequisite()` skips a bit too much validation. It was unfortunately the cleanest way I could think of. I struggled a bit to come up with what ended up becoming `RuleContextConstraintSemantics.IncompatibleCheckResult`. The purpose of that class is to centralize the logic for checking for `OutputFileConfiguredTarget` dependencies. Such dependencies need a bit of special logic in order to find `IncompatiblePlatformProvider` instances. When I implemented this patch, I realized that the `TopLevelConstraintSemantics` logic had a very similar problem. It didn't deal with the `OutputFileConfiguredTarget` problem at all. I took the liberty of fixing that issue in this patch also because it nicely re-used the same new helper. I could not figure out a good way to avoid making `RuleContext` depend on `RuleContextConstraintSemantics`. Fixes bazelbuild#12553 Closes bazelbuild#12560. PiperOrigin-RevId: 346796174
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
While adding
target_compatible_withattributes in the FRC team 971'scode base I came across bug #12553. It wasn't possible to make a
Python target depend on another incompatible Python target.
This patch fixes that issue by teaching
RuleContextaboutincompatible prerequisites. This also fixes an issue with the
validation of file extensions.
It's possible that
validateDirectPrerequisite()skips a bit too muchvalidation. It was unfortunately the cleanest way I could think of.
I struggled a bit to come up with what ended up becoming
RuleContextConstraintSemantics.IncompatibleCheckResult. The purposeof that class is to centralize the logic for checking for
OutputFileConfiguredTargetdependencies. Such dependencies need abit of special logic in order to find
IncompatiblePlatformProviderinstances. When I implemented this patch, I realized that the
TopLevelConstraintSemanticslogic had a very similar problem. Itdidn't deal with the
OutputFileConfiguredTargetproblem at all. Itook the liberty of fixing that issue in this patch also because it
nicely re-used the same new helper.
I could not figure out a good way to avoid making
RuleContextdependon
RuleContextConstraintSemantics.Fixes #12553