Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ExecGroup to use a Builder interface. #14758

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
49 changes: 34 additions & 15 deletions src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -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>()
Expand All @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -249,17 +258,25 @@ 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 =
new RuleClass.Builder("ruleB", RuleClassType.NORMAL, false)
.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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/starlark/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down