commit 5aab75008b8c230f085331f0694dea1aa60a949f
author juliexxia <juliexxia@google.com> Fri Jun 12 12:09:25 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Fri Jun 12 12:10:30 2020 -0700
tree 5a1dade68bdd772181dab59aa8f95519d30f7063
parent fae8904c1601aab6cce6a102b282722e14ca6f53

Allow duplicate-named exec groups to be added if they're equal.

It's a problem if multiple execution groups with the same name but different toolchain or constraint requirements are added, but if those requirements are all the same, then it's fine, don't error out.

PiperOrigin-RevId: 316150971

src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

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 134b7b4..ee08ccb 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
@@ -758,7 +758,8 @@
         } catch (DuplicateExecGroupError e) {
           throw new IllegalArgumentException(
               String.format(
-                  "An execution group named '%s' is inherited multiple times in %s ruleclass",
+                  "An execution group named '%s' is inherited multiple times with different"
+                      + " requirements in %s ruleclass",
                   e.getDuplicateGroup(), name));
         }
 
@@ -1419,14 +1420,22 @@
     }
 
     /**
-     * Adds execution groups to this rule class. Errors out if multiple groups with the same name
-     * are added.
+     * Adds execution groups to this rule class. Errors out if multiple different groups with the
+     * same name are added.
      */
     public Builder addExecGroups(Map<String, ExecGroup> execGroups) throws DuplicateExecGroupError {
       for (Map.Entry<String, ExecGroup> group : execGroups.entrySet()) {
         String name = group.getKey();
-        if (this.execGroups.put(name, group.getValue()) != null) {
-          throw new DuplicateExecGroupError(name);
+        if (this.execGroups.containsKey(name)) {
+          // If trying to add a new execution group with the same name as a execution group that
+          // already exists, check if they are equivalent and error out if not.
+          ExecGroup existingGroup = this.execGroups.get(name);
+          ExecGroup newGroup = group.getValue();
+          if (!existingGroup.equals(newGroup)) {
+            throw new DuplicateExecGroupError(name);
+          }
+        } else {
+          this.execGroups.put(name, group.getValue());
         }
       }
       return this;

src/test/java/com/google/devtools/build/lib/packages/RuleClassBuilderTest.java

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 faa0cc2..ad4c470 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
@@ -215,13 +215,38 @@
   }
 
   @Test
-  public void testDuplicateExecGroupNamesErrors() throws Exception {
+  public void testDuplicateExecGroupsThatAreEqualIsOk() throws Exception {
+    ExecGroup execGroup =
+        ExecGroup.create(
+            ImmutableSet.of(Label.parseAbsoluteUnchecked("//some/toolchain")),
+            ImmutableSet.of(Label.parseAbsoluteUnchecked("//some/constraint")));
+    RuleClass a =
+        new RuleClass.Builder("ruleA", RuleClassType.NORMAL, false)
+            .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
+            .addExecGroups(ImmutableMap.of("blueberry", execGroup))
+            .add(attr("tags", STRING_LIST))
+            .build();
+    RuleClass b =
+        new RuleClass.Builder("ruleB", RuleClassType.NORMAL, false)
+            .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
+            .addExecGroups(ImmutableMap.of("blueberry", execGroup))
+            .add(attr("tags", STRING_LIST))
+            .build();
+    RuleClass c = new RuleClass.Builder("$ruleC", RuleClassType.ABSTRACT, false, a, b).build();
+    assertThat(c.getExecGroups()).containsExactly("blueberry", execGroup);
+  }
+
+  @Test
+  public void testDuplicateExecGroupsThrowsError() throws Exception {
     RuleClass a =
         new RuleClass.Builder("ruleA", RuleClassType.NORMAL, false)
             .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
             .addExecGroups(
                 ImmutableMap.of(
-                    "blueberry", ExecGroup.create(ImmutableSet.of(), ImmutableSet.of())))
+                    "blueberry",
+                    ExecGroup.create(
+                        ImmutableSet.of(Label.parseAbsoluteUnchecked("//some/toolchain")),
+                        ImmutableSet.of())))
             .add(attr("tags", STRING_LIST))
             .build();
     RuleClass b =
@@ -239,7 +264,8 @@
     assertThat(e)
         .hasMessageThat()
         .isEqualTo(
-            "An execution group named 'blueberry' is inherited multiple times in ruleC ruleclass");
+            "An execution group named 'blueberry' is inherited multiple times with different"
+                + " requirements in ruleC ruleclass");
   }
 
   @Test