Skip to content

Commit

Permalink
Remove the unused concept of exec group inheritance
Browse files Browse the repository at this point in the history
  • Loading branch information
fmeum committed Dec 15, 2024
1 parent 48fe861 commit da7755b
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;
import static java.util.stream.Collectors.joining;

import com.google.auto.value.AutoValue;
import com.google.common.collect.HashBasedTable;
Expand Down Expand Up @@ -48,8 +47,7 @@ public abstract class ExecGroupCollection {
/**
* Prepares the input exec groups to serve as {@link Builder#execGroups}.
*
* <p>Applies any inheritance specified via {@link ExecGroup#copyFrom} and adds auto exec groups
* when {@code useAutoExecGroups} is true.
* <p>Adds auto exec groups when {@code useAutoExecGroups} is true.
*/
public static ImmutableMap<String, ExecGroup> process(
ImmutableMap<String, ExecGroup> execGroups,
Expand All @@ -65,16 +63,12 @@ public static ImmutableMap<String, ExecGroup> process(
String name = entry.getKey();
ExecGroup execGroup = entry.getValue();

if (execGroup.copyFrom() != null) {
if (execGroup.copyFrom().equals(DEFAULT_EXEC_GROUP_NAME)) {
if (execGroup.copyFromDefault()) {
execGroup =
ExecGroup.builder()
.execCompatibleWith(defaultExecWith)
.toolchainTypes(defaultToolchainTypes)
.build();
} else {
execGroup = execGroup.inheritFrom(execGroups.get(execGroup.copyFrom()));
}
}

processedGroups.put(name, execGroup);
Expand All @@ -87,7 +81,6 @@ public static ImmutableMap<String, ExecGroup> process(
toolchainType.toolchainType().toString(),
ExecGroup.builder()
.addToolchainType(toolchainType)
.copyFrom(null)
.execCompatibleWith(defaultExecWith)
.build());
}
Expand Down Expand Up @@ -268,7 +261,7 @@ public InvalidExecGroupException(Collection<String> invalidNames) {
super(
String.format(
"Tried to set properties for non-existent exec groups: %s.",
invalidNames.stream().collect(joining(","))));
String.join(",", invalidNames)));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -602,9 +602,10 @@ public StarlarkRuleFunction rule(
/**
* Returns a new callable representing a Starlark-defined rule.
*
* <p>This is public for the benefit of {@link StarlarkTestingModule}, which has the unusual use
* case of creating new rule types to house analysis-time test assertions ({@code analysis_test}).
* It's probably not a good idea to add new callers of this method.
* <p>This is public for the benefit of {@link
* com.google.devtools.build.lib.rules.test.StarlarkTestingModule}, which has the unusual use case
* of creating new rule types to house analysis-time test assertions ({@code analysis_test}). It's
* probably not a good idea to add new callers of this method.
*
* <p>Note that the bzlFile and transitiveDigest params correspond to the outermost .bzl file
* being evaluated, not the one in which rule() is called.
Expand Down Expand Up @@ -1989,7 +1990,6 @@ public ExecGroup execGroup(
return ExecGroup.builder()
.toolchainTypes(toolchainTypes)
.execCompatibleWith(constraints)
.copyFrom(null)
.build();
}

Expand Down
36 changes: 12 additions & 24 deletions src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.build.lib.packages;

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;

import com.google.auto.value.AutoBuilder;
Expand All @@ -31,35 +32,37 @@
*
* @param toolchainTypesMap Returns the underlying map from label to ToolchainTypeRequirement.
* @param execCompatibleWith Returns the execution constraints for this exec group.
* @param copyFrom Returns the name of another exec group in the same rule to copy data from.
* @param copyFromDefault Whether this exec group should copy the data from the default exec group
* in the same rule.
*/
@AutoCodec
public record ExecGroup(
ImmutableMap<Label, ToolchainTypeRequirement> toolchainTypesMap,
ImmutableSet<Label> execCompatibleWith,
@Nullable String copyFrom)
boolean copyFromDefault)
implements ExecGroupApi {
public ExecGroup {
requireNonNull(toolchainTypesMap, "toolchainTypesMap");
requireNonNull(execCompatibleWith, "execCompatibleWith");
checkArgument(
!copyFromDefault || (toolchainTypesMap.isEmpty() && execCompatibleWith.isEmpty()));
}

// This is intentionally a string that would fail {@code Identifier.isValid} so that
// users can't create a group with the same name.
public static final String DEFAULT_EXEC_GROUP_NAME = "default-exec-group";

/** An exec group that copies all data from the default exec group. */
public static final ExecGroup COPY_FROM_DEFAULT = builder().copyFromDefault(true).build();

/** Returns a builder for a new ExecGroup. */
public static Builder builder() {
return new AutoBuilder_ExecGroup_Builder()
.copyFromDefault(false)
.toolchainTypes(ImmutableSet.of())
.execCompatibleWith(ImmutableSet.of());
}

/** 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 ImmutableSet<ToolchainTypeRequirement> toolchainTypes() {
return ImmutableSet.copyOf(toolchainTypesMap().values());
Expand All @@ -70,21 +73,6 @@ public ToolchainTypeRequirement toolchainType(Label label) {
return toolchainTypesMap().get(label);
}

/** Creates a new exec group that inherits from the given group and this group. */
public ExecGroup inheritFrom(ExecGroup other) {
Builder builder = builder().copyFrom(null);
builder.toolchainTypesMapBuilder().putAll(this.toolchainTypesMap());
builder.toolchainTypesMapBuilder().putAll(other.toolchainTypesMap());

builder.execCompatibleWith(
new ImmutableSet.Builder<Label>()
.addAll(this.execCompatibleWith())
.addAll(other.execCompatibleWith())
.build());

return builder.build();
}

/** A builder interface to create ExecGroup instances. */
@AutoBuilder
public interface Builder {
Expand All @@ -108,8 +96,8 @@ default Builder addToolchainType(ToolchainTypeRequirement toolchainTypeRequireme
/** 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);
/** Do not call, internal usage only. */
Builder copyFromDefault(boolean copyFromDefault);

/** Returns the new ExecGroup instance. */
ExecGroup build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1624,7 +1624,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, ExecGroup.copyFromDefault()));
return addExecGroups(ImmutableMap.of(name, ExecGroup.COPY_FROM_DEFAULT));
}

/** 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 @@ -205,13 +205,11 @@ public void testExecGroupsAreInherited() throws Exception {
ExecGroup.builder()
.addToolchainType(ToolchainTypeRequirement.create(mockToolchainType))
.execCompatibleWith(ImmutableSet.of(mockConstraint))
.copyFrom(null)
.build();
ExecGroup childGroup =
ExecGroup.builder()
.toolchainTypes(ImmutableSet.of())
.execCompatibleWith(ImmutableSet.of())
.copyFrom(null)
.build();
RuleClass parent =
new RuleClass.Builder("$parent", RuleClassType.ABSTRACT, false)
Expand All @@ -233,15 +231,15 @@ public void testDuplicateExecGroupsThatInheritFromRuleIsOk() throws Exception {
RuleClass a =
new RuleClass.Builder("ruleA", RuleClassType.NORMAL, false)
.factory(DUMMY_CONFIGURED_TARGET_FACTORY)
.addExecGroups(ImmutableMap.of("blueberry", ExecGroup.copyFromDefault()))
.addExecGroups(ImmutableMap.of("blueberry", ExecGroup.COPY_FROM_DEFAULT))
.add(attr("tags", STRING_LIST))
.addToolchainTypes(
ToolchainTypeRequirement.create(Label.parseCanonicalUnchecked("//some/toolchain")))
.build();
RuleClass b =
new RuleClass.Builder("ruleB", RuleClassType.NORMAL, false)
.factory(DUMMY_CONFIGURED_TARGET_FACTORY)
.addExecGroups(ImmutableMap.of("blueberry", ExecGroup.copyFromDefault()))
.addExecGroups(ImmutableMap.of("blueberry", ExecGroup.COPY_FROM_DEFAULT))
.add(attr("tags", STRING_LIST))
.addToolchainTypes(
ToolchainTypeRequirement.create(
Expand Down Expand Up @@ -271,7 +269,6 @@ public void testDuplicateExecGroupsThrowsError() throws Exception {
ToolchainTypeRequirement.create(
Label.parseCanonicalUnchecked("//some/toolchain")))
.execCompatibleWith(ImmutableSet.of())
.copyFrom(null)
.build()))
.add(attr("tags", STRING_LIST))
.build();
Expand All @@ -284,7 +281,6 @@ public void testDuplicateExecGroupsThrowsError() throws Exception {
ExecGroup.builder()
.toolchainTypes(ImmutableSet.of())
.execCompatibleWith(ImmutableSet.of())
.copyFrom(null)
.build()))
.add(attr("tags", STRING_LIST))
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1206,7 +1206,6 @@ public void testExecGroups() {
ExecGroup.builder()
.addToolchainType(ToolchainTypeRequirement.create(toolchain))
.execCompatibleWith(ImmutableSet.of(constraint))
.copyFrom(null)
.build()));

RuleClass ruleClass = ruleClassBuilder.build();
Expand Down

0 comments on commit da7755b

Please sign in to comment.