Skip to content

Commit

Permalink
Update creating exec groups that explicitly copy from defaults.
Browse files Browse the repository at this point in the history
Also add an ExecGroupSubject to improve testing.

PiperOrigin-RevId: 372387338
  • Loading branch information
katre committed Jul 13, 2021
1 parent 58a6fb1 commit 7d5493d
Show file tree
Hide file tree
Showing 8 changed files with 106 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -56,5 +58,5 @@ private static ExecGroup create(

public abstract ImmutableSet<Label> execCompatibleWith();

public abstract boolean copyFromRule();
public abstract boolean isCopiedFromDefault();
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ java_library(
name = "testing",
testonly = 1,
srcs = [
"ExecGroupSubject.java",
"ResolvedToolchainContextSubject.java",
"ToolchainCollectionSubject.java",
"ToolchainContextSubject.java",
Expand Down
Original file line number Diff line number Diff line change
@@ -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());
}
}
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/packages/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down

0 comments on commit 7d5493d

Please sign in to comment.