From 9ad119480e15a71f33d5f58ea8c47545e0fd9311 Mon Sep 17 00:00:00 2001
From: John Cater <jcater@google.com>
Date: Mon, 7 Feb 2022 16:49:12 -0500
Subject: [PATCH] Update ExecGroup to use a Builder interface.

Also fix tests that access ExecGroup to use the correct Subject.

Part of work on optional toolchains (#14726).
---
 .../starlark/StarlarkRuleClassFunctions.java  |  6 ++-
 .../build/lib/packages/ExecGroup.java         | 49 +++++++++++++------
 .../skyframe/ConfiguredTargetFunction.java    |  6 ++-
 .../lib/packages/RuleClassBuilderTest.java    | 29 ++++++++---
 .../build/lib/packages/RuleClassTest.java     | 14 ++++--
 .../google/devtools/build/lib/starlark/BUILD  |  1 +
 .../StarlarkRuleClassFunctionsTest.java       | 19 +++----
 7 files changed, 84 insertions(+), 40 deletions(-)

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 07a3a45bce426f..923abbf0936c75 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
@@ -1054,6 +1054,10 @@ public ExecGroup execGroup(
     ImmutableSet<Label> toolchainTypes = ImmutableSet.copyOf(parseToolchains(toolchains, thread));
     ImmutableSet<Label> constraints =
         ImmutableSet.copyOf(parseExecCompatibleWith(execCompatibleWith, thread));
-    return ExecGroup.create(toolchainTypes, constraints);
+    return ExecGroup.builder()
+        .requiredToolchains(toolchainTypes)
+        .execCompatibleWith(constraints)
+        .copyFrom(null)
+        .build();
   }
 }
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 ec56d4854cfe0f..2ac62f850f6e4e 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
@@ -18,7 +18,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.starlarkbuildapi.ExecGroupApi;
-import java.util.Set;
 import javax.annotation.Nullable;
 
 /** Resolves the appropriate toolchains for the given parameters. */
@@ -29,30 +28,29 @@ public abstract class ExecGroup implements ExecGroupApi {
   // users can't create a group with the same name.
   public static final String DEFAULT_EXEC_GROUP_NAME = "default-exec-group";
 
-  /** Create an exec group that inherits from the default exec group. */
-  public static ExecGroup copyFromDefault() {
-    return create(ImmutableSet.of(), ImmutableSet.of(), /* copyFrom= */ DEFAULT_EXEC_GROUP_NAME);
-  }
-
-  /** 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, /* copyFrom= */ null);
+  /** Returns a builder for a new ExecGroup. */
+  public static Builder builder() {
+    return new AutoValue_ExecGroup.Builder()
+        .requiredToolchains(ImmutableSet.of())
+        .execCompatibleWith(ImmutableSet.of());
   }
 
-  private static ExecGroup create(
-      Set<Label> requiredToolchains, Set<Label> execCompatibleWith, @Nullable String copyFrom) {
-    return new AutoValue_ExecGroup(
-        ImmutableSet.copyOf(requiredToolchains), ImmutableSet.copyOf(execCompatibleWith), copyFrom);
+  /** Create an exec group that inherits from the default exec group. */
+  public static ExecGroup copyFromDefault() {
+    return builder().copyFrom(DEFAULT_EXEC_GROUP_NAME).build();
   }
 
+  /** Returns the required toolchain types for this exec group. */
   public abstract ImmutableSet<Label> requiredToolchains();
 
+  /** Returns the execution constraints for this exec group. */
   public abstract ImmutableSet<Label> execCompatibleWith();
 
+  /** Returns the name of another exec group in the same rule to copy data from. */
   @Nullable
   public abstract String copyFrom();
 
-  /** Creates a new exec group that inherits from the given group. */
+  /** Creates a new exec group that inherits from the given group and this group. */
   public ExecGroup inheritFrom(ExecGroup other) {
     ImmutableSet<Label> requiredToolchains =
         new ImmutableSet.Builder<Label>()
@@ -64,6 +62,27 @@ public ExecGroup inheritFrom(ExecGroup other) {
             .addAll(this.execCompatibleWith())
             .addAll(other.execCompatibleWith())
             .build();
-    return create(requiredToolchains, execCompatibleWith);
+    return builder()
+        .requiredToolchains(requiredToolchains)
+        .execCompatibleWith(execCompatibleWith)
+        .copyFrom(null)
+        .build();
+  }
+
+  /** A builder interface to create ExecGroup instances. */
+  @AutoValue.Builder
+  public interface Builder {
+
+    /** Sets the required toolchain types. */
+    Builder requiredToolchains(ImmutableSet<Label> toolchainTypes);
+
+    /** Sets the execution constraints. */
+    Builder execCompatibleWith(ImmutableSet<Label> execCompatibleWith);
+
+    /** Sets the name of another exec group in the same rule to copy from. */
+    Builder copyFrom(@Nullable String copyfrom);
+
+    /** Returns the new ExecGroup instance. */
+    ExecGroup build();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index 84d4e7c2a21e40..5dc1889ed78270 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -559,7 +559,11 @@ public static ComputedToolchainContexts computeUnloadedToolchainContexts(
 
     // Create a merged version of the exec groups that handles exec group inheritance properly.
     ExecGroup defaultExecGroup =
-        ExecGroup.create(requiredDefaultToolchains, defaultExecConstraintLabels);
+        ExecGroup.builder()
+            .requiredToolchains(requiredDefaultToolchains)
+            .execCompatibleWith(defaultExecConstraintLabels)
+            .copyFrom(null)
+            .build();
     ExecGroupCollection.Builder execGroupCollectionBuilder =
         ExecGroupCollection.builder(defaultExecGroup, rule.getRuleClassObject().getExecGroups());
 
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 13ea0b9a486880..a590861ad3a828 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
@@ -199,8 +199,17 @@ public void testExecGroupsAreInherited() throws Exception {
     Label mockToolchainType = Label.parseAbsoluteUnchecked("//mock_toolchain_type");
     Label mockConstraint = Label.parseAbsoluteUnchecked("//mock_constraint");
     ExecGroup parentGroup =
-        ExecGroup.create(ImmutableSet.of(mockToolchainType), ImmutableSet.of(mockConstraint));
-    ExecGroup childGroup = ExecGroup.create(ImmutableSet.of(), ImmutableSet.of());
+        ExecGroup.builder()
+            .requiredToolchains(ImmutableSet.of(mockToolchainType))
+            .execCompatibleWith(ImmutableSet.of(mockConstraint))
+            .copyFrom(null)
+            .build();
+    ExecGroup childGroup =
+        ExecGroup.builder()
+            .requiredToolchains(ImmutableSet.of())
+            .execCompatibleWith(ImmutableSet.of())
+            .copyFrom(null)
+            .build();
     RuleClass parent =
         new RuleClass.Builder("$parent", RuleClassType.ABSTRACT, false)
             .add(attr("tags", STRING_LIST))
@@ -249,9 +258,12 @@ public void testDuplicateExecGroupsThrowsError() throws Exception {
             .addExecGroups(
                 ImmutableMap.of(
                     "blueberry",
-                    ExecGroup.create(
-                        ImmutableSet.of(Label.parseAbsoluteUnchecked("//some/toolchain")),
-                        ImmutableSet.of())))
+                    ExecGroup.builder()
+                        .requiredToolchains(
+                            ImmutableSet.of(Label.parseAbsoluteUnchecked("//some/toolchain")))
+                        .execCompatibleWith(ImmutableSet.of())
+                        .copyFrom(null)
+                        .build()))
             .add(attr("tags", STRING_LIST))
             .build();
     RuleClass b =
@@ -259,7 +271,12 @@ public void testDuplicateExecGroupsThrowsError() throws Exception {
             .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
             .addExecGroups(
                 ImmutableMap.of(
-                    "blueberry", ExecGroup.create(ImmutableSet.of(), ImmutableSet.of())))
+                    "blueberry",
+                    ExecGroup.builder()
+                        .requiredToolchains(ImmutableSet.of())
+                        .execCompatibleWith(ImmutableSet.of())
+                        .copyFrom(null)
+                        .build()))
             .add(attr("tags", STRING_LIST))
             .build();
     IllegalArgumentException e =
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index 99e4d78425d4d1..482dfcc9d5f633 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages;
 
+import static com.google.devtools.build.lib.analysis.testing.ExecGroupSubject.assertThat;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.devtools.build.lib.packages.Attribute.attr;
@@ -1081,15 +1082,18 @@ public void testExecGroups() throws Exception {
 
     ruleClassBuilder.addExecGroups(
         ImmutableMap.of(
-            "cherry", ExecGroup.create(ImmutableSet.of(toolchain), ImmutableSet.of(constraint))));
+            "cherry",
+            ExecGroup.builder()
+                .requiredToolchains(ImmutableSet.of(toolchain))
+                .execCompatibleWith(ImmutableSet.of(constraint))
+                .copyFrom(null)
+                .build()));
 
     RuleClass ruleClass = ruleClassBuilder.build();
 
     assertThat(ruleClass.getExecGroups()).hasSize(1);
-    assertThat(ruleClass.getExecGroups().get("cherry").requiredToolchains())
-        .containsExactly(toolchain);
-    assertThat(ruleClass.getExecGroups().get("cherry").execCompatibleWith())
-        .containsExactly(constraint);
+    assertThat(ruleClass.getExecGroups().get("cherry")).hasRequiredToolchain(toolchain);
+    assertThat(ruleClass.getExecGroups().get("cherry")).hasExecCompatibleWith(constraint);
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/BUILD b/src/test/java/com/google/devtools/build/lib/starlark/BUILD
index 2889c429d45a78..d7b97a8830c623 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/starlark/BUILD
@@ -76,6 +76,7 @@ java_test(
         "//src/main/java/net/starlark/java/syntax",
         "//src/main/protobuf:failure_details_java_proto",
         "//src/test/java/com/google/devtools/build/lib/actions/util",
+        "//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/exec/util",
         "//src/test/java/com/google/devtools/build/lib/starlark/util",
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
index 5553db7684e580..614a57cd1e802a 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.starlark;
 
+import static com.google.devtools.build.lib.analysis.testing.ExecGroupSubject.assertThat;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
@@ -2337,13 +2338,10 @@ public void testRuleAddExecGroup() throws Exception {
         ")");
     RuleClass plum = ((StarlarkRuleFunction) ev.lookup("plum")).getRuleClass();
     assertThat(plum.getRequiredToolchains()).isEmpty();
-    assertThat(plum.getExecGroups().get("group").requiredToolchains())
-        .containsExactly(Label.parseAbsoluteUnchecked("//test:my_toolchain_type"));
+    assertThat(plum.getExecGroups().get("group")).hasRequiredToolchain("//test:my_toolchain_type");
     assertThat(plum.getExecutionPlatformConstraints()).isEmpty();
-    assertThat(plum.getExecGroups().get("group").execCompatibleWith())
-        .containsExactly(
-            Label.parseAbsoluteUnchecked("//constraint:cv1"),
-            Label.parseAbsoluteUnchecked("//constraint:cv2"));
+    assertThat(plum.getExecGroups().get("group")).hasExecCompatibleWith("//constraint:cv1");
+    assertThat(plum.getExecGroups().get("group")).hasExecCompatibleWith("//constraint:cv2");
   }
 
   @Test
@@ -2390,12 +2388,9 @@ public void testCreateExecGroup() throws Exception {
         "  exec_compatible_with=['//constraint:cv1', '//constraint:cv2'],",
         ")");
     ExecGroup group = ((ExecGroup) ev.lookup("group"));
-    assertThat(group.requiredToolchains())
-        .containsExactly(Label.parseAbsoluteUnchecked("//test:my_toolchain_type"));
-    assertThat(group.execCompatibleWith())
-        .containsExactly(
-            Label.parseAbsoluteUnchecked("//constraint:cv1"),
-            Label.parseAbsoluteUnchecked("//constraint:cv2"));
+    assertThat(group).hasRequiredToolchain("//test:my_toolchain_type");
+    assertThat(group).hasExecCompatibleWith("//constraint:cv1");
+    assertThat(group).hasExecCompatibleWith("//constraint:cv2");
   }
 
   @Test