From 7d5493d922761c3ce0037f0025912cc532c55ad7 Mon Sep 17 00:00:00 2001
From: jcater <jcater@google.com>
Date: Thu, 6 May 2021 11:31:47 -0700
Subject: [PATCH] Update creating exec groups that explicitly copy from
 defaults.

Also add an ExecGroupSubject to improve testing.

PiperOrigin-RevId: 372387338
---
 .../starlark/StarlarkRuleClassFunctions.java  |  2 +-
 .../build/lib/packages/ExecGroup.java         | 14 ++--
 .../build/lib/packages/RuleClass.java         |  9 +--
 .../lib/analysis/StarlarkExecGroupTest.java   | 14 ++--
 .../devtools/build/lib/analysis/testing/BUILD |  1 +
 .../analysis/testing/ExecGroupSubject.java    | 78 +++++++++++++++++++
 .../google/devtools/build/lib/packages/BUILD  |  1 +
 .../lib/packages/RuleClassBuilderTest.java    | 16 ++--
 8 files changed, 106 insertions(+), 29 deletions(-)
 create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/testing/ExecGroupSubject.java

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
index 64825ca315aada..2a0f67134185a0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -933,7 +933,7 @@ public ExecGroup execGroup(
         throw Starlark.errorf(
             "An exec group cannot set copy_from_rule=True and declare toolchains or constraints.");
       }
-      return ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
+      return ExecGroup.copyFromDefault();
     }
 
     ImmutableSet<Label> toolchainTypes = ImmutableSet.copyOf(parseToolchains(toolchains, thread));
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java b/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java
index f91c32aae703d6..a4de2bf17ea9bf 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java
@@ -25,21 +25,23 @@
 @AutoValue
 public abstract class ExecGroup implements ExecGroupApi {
 
-  // An exec group that inherits requirements from the rule.
-  public static final ExecGroup COPY_FROM_RULE_EXEC_GROUP =
-      createCopied(ImmutableSet.of(), ImmutableSet.of());
-
   // This is intentionally a string that would fail {@code Identifier.isValid} so that
   // users can't create a group with the same name.
   @VisibleForTesting public static final String DEFAULT_EXEC_GROUP_NAME = "default-exec-group";
 
   // Create an exec group that is marked as copying from the rule.
+  // TODO(b/183268405): Remove this when RuleClass is updated to better handle inheritance.
   public static ExecGroup createCopied(
       Set<Label> requiredToolchains, Set<Label> execCompatibleWith) {
     return create(requiredToolchains, execCompatibleWith, /* copyFromRule= */ true);
   }
 
-  // Create an exec group.
+  /** Create an exec group that inherits from the default exec group. */
+  public static ExecGroup copyFromDefault() {
+    return create(ImmutableSet.of(), ImmutableSet.of(), /* copyFromRule= */ true);
+  }
+
+  /** Create an exec group with the given toolchains and execution constraints. */
   public static ExecGroup create(Set<Label> requiredToolchains, Set<Label> execCompatibleWith) {
     return create(requiredToolchains, execCompatibleWith, /* copyFromRule= */ false);
   }
@@ -56,5 +58,5 @@ private static ExecGroup create(
 
   public abstract ImmutableSet<Label> execCompatibleWith();
 
-  public abstract boolean copyFromRule();
+  public abstract boolean isCopiedFromDefault();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index d7616ab2175d65..f218342ebf88b6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -16,7 +16,6 @@
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
-import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
 import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -772,8 +771,8 @@ public Builder(String name, RuleClassType type, boolean starlark, RuleClass... p
           // we need to clear out whatever toolchains and constraints have been copied from the rule
           // in order to prevent clashing and fill with the the child's toolchain and constraints.
           for (Map.Entry<String, ExecGroup> execGroup : parent.getExecGroups().entrySet()) {
-            if (execGroup.getValue().copyFromRule()) {
-              cleanedExecGroups.put(execGroup.getKey(), COPY_FROM_RULE_EXEC_GROUP);
+            if (execGroup.getValue().isCopiedFromDefault()) {
+              cleanedExecGroups.put(execGroup.getKey(), ExecGroup.copyFromDefault());
             } else {
               cleanedExecGroups.put(execGroup);
             }
@@ -871,7 +870,7 @@ public RuleClass build(String name, String key) {
       ExecGroup copiedFromRule = null;
       for (Map.Entry<String, ExecGroup> groupEntry : execGroups.entrySet()) {
         ExecGroup group = groupEntry.getValue();
-        if (group.copyFromRule()) {
+        if (group.isCopiedFromDefault()) {
           if (copiedFromRule == null) {
             copiedFromRule =
                 ExecGroup.createCopied(requiredToolchains, executionPlatformConstraints);
@@ -1485,7 +1484,7 @@ public Builder addExecGroups(Map<String, ExecGroup> execGroups) {
 
     /** Adds an exec group that copies its toolchains and constraints from the rule. */
     public Builder addExecGroup(String name) {
-      return addExecGroups(ImmutableMap.of(name, COPY_FROM_RULE_EXEC_GROUP));
+      return addExecGroups(ImmutableMap.of(name, ExecGroup.copyFromDefault()));
     }
 
     /** An error to help report {@link ExecGroup}s with the same name */
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java
index 65ff894f277a5d..5ed82ef3093875 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java
@@ -15,10 +15,10 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.analysis.testing.ExecGroupSubject.assertThat;
 import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -388,12 +388,12 @@ public void testInheritsRuleRequirements() throws Exception {
     scratch.file("test/BUILD", "load('//test:defs.bzl', 'my_rule')", "my_rule(name = 'papaya')");
 
     ConfiguredTarget ct = getConfiguredTarget("//test:papaya");
-    assertThat(getRuleContext(ct).getRule().getRuleClassObject().getExecGroups())
-        .containsExactly(
-            "watermelon",
-            ExecGroup.createCopied(
-                ImmutableSet.of(Label.parseAbsoluteUnchecked("//rule:toolchain_type_1")),
-                ImmutableSet.of(Label.parseAbsoluteUnchecked("//platform:constraint_1"))));
+    ImmutableMap<String, ExecGroup> execGroups =
+        getRuleContext(ct).getRule().getRuleClassObject().getExecGroups();
+    assertThat(execGroups).containsKey("watermelon");
+    ExecGroup watermelon = execGroups.get("watermelon");
+    assertThat(watermelon).hasRequiredToolchain("//rule:toolchain_type_1");
+    assertThat(watermelon).hasExecCompatibleWith("//platform:constraint_1");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/testing/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/testing/BUILD
index 28927546cb0139..7017d8b3f2b4ad 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/testing/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/testing/BUILD
@@ -16,6 +16,7 @@ java_library(
     name = "testing",
     testonly = 1,
     srcs = [
+        "ExecGroupSubject.java",
         "ResolvedToolchainContextSubject.java",
         "ToolchainCollectionSubject.java",
         "ToolchainContextSubject.java",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/testing/ExecGroupSubject.java b/src/test/java/com/google/devtools/build/lib/analysis/testing/ExecGroupSubject.java
new file mode 100644
index 00000000000000..3c49c15b09103e
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/analysis/testing/ExecGroupSubject.java
@@ -0,0 +1,78 @@
+// Copyright 2021 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.analysis.testing;
+
+import static com.google.common.truth.Truth.assertAbout;
+
+import com.google.common.truth.BooleanSubject;
+import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.IterableSubject;
+import com.google.common.truth.Subject;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.ExecGroup;
+import java.util.stream.Collectors;
+
+/** A Truth {@link Subject} for {@link ExecGroup}. */
+public class ExecGroupSubject extends Subject {
+  // Static data.
+
+  /** Entry point for test assertions related to {@link ExecGroup}. */
+  public static ExecGroupSubject assertThat(ExecGroup execGroup) {
+    return assertAbout(ExecGroupSubject::new).that(execGroup);
+  }
+
+  /** Static method for getting the subject factory (for use with assertAbout()). */
+  public static Subject.Factory<ExecGroupSubject, ExecGroup> execGroups() {
+    return ExecGroupSubject::new;
+  }
+
+  // Instance fields.
+
+  private final ExecGroup actual;
+
+  protected ExecGroupSubject(FailureMetadata failureMetadata, ExecGroup subject) {
+    super(failureMetadata, subject);
+    this.actual = subject;
+  }
+
+  public IterableSubject requiredToolchains() {
+    return check("requiredToolchainTypes()")
+        .that(actual.requiredToolchains().stream().collect(Collectors.toList()));
+  }
+
+  public void hasRequiredToolchain(String toolchainTypeLabel) {
+    hasRequiredToolchain(Label.parseAbsoluteUnchecked(toolchainTypeLabel));
+  }
+
+  public void hasRequiredToolchain(Label toolchainType) {
+    requiredToolchains().contains(toolchainType);
+  }
+
+  public IterableSubject execCompatibleWith() {
+    return check("execCompatibleWith()")
+        .that(actual.execCompatibleWith().stream().collect(Collectors.toList()));
+  }
+
+  public void hasExecCompatibleWith(String constraintLabel) {
+    hasExecCompatibleWith(Label.parseAbsoluteUnchecked(constraintLabel));
+  }
+
+  public void hasExecCompatibleWith(Label constraintLabel) {
+    execCompatibleWith().contains(constraintLabel);
+  }
+
+  public BooleanSubject isCopiedFromDefault() {
+    return check("isCopiedFromDefault()").that(actual.isCopiedFromDefault());
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/BUILD b/src/test/java/com/google/devtools/build/lib/packages/BUILD
index af46185d2c7455..0f6e95f2b8ab33 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/packages/BUILD
@@ -80,6 +80,7 @@ java_test(
         "//src/main/java/net/starlark/java/annot",
         "//src/main/java/net/starlark/java/eval",
         "//src/main/java/net/starlark/java/syntax",
+        "//src/test/java/com/google/devtools/build/lib/analysis/testing",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//src/test/java/com/google/devtools/build/lib/events:testutil",
         "//src/test/java/com/google/devtools/build/lib/testutil",
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassBuilderTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassBuilderTest.java
index 8514c316f06181..7bfebabc31d98f 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassBuilderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassBuilderTest.java
@@ -14,8 +14,8 @@
 package com.google.devtools.build.lib.packages;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.analysis.testing.ExecGroupSubject.assertThat;
 import static com.google.devtools.build.lib.packages.Attribute.attr;
-import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
 import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
 import static com.google.devtools.build.lib.packages.Type.INTEGER;
 import static com.google.devtools.build.lib.packages.Type.STRING;
@@ -218,19 +218,17 @@ public void testExecGroupsAreInherited() throws Exception {
 
   @Test
   public void testDuplicateExecGroupsThatInheritFromRuleIsOk() throws Exception {
-    Label aToolchain = Label.parseAbsoluteUnchecked("//some/toolchain");
     RuleClass a =
         new RuleClass.Builder("ruleA", RuleClassType.NORMAL, false)
             .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
-            .addExecGroups(ImmutableMap.of("blueberry", COPY_FROM_RULE_EXEC_GROUP))
+            .addExecGroups(ImmutableMap.of("blueberry", ExecGroup.copyFromDefault()))
             .add(attr("tags", STRING_LIST))
             .addRequiredToolchains(Label.parseAbsoluteUnchecked("//some/toolchain"))
             .build();
-    Label bToolchain = Label.parseAbsoluteUnchecked("//some/other/toolchain");
     RuleClass b =
         new RuleClass.Builder("ruleB", RuleClassType.NORMAL, false)
             .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
-            .addExecGroups(ImmutableMap.of("blueberry", COPY_FROM_RULE_EXEC_GROUP))
+            .addExecGroups(ImmutableMap.of("blueberry", ExecGroup.copyFromDefault()))
             .add(attr("tags", STRING_LIST))
             .addRequiredToolchains(Label.parseAbsoluteUnchecked("//some/other/toolchain"))
             .build();
@@ -239,11 +237,9 @@ public void testDuplicateExecGroupsThatInheritFromRuleIsOk() throws Exception {
         new RuleClass.Builder("$ruleC", RuleClassType.ABSTRACT, false, a, b)
             .addRequiredToolchains(cToolchain)
             .build();
-    assertThat(c.getExecGroups())
-        .containsExactly(
-            "blueberry",
-            ExecGroup.createCopied(
-                ImmutableSet.of(aToolchain, bToolchain, cToolchain), ImmutableSet.of()));
+    assertThat(c.getExecGroups()).containsKey("blueberry");
+    ExecGroup blueberry = c.getExecGroups().get("blueberry");
+    assertThat(blueberry).isCopiedFromDefault().isTrue();
   }
 
   @Test