Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatible Target Skipping breaks rules with custom providers #12553

Closed
philsc opened this issue Nov 24, 2020 · 4 comments
Closed

Incompatible Target Skipping breaks rules with custom providers #12553

philsc opened this issue Nov 24, 2020 · 4 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions

Comments

@philsc
Copy link
Contributor

philsc commented Nov 24, 2020

Description of the problem / feature request:

Consider the following Starlark rule:

DummyProvider = provider()

def _dummy_rule_impl(ctx):
    return [DummyProvider()]

dummy_rule = rule(
    implementation = _dummy_rule_impl,
    attrs = {
        "deps": attr.label_list(providers=[DummyProvider]),
    },
)

If you instantiate it like this:

load(":rules.bzl", "dummy_rule")

dummy_rule(
    name = "dummy1",
    target_compatible_with = ["@platforms//:incompatible"],
)

dummy_rule(
    name = "dummy2",
    deps = [
        ":dummy1",
    ],
)

then building fails with:

ERROR: /home/phil/.cache/bazel/_bazel_phil/b4868b516be4e3bea4d221937a55ced4/sandbox/linux-sandbox/2572/execroot/io_bazel/_tmp/c749fd38ac9a4ad6bd41e9653bbceab5/workspace/target_skipping/BUILD:112:11: in deps attribute of dummy_rule rule //target_skipping:dummy2: '//target_skipping:dummy1' does not have mandatory providers: 'DummyProvider'

This occurs because the incompatible target is assigned an IncompatiblePlatformProvider and not a DummyProvider.

The same thing happens when using Python targets:

ERROR: /home/phil/repos/971-Robot-Code/frc971/control_loops/python/BUILD:189:11: in deps attribute of py_library rule //frc971/control_loops/python:basic_window: '@python_gtk//:python_gtk' does not have mandatory providers: 'py' or 'PyInfo'

A quick hack to fix this is:

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 78ee802789..e094b7b529 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -43,6 +43,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode;
+import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider;
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -2292,6 +2293,10 @@ public final class RuleContext extends TargetContext
         return true;
       }

+      if (prerequisite.getConfiguredTarget().getProvider(IncompatiblePlatformProvider.class) != null) {
+        return true;
+      }
+
       unfulfilledRequirements.add(
           String.format(
               "'%s' does not have mandatory providers: %s",

What operating system are you running Bazel on?

x86_64 Linux

What's the output of bazel info release?

Running off latest master (d0efd7b).

If bazel info release returns "development version" or "(@non-git)", tell us how you built Bazel.

$ bazel build -c opt //src:bazel
@philsc
Copy link
Contributor Author

philsc commented Nov 24, 2020

I'll try to figure out what a less hacky way of addressing this is.

/cc @gregestren , @AustinSchuh

@aiuto aiuto added P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions labels Nov 25, 2020
philsc added a commit to philsc/bazel that referenced this issue Nov 25, 2020
philsc added a commit to philsc/bazel that referenced this issue Nov 25, 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 have
transitive incompatibility for Python targets.

This patch fixes that issue by teaching `RuleContext` about
incompatible prerequisites.

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`. When I implemented this, I realized that
the `TopLevelConstraintSemantics` logic also had a 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
philsc added a commit to philsc/bazel that referenced this issue Nov 25, 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 have
transitive incompatibility for Python targets.

This patch fixes that issue by teaching `RuleContext` about
incompatible prerequisites.

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`. When I implemented this, I realized that
the `TopLevelConstraintSemantics` logic also had a 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
@philsc
Copy link
Contributor Author

philsc commented Nov 28, 2020

Hmm. I discovered another bug while testing #12560. A filegroup that depends on an incompatible target is itself not incompatible.

@philsc
Copy link
Contributor Author

philsc commented Nov 28, 2020

Hmm. I discovered another bug while testing #12560. A filegroup that depends on an incompatible target is itself not incompatible.

Filed #12582 for this.

@philsc
Copy link
Contributor Author

philsc commented Nov 29, 2020

There may also be something similar going on with the checks for the extensions.

ERROR: /home/phil/repos/971-Robot-Code/third_party/gmp/BUILD:481:11: in srcs attribute of cc_library rule //third_party/gmp:mpn_core: '//third_party/gmp:dump' does not produce any cc_library srcs files (expected .cc, .cpp, .cxx, .c++, .C, .cu, .cl, .c, .h, .hh, .hpp, .ipp, .hxx, .h++, .inc, .inl, .tlh, .tli, .H, .tcc, .S, .s, .asm, .a, .lib, .pic.a, .lo,
 .lo.lib, .pic.lo, .so, .dylib, .dll, .o, .obj or .pic.o)

philsc added a commit to philsc/bazel that referenced this issue Dec 2, 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
philsc added a commit to philsc/bazel that referenced this issue Dec 10, 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
meisterT pushed a commit that referenced this issue 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 issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants