Skip to content

Commit 7d5493d

Browse files
katrekatre
committed
Update creating exec groups that explicitly copy from defaults.
Also add an ExecGroupSubject to improve testing. PiperOrigin-RevId: 372387338
1 parent 58a6fb1 commit 7d5493d

File tree

8 files changed

+106
-29
lines changed

8 files changed

+106
-29
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -933,7 +933,7 @@ public ExecGroup execGroup(
933933
throw Starlark.errorf(
934934
"An exec group cannot set copy_from_rule=True and declare toolchains or constraints.");
935935
}
936-
return ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
936+
return ExecGroup.copyFromDefault();
937937
}
938938

939939
ImmutableSet<Label> toolchainTypes = ImmutableSet.copyOf(parseToolchains(toolchains, thread));

‎src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,23 @@
2525
@AutoValue
2626
public abstract class ExecGroup implements ExecGroupApi {
2727

28-
// An exec group that inherits requirements from the rule.
29-
public static final ExecGroup COPY_FROM_RULE_EXEC_GROUP =
30-
createCopied(ImmutableSet.of(), ImmutableSet.of());
31-
3228
// This is intentionally a string that would fail {@code Identifier.isValid} so that
3329
// users can't create a group with the same name.
3430
@VisibleForTesting public static final String DEFAULT_EXEC_GROUP_NAME = "default-exec-group";
3531

3632
// Create an exec group that is marked as copying from the rule.
33+
// TODO(b/183268405): Remove this when RuleClass is updated to better handle inheritance.
3734
public static ExecGroup createCopied(
3835
Set<Label> requiredToolchains, Set<Label> execCompatibleWith) {
3936
return create(requiredToolchains, execCompatibleWith, /* copyFromRule= */ true);
4037
}
4138

42-
// Create an exec group.
39+
/** Create an exec group that inherits from the default exec group. */
40+
public static ExecGroup copyFromDefault() {
41+
return create(ImmutableSet.of(), ImmutableSet.of(), /* copyFromRule= */ true);
42+
}
43+
44+
/** Create an exec group with the given toolchains and execution constraints. */
4345
public static ExecGroup create(Set<Label> requiredToolchains, Set<Label> execCompatibleWith) {
4446
return create(requiredToolchains, execCompatibleWith, /* copyFromRule= */ false);
4547
}
@@ -56,5 +58,5 @@ private static ExecGroup create(
5658

5759
public abstract ImmutableSet<Label> execCompatibleWith();
5860

59-
public abstract boolean copyFromRule();
61+
public abstract boolean isCopiedFromDefault();
6062
}

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
import static com.google.devtools.build.lib.packages.Attribute.attr;
1818
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
19-
import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
2019
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
2120

2221
import com.google.common.annotations.VisibleForTesting;
@@ -772,8 +771,8 @@ public Builder(String name, RuleClassType type, boolean starlark, RuleClass... p
772771
// we need to clear out whatever toolchains and constraints have been copied from the rule
773772
// in order to prevent clashing and fill with the the child's toolchain and constraints.
774773
for (Map.Entry<String, ExecGroup> execGroup : parent.getExecGroups().entrySet()) {
775-
if (execGroup.getValue().copyFromRule()) {
776-
cleanedExecGroups.put(execGroup.getKey(), COPY_FROM_RULE_EXEC_GROUP);
774+
if (execGroup.getValue().isCopiedFromDefault()) {
775+
cleanedExecGroups.put(execGroup.getKey(), ExecGroup.copyFromDefault());
777776
} else {
778777
cleanedExecGroups.put(execGroup);
779778
}
@@ -871,7 +870,7 @@ public RuleClass build(String name, String key) {
871870
ExecGroup copiedFromRule = null;
872871
for (Map.Entry<String, ExecGroup> groupEntry : execGroups.entrySet()) {
873872
ExecGroup group = groupEntry.getValue();
874-
if (group.copyFromRule()) {
873+
if (group.isCopiedFromDefault()) {
875874
if (copiedFromRule == null) {
876875
copiedFromRule =
877876
ExecGroup.createCopied(requiredToolchains, executionPlatformConstraints);
@@ -1485,7 +1484,7 @@ public Builder addExecGroups(Map<String, ExecGroup> execGroups) {
14851484

14861485
/** Adds an exec group that copies its toolchains and constraints from the rule. */
14871486
public Builder addExecGroup(String name) {
1488-
return addExecGroups(ImmutableMap.of(name, COPY_FROM_RULE_EXEC_GROUP));
1487+
return addExecGroups(ImmutableMap.of(name, ExecGroup.copyFromDefault()));
14891488
}
14901489

14911490
/** An error to help report {@link ExecGroup}s with the same name */

‎src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@
1515
package com.google.devtools.build.lib.analysis;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static com.google.devtools.build.lib.analysis.testing.ExecGroupSubject.assertThat;
1819
import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
1920

2021
import com.google.common.collect.ImmutableMap;
21-
import com.google.common.collect.ImmutableSet;
2222
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
2323
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
2424
import com.google.devtools.build.lib.cmdline.Label;
@@ -388,12 +388,12 @@ public void testInheritsRuleRequirements() throws Exception {
388388
scratch.file("test/BUILD", "load('//test:defs.bzl', 'my_rule')", "my_rule(name = 'papaya')");
389389

390390
ConfiguredTarget ct = getConfiguredTarget("//test:papaya");
391-
assertThat(getRuleContext(ct).getRule().getRuleClassObject().getExecGroups())
392-
.containsExactly(
393-
"watermelon",
394-
ExecGroup.createCopied(
395-
ImmutableSet.of(Label.parseAbsoluteUnchecked("//rule:toolchain_type_1")),
396-
ImmutableSet.of(Label.parseAbsoluteUnchecked("//platform:constraint_1"))));
391+
ImmutableMap<String, ExecGroup> execGroups =
392+
getRuleContext(ct).getRule().getRuleClassObject().getExecGroups();
393+
assertThat(execGroups).containsKey("watermelon");
394+
ExecGroup watermelon = execGroups.get("watermelon");
395+
assertThat(watermelon).hasRequiredToolchain("//rule:toolchain_type_1");
396+
assertThat(watermelon).hasExecCompatibleWith("//platform:constraint_1");
397397
}
398398

399399
@Test

‎src/test/java/com/google/devtools/build/lib/analysis/testing/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ java_library(
1616
name = "testing",
1717
testonly = 1,
1818
srcs = [
19+
"ExecGroupSubject.java",
1920
"ResolvedToolchainContextSubject.java",
2021
"ToolchainCollectionSubject.java",
2122
"ToolchainContextSubject.java",

‎src/test/java/com/google/devtools/build/lib/analysis/testing/ExecGroupSubject.java

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// Copyright 2021 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.analysis.testing;
15+
16+
import static com.google.common.truth.Truth.assertAbout;
17+
18+
import com.google.common.truth.BooleanSubject;
19+
import com.google.common.truth.FailureMetadata;
20+
import com.google.common.truth.IterableSubject;
21+
import com.google.common.truth.Subject;
22+
import com.google.devtools.build.lib.cmdline.Label;
23+
import com.google.devtools.build.lib.packages.ExecGroup;
24+
import java.util.stream.Collectors;
25+
26+
/** A Truth {@link Subject} for {@link ExecGroup}. */
27+
public class ExecGroupSubject extends Subject {
28+
// Static data.
29+
30+
/** Entry point for test assertions related to {@link ExecGroup}. */
31+
public static ExecGroupSubject assertThat(ExecGroup execGroup) {
32+
return assertAbout(ExecGroupSubject::new).that(execGroup);
33+
}
34+
35+
/** Static method for getting the subject factory (for use with assertAbout()). */
36+
public static Subject.Factory<ExecGroupSubject, ExecGroup> execGroups() {
37+
return ExecGroupSubject::new;
38+
}
39+
40+
// Instance fields.
41+
42+
private final ExecGroup actual;
43+
44+
protected ExecGroupSubject(FailureMetadata failureMetadata, ExecGroup subject) {
45+
super(failureMetadata, subject);
46+
this.actual = subject;
47+
}
48+
49+
public IterableSubject requiredToolchains() {
50+
return check("requiredToolchainTypes()")
51+
.that(actual.requiredToolchains().stream().collect(Collectors.toList()));
52+
}
53+
54+
public void hasRequiredToolchain(String toolchainTypeLabel) {
55+
hasRequiredToolchain(Label.parseAbsoluteUnchecked(toolchainTypeLabel));
56+
}
57+
58+
public void hasRequiredToolchain(Label toolchainType) {
59+
requiredToolchains().contains(toolchainType);
60+
}
61+
62+
public IterableSubject execCompatibleWith() {
63+
return check("execCompatibleWith()")
64+
.that(actual.execCompatibleWith().stream().collect(Collectors.toList()));
65+
}
66+
67+
public void hasExecCompatibleWith(String constraintLabel) {
68+
hasExecCompatibleWith(Label.parseAbsoluteUnchecked(constraintLabel));
69+
}
70+
71+
public void hasExecCompatibleWith(Label constraintLabel) {
72+
execCompatibleWith().contains(constraintLabel);
73+
}
74+
75+
public BooleanSubject isCopiedFromDefault() {
76+
return check("isCopiedFromDefault()").that(actual.isCopiedFromDefault());
77+
}
78+
}

‎src/test/java/com/google/devtools/build/lib/packages/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ java_test(
8080
"//src/main/java/net/starlark/java/annot",
8181
"//src/main/java/net/starlark/java/eval",
8282
"//src/main/java/net/starlark/java/syntax",
83+
"//src/test/java/com/google/devtools/build/lib/analysis/testing",
8384
"//src/test/java/com/google/devtools/build/lib/analysis/util",
8485
"//src/test/java/com/google/devtools/build/lib/events:testutil",
8586
"//src/test/java/com/google/devtools/build/lib/testutil",

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

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414
package com.google.devtools.build.lib.packages;
1515

1616
import static com.google.common.truth.Truth.assertThat;
17+
import static com.google.devtools.build.lib.analysis.testing.ExecGroupSubject.assertThat;
1718
import static com.google.devtools.build.lib.packages.Attribute.attr;
18-
import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
1919
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
2020
import static com.google.devtools.build.lib.packages.Type.INTEGER;
2121
import static com.google.devtools.build.lib.packages.Type.STRING;
@@ -218,19 +218,17 @@ public void testExecGroupsAreInherited() throws Exception {
218218

219219
@Test
220220
public void testDuplicateExecGroupsThatInheritFromRuleIsOk() throws Exception {
221-
Label aToolchain = Label.parseAbsoluteUnchecked("//some/toolchain");
222221
RuleClass a =
223222
new RuleClass.Builder("ruleA", RuleClassType.NORMAL, false)
224223
.factory(DUMMY_CONFIGURED_TARGET_FACTORY)
225-
.addExecGroups(ImmutableMap.of("blueberry", COPY_FROM_RULE_EXEC_GROUP))
224+
.addExecGroups(ImmutableMap.of("blueberry", ExecGroup.copyFromDefault()))
226225
.add(attr("tags", STRING_LIST))
227226
.addRequiredToolchains(Label.parseAbsoluteUnchecked("//some/toolchain"))
228227
.build();
229-
Label bToolchain = Label.parseAbsoluteUnchecked("//some/other/toolchain");
230228
RuleClass b =
231229
new RuleClass.Builder("ruleB", RuleClassType.NORMAL, false)
232230
.factory(DUMMY_CONFIGURED_TARGET_FACTORY)
233-
.addExecGroups(ImmutableMap.of("blueberry", COPY_FROM_RULE_EXEC_GROUP))
231+
.addExecGroups(ImmutableMap.of("blueberry", ExecGroup.copyFromDefault()))
234232
.add(attr("tags", STRING_LIST))
235233
.addRequiredToolchains(Label.parseAbsoluteUnchecked("//some/other/toolchain"))
236234
.build();
@@ -239,11 +237,9 @@ public void testDuplicateExecGroupsThatInheritFromRuleIsOk() throws Exception {
239237
new RuleClass.Builder("$ruleC", RuleClassType.ABSTRACT, false, a, b)
240238
.addRequiredToolchains(cToolchain)
241239
.build();
242-
assertThat(c.getExecGroups())
243-
.containsExactly(
244-
"blueberry",
245-
ExecGroup.createCopied(
246-
ImmutableSet.of(aToolchain, bToolchain, cToolchain), ImmutableSet.of()));
240+
assertThat(c.getExecGroups()).containsKey("blueberry");
241+
ExecGroup blueberry = c.getExecGroups().get("blueberry");
242+
assertThat(blueberry).isCopiedFromDefault().isTrue();
247243
}
248244

249245
@Test

0 commit comments

Comments
 (0)