From 8c6382a81237e72dbec24b3850df9481461e0015 Mon Sep 17 00:00:00 2001 From: jcater Date: Wed, 12 May 2021 09:35:48 -0700 Subject: [PATCH] Create a new ExecGroupCollection container to manage exec group inheritance and exec property parsing. Fixes #13459. PiperOrigin-RevId: 373388266 --- .../google/devtools/build/lib/analysis/BUILD | 19 +- .../lib/analysis/ConfiguredTargetFactory.java | 13 +- .../lib/analysis/ExecGroupCollection.java | 267 ++++++++++++++++++ .../build/lib/analysis/RuleContext.java | 187 +++--------- .../google/devtools/build/lib/packages/BUILD | 1 + .../build/lib/packages/ExecGroup.java | 39 +-- .../build/lib/packages/RuleClass.java | 39 +-- .../build/lib/skyframe/AspectFunction.java | 2 +- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/ConfiguredTargetFunction.java | 58 +++- .../build/lib/skyframe/SkyframeBuildView.java | 9 +- .../google/devtools/build/lib/analysis/BUILD | 1 + .../lib/analysis/StarlarkExecGroupTest.java | 14 +- .../devtools/build/lib/analysis/testing/BUILD | 2 + .../testing/ExecGroupCollectionSubject.java | 56 ++++ .../analysis/testing/ExecGroupSubject.java | 10 +- .../devtools/build/lib/analysis/util/BUILD | 3 +- .../analysis/util/BuildViewForTesting.java | 11 +- .../lib/packages/RuleClassBuilderTest.java | 5 +- .../skyframe/ToolchainsForTargetsTest.java | 10 +- src/test/shell/integration/exec_group_test.sh | 238 +++++++++++++++- 21 files changed, 735 insertions(+), 250 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/ExecGroupCollection.java create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/testing/ExecGroupCollectionSubject.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index bc5842d291068e..30510adab44cbd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -324,6 +324,7 @@ java_library( ":dependency_key", ":dependency_kind", ":duplicate_exception", + ":exec_group_collection", ":extra/extra_action_info_file_write_action", ":extra_action_artifacts_provider", ":file_provider", @@ -418,7 +419,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:package_value", "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value", - "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception", "//src/main/java/com/google/devtools/build/lib/skyframe:toolchain_context_key", "//src/main/java/com/google/devtools/build/lib/skyframe:transitive_target_key", "//src/main/java/com/google/devtools/build/lib/skyframe:transitive_target_value", @@ -758,6 +758,23 @@ java_library( srcs = ["DuplicateException.java"], ) +java_library( + name = "exec_group_collection", + srcs = ["ExecGroupCollection.java"], + deps = [ + ":resolved_toolchain_context", + ":toolchain_collection", + "//src/main/java/com/google/devtools/build/lib/analysis/platform", + "//src/main/java/com/google/devtools/build/lib/packages:exec_group", + "//src/main/java/com/google/devtools/build/lib/skyframe:sane_analysis_exception", + "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code", + "//src/main/protobuf:failure_details_java_proto", + "//third_party:auto_value", + "//third_party:guava", + "//third_party:jsr305", + ], +) + java_library( name = "extra_action_artifacts_provider", srcs = ["ExtraActionArtifactsProvider.java"], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 5ff697252db34b..ffec432bba5688 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java @@ -25,7 +25,7 @@ import com.google.devtools.build.lib.actions.ArtifactFactory; import com.google.devtools.build.lib.actions.FailAction; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; -import com.google.devtools.build.lib.analysis.RuleContext.InvalidExecGroupException; +import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.analysis.config.Fragment; @@ -186,7 +186,8 @@ public final ConfiguredTarget createConfiguredTarget( ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap prerequisiteMap, ImmutableMap configConditions, - @Nullable ToolchainCollection toolchainContexts) + @Nullable ToolchainCollection toolchainContexts, + ExecGroupCollection.Builder execGroupCollectionBuilder) throws InterruptedException, ActionConflictException, InvalidExecGroupException { if (target instanceof Rule) { try { @@ -199,7 +200,8 @@ public final ConfiguredTarget createConfiguredTarget( configuredTargetKey, prerequisiteMap, configConditions, - toolchainContexts); + toolchainContexts, + execGroupCollectionBuilder); } finally { CurrentRuleTracker.endConfiguredTarget(); } @@ -292,7 +294,8 @@ private ConfiguredTarget createRule( ConfiguredTargetKey configuredTargetKey, OrderedSetMultimap prerequisiteMap, ImmutableMap configConditions, - @Nullable ToolchainCollection toolchainContexts) + @Nullable ToolchainCollection toolchainContexts, + ExecGroupCollection.Builder execGroupCollectionBuilder) throws InterruptedException, ActionConflictException, InvalidExecGroupException { ConfigurationFragmentPolicy configurationFragmentPolicy = rule.getRuleClassObject().getConfigurationFragmentPolicy(); @@ -312,6 +315,7 @@ private ConfiguredTarget createRule( .setConfigConditions(configConditions) .setUniversalFragments(ruleClassProvider.getUniversalFragments()) .setToolchainContexts(toolchainContexts) + .setExecGroupCollectionBuilder(execGroupCollectionBuilder) .setConstraintSemantics(ruleClassProvider.getConstraintSemantics()) .setRequiredConfigFragments( RequiredFragmentsUtil.getRequiredFragments( @@ -527,6 +531,7 @@ public ConfiguredAspect createAspect( .setToolchainContext(toolchainContext) // TODO(b/161222568): Implement the exec_properties attr for aspects and read its value // here. + .setExecGroupCollectionBuilder(ExecGroupCollection.emptyBuilder()) .setExecProperties(ImmutableMap.of()) .setConstraintSemantics(ruleClassProvider.getConstraintSemantics()) .setRequiredConfigFragments( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ExecGroupCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/ExecGroupCollection.java new file mode 100644 index 00000000000000..1a42e456e08eba --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/ExecGroupCollection.java @@ -0,0 +1,267 @@ +// 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; + +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; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableTable; +import com.google.common.collect.Table; +import com.google.devtools.build.lib.analysis.platform.PlatformInfo; +import com.google.devtools.build.lib.packages.ExecGroup; +import com.google.devtools.build.lib.server.FailureDetails.Analysis; +import com.google.devtools.build.lib.server.FailureDetails.Analysis.Code; +import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.skyframe.SaneAnalysisException; +import com.google.devtools.build.lib.util.DetailedExitCode; +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.Map; +import javax.annotation.Nullable; + +/** + * A container class for groups of {@link ExecGroup} instances. This correctly handles exec group + * inheritance between rules and targets. See + * https://docs.bazel.build/versions/master/exec-groups.html for further details. + */ +@AutoValue +public abstract class ExecGroupCollection { + + public static Builder emptyBuilder() { + return new AutoValue_ExecGroupCollection_Builder(ImmutableMap.of()); + } + + public static Builder builder( + ExecGroup defaultExecGroup, ImmutableMap execGroups) { + // Post-process the groups to handle inheritance. + Map processedGroups = new LinkedHashMap<>(); + for (Map.Entry entry : execGroups.entrySet()) { + String name = entry.getKey(); + ExecGroup execGroup = entry.getValue(); + + if (execGroup.copyFrom() != null) { + if (execGroup.copyFrom().equals(DEFAULT_EXEC_GROUP_NAME)) { + execGroup = execGroup.inheritFrom(defaultExecGroup); + } else { + execGroup = execGroup.inheritFrom(execGroups.get(execGroup.copyFrom())); + } + } + + processedGroups.put(name, execGroup); + } + + return new AutoValue_ExecGroupCollection_Builder(ImmutableMap.copyOf(processedGroups)); + } + + /** Builder class for correctly constructing ExecGroupCollection instances. */ + // Note that this is _not_ an actual @AutoValue.Builder: it provides more logic and has different + // fields. + @AutoValue + public abstract static class Builder { + public abstract ImmutableMap execGroups(); + + public ImmutableSet getExecGroupNames() { + return execGroups().keySet(); + } + + public ExecGroup getExecGroup(String name) { + return execGroups().get(name); + } + + public ExecGroupCollection build( + @Nullable ToolchainCollection toolchainContexts, + ImmutableMap rawExecProperties) + throws InvalidExecGroupException { + + // For each exec group, compute the combined execution properties. + ImmutableTable combinedExecProperties = + computeCombinedExecProperties(toolchainContexts, rawExecProperties); + + return new AutoValue_ExecGroupCollection(execGroups(), combinedExecProperties); + } + } + + /** + * Gets the combined exec properties of the platform and the target's exec properties. If a + * property is set in both, the target properties take precedence. + */ + private static ImmutableTable computeCombinedExecProperties( + @Nullable ToolchainCollection toolchainContexts, + ImmutableMap rawExecProperties) + throws InvalidExecGroupException { + + ImmutableSet execGroupNames; + if (toolchainContexts == null) { + execGroupNames = ImmutableSet.of(DEFAULT_EXEC_GROUP_NAME); + } else { + execGroupNames = toolchainContexts.getExecGroupNames(); + } + + // Parse the target-level exec properties. + ImmutableTable parsedTargetProperties = + parseExecProperties(rawExecProperties); + // Validate the exec group names in the properties. + if (toolchainContexts != null) { + ImmutableSet unknownTargetExecGroupNames = + parsedTargetProperties.rowKeySet().stream() + .filter(name -> !name.equals(DEFAULT_EXEC_GROUP_NAME)) + .filter(name -> !execGroupNames.contains(name)) + .collect(toImmutableSet()); + if (!unknownTargetExecGroupNames.isEmpty()) { + throw new InvalidExecGroupException(unknownTargetExecGroupNames); + } + } + + // Parse each execution platform's exec properties. + ImmutableSet executionPlatforms; + if (toolchainContexts == null) { + executionPlatforms = ImmutableSet.of(); + } else { + executionPlatforms = + execGroupNames.stream() + .map(name -> toolchainContexts.getToolchainContext(name).executionPlatform()) + .distinct() + .collect(toImmutableSet()); + } + Map> parsedPlatformProperties = + new LinkedHashMap<>(); + for (PlatformInfo executionPlatform : executionPlatforms) { + ImmutableTable parsed = + parseExecProperties(executionPlatform.execProperties()); + parsedPlatformProperties.put(executionPlatform, parsed); + } + + // First, get the defaults. + ImmutableMap defaultExecProperties = + parsedTargetProperties.row(DEFAULT_EXEC_GROUP_NAME); + Table result = HashBasedTable.create(); + putAll(result, DEFAULT_EXEC_GROUP_NAME, defaultExecProperties); + + for (String execGroupName : execGroupNames) { + ImmutableMap combined = + computeProperties( + execGroupName, + defaultExecProperties, + toolchainContexts, + parsedPlatformProperties, + parsedTargetProperties); + putAll(result, execGroupName, combined); + } + + return ImmutableTable.copyOf(result); + } + + private static void putAll(Table builder, R row, Map values) { + for (Map.Entry entry : values.entrySet()) { + builder.put(row, entry.getKey(), entry.getValue()); + } + } + + private static ImmutableMap computeProperties( + String execGroupName, + ImmutableMap defaultExecProperties, + @Nullable ToolchainCollection toolchainContexts, + Map> parsedPlatformProperties, + ImmutableTable parsedTargetProperties) { + + ImmutableMap defaultExecGroupPlatformProperties; + ImmutableMap platformProperties; + if (toolchainContexts == null) { + defaultExecGroupPlatformProperties = ImmutableMap.of(); + platformProperties = ImmutableMap.of(); + } else { + PlatformInfo executionPlatform = + toolchainContexts.getToolchainContext(execGroupName).executionPlatform(); + defaultExecGroupPlatformProperties = + parsedPlatformProperties.get(executionPlatform).row(DEFAULT_EXEC_GROUP_NAME); + platformProperties = parsedPlatformProperties.get(executionPlatform).row(execGroupName); + } + Map targetProperties = + new LinkedHashMap<>(parsedTargetProperties.row(execGroupName)); + for (String propertyName : defaultExecProperties.keySet()) { + // If the property exists in the default and not in the target, copy it. + targetProperties.computeIfAbsent(propertyName, defaultExecProperties::get); + } + + // Combine the target and exec platform properties. Target properties take precedence. + // Use a HashMap instead of an ImmutableMap.Builder because we expect duplicate keys. + Map combined = new LinkedHashMap<>(); + combined.putAll(defaultExecGroupPlatformProperties); + combined.putAll(defaultExecProperties); + combined.putAll(platformProperties); + combined.putAll(targetProperties); + return ImmutableMap.copyOf(combined); + } + + protected abstract ImmutableMap execGroups(); + + protected abstract ImmutableTable execProperties(); + + public ExecGroup getExecGroup(String execGroupName) { + return execGroups().get(execGroupName); + } + + public ImmutableMap getExecProperties(String execGroupName) { + return execProperties().row(execGroupName); + } + + /** + * Parse raw exec properties attribute value into a map of exec group names to their properties. + * The raw map can have keys of two forms: (1) 'property' and (2) 'exec_group_name.property'. The + * former get parsed into the default exec group, the latter get parsed into their relevant exec + * groups. + */ + private static ImmutableTable parseExecProperties( + Map rawExecProperties) { + ImmutableTable.Builder execProperties = ImmutableTable.builder(); + for (Map.Entry execProperty : rawExecProperties.entrySet()) { + String rawProperty = execProperty.getKey(); + int delimiterIndex = rawProperty.indexOf('.'); + if (delimiterIndex == -1) { + execProperties.put(DEFAULT_EXEC_GROUP_NAME, rawProperty, execProperty.getValue()); + } else { + String execGroup = rawProperty.substring(0, delimiterIndex); + String property = rawProperty.substring(delimiterIndex + 1); + execProperties.put(execGroup, property, execProperty.getValue()); + } + } + return execProperties.build(); + } + + /** An error for when the user tries to access a non-existent exec group. */ + public static final class InvalidExecGroupException extends Exception + implements SaneAnalysisException { + + public InvalidExecGroupException(Collection invalidNames) { + super( + String.format( + "Tried to set properties for non-existent exec groups: %s.", + invalidNames.stream().collect(joining(",")))); + } + + @Override + public DetailedExitCode getDetailedExitCode() { + return DetailedExitCode.of( + FailureDetail.newBuilder() + .setMessage(getMessage()) + .setAnalysis(Analysis.newBuilder().setCode(Code.EXEC_GROUP_MISSING)) + .build()); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index ce49a4d4a7bb48..2a71d36fe42fb7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -28,7 +28,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; -import com.google.common.collect.ImmutableTable; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; @@ -44,6 +43,7 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode; +import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException; import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoKey; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -90,12 +90,7 @@ import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.packages.Type.LabelClass; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; -import com.google.devtools.build.lib.server.FailureDetails.Analysis; -import com.google.devtools.build.lib.server.FailureDetails.Analysis.Code; -import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; -import com.google.devtools.build.lib.skyframe.SaneAnalysisException; -import com.google.devtools.build.lib.util.DetailedExitCode; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.util.OrderedSetMultimap; @@ -208,10 +203,10 @@ public ImmutableList getFiles() { private final ImmutableList> universalFragments; private final RuleErrorConsumer reporter; @Nullable private final ToolchainCollection toolchainContexts; + private final ExecGroupCollection execGroupCollection; private final ConstraintSemantics constraintSemantics; private final ImmutableSet requiredConfigFragments; private final List makeVariableExpanders = new ArrayList<>(); - private final ImmutableTable execProperties; /** Map of exec group names to ActionOwners. */ private final Map actionOwners = new HashMap<>(); @@ -232,6 +227,7 @@ private RuleContext( ActionLookupKey actionLookupKey, ImmutableMap aspectAttributes, @Nullable ToolchainCollection toolchainContexts, + ExecGroupCollection execGroupCollection, ConstraintSemantics constraintSemantics, ImmutableSet requiredConfigFragments) throws InvalidExecGroupException { @@ -265,7 +261,7 @@ private RuleContext( this.actionOwnerSymbolGenerator = new SymbolGenerator<>(actionLookupKey); reporter = builder.reporter; this.toolchainContexts = toolchainContexts; - this.execProperties = parseExecProperties(builder.rawExecProperties); + this.execGroupCollection = execGroupCollection; this.constraintSemantics = constraintSemantics; this.requiredConfigFragments = requiredConfigFragments; } @@ -483,9 +479,8 @@ public ActionOwner getActionOwner(String execGroup) { rule, aspectDescriptors, getConfiguration(), - getExecProperties(execGroup, execProperties), - getExecutionPlatform(execGroup), - ImmutableSet.of(execGroup)); + getExecGroups().getExecProperties(execGroup), + getExecutionPlatform(execGroup)); actionOwners.put(execGroup, actionOwner); return actionOwner; } @@ -592,47 +587,13 @@ public ImmutableList getBuildInfo(BuildInfoKey key) throws Interrupted AnalysisUtils.isStampingEnabled(this, getConfiguration()), key, getConfiguration()); } - /** - * Computes a map of exec properties given the execution platform, taking only properties in exec - * groups that are applicable to this action. Properties for specific exec groups take precedence - * over properties that don't specify an exec group. - */ - private static ImmutableMap computeExecProperties( - Map targetExecProperties, - @Nullable PlatformInfo executionPlatform, - Set execGroups) { - Map execProperties = new HashMap<>(); - - if (executionPlatform != null) { - ImmutableTable execPropertiesPerGroup = - parseExecGroups(executionPlatform.execProperties()); - - if (execPropertiesPerGroup.containsRow(DEFAULT_EXEC_GROUP_NAME)) { - execProperties.putAll(execPropertiesPerGroup.row(DEFAULT_EXEC_GROUP_NAME)); - } - - for (String execGroup : execPropertiesPerGroup.rowKeySet()) { - if (execGroups.contains(execGroup)) { - execProperties.putAll(execPropertiesPerGroup.row(execGroup)); - } - } - } - - // If the same key occurs both in the platform and in target-specific properties, the - // value is taken from target-specific properties (effectively overriding the platform - // properties). - execProperties.putAll(targetExecProperties); - return ImmutableMap.copyOf(execProperties); - } - @VisibleForTesting public static ActionOwner createActionOwner( Rule rule, ImmutableList aspectDescriptors, BuildConfiguration configuration, - Map targetExecProperties, - @Nullable PlatformInfo executionPlatform, - Set execGroups) { + ImmutableMap execProperties, + @Nullable PlatformInfo executionPlatform) { return ActionOwner.create( rule.getLabel(), aspectDescriptors, @@ -642,7 +603,7 @@ public static ActionOwner createActionOwner( configuration.checksum(), configuration.toBuildEvent(), configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null, - computeExecProperties(targetExecProperties, executionPlatform, execGroups), + execProperties, executionPlatform); } @@ -1239,6 +1200,10 @@ public ToolchainCollection getToolchainContexts() { return toolchainContexts; } + public ExecGroupCollection getExecGroups() { + return execGroupCollection; + } + public boolean targetPlatformHasConstraint(ConstraintValueInfo constraintValue) { if (toolchainContexts == null || toolchainContexts.getTargetPlatform() == null) { return false; @@ -1273,101 +1238,6 @@ public ImmutableSortedSet getRequiredConfigFragments() { return ans.build(); } - private ImmutableTable parseExecProperties( - Map execProperties) throws InvalidExecGroupException { - if (execProperties.isEmpty()) { - return ImmutableTable.of(); - } else { - return parseExecProperties( - execProperties, toolchainContexts == null ? null : toolchainContexts.getExecGroupNames()); - } - } - - /** - * Parse raw exec properties attribute value into a map of exec group names to their properties. - * The raw map can have keys of two forms: (1) 'property' and (2) 'exec_group_name.property'. The - * former get parsed into the default exec group, the latter get parsed into their relevant exec - * groups. - */ - private static ImmutableTable parseExecGroups( - Map rawExecProperties) { - ImmutableTable.Builder execProperties = ImmutableTable.builder(); - for (Map.Entry execProperty : rawExecProperties.entrySet()) { - String rawProperty = execProperty.getKey(); - int delimiterIndex = rawProperty.indexOf('.'); - if (delimiterIndex == -1) { - execProperties.put(DEFAULT_EXEC_GROUP_NAME, rawProperty, execProperty.getValue()); - } else { - String execGroup = rawProperty.substring(0, delimiterIndex); - String property = rawProperty.substring(delimiterIndex + 1); - execProperties.put(execGroup, property, execProperty.getValue()); - } - } - return execProperties.build(); - } - - /** - * Parse raw exec properties attribute value into a map of exec group names to their properties. - * If given a set of exec groups, validates all the exec groups in the map are applicable to the - * action. - */ - private static ImmutableTable parseExecProperties( - Map rawExecProperties, @Nullable Set execGroups) - throws InvalidExecGroupException { - ImmutableTable consolidatedProperties = - parseExecGroups(rawExecProperties); - if (execGroups != null) { - for (String execGroupName : consolidatedProperties.rowKeySet()) { - if (!execGroupName.equals(DEFAULT_EXEC_GROUP_NAME) && !execGroups.contains(execGroupName)) { - throw new InvalidExecGroupException( - String.format( - "Tried to set properties for non-existent exec group '%s'.", execGroupName)); - } - } - } - - return consolidatedProperties; - } - - /** - * Gets the combined exec properties of the given exec group and the target's exec properties. If - * a property is set in both, the exec group properties take precedence. If a non-existent exec - * group is passed in, just returns the target's exec properties. - * - * @param execGroup group whose properties to retrieve - * @param execProperties Map of exec group name to map of properties and values - */ - private static ImmutableMap getExecProperties( - String execGroup, ImmutableTable execProperties) { - if (!execProperties.containsRow(execGroup) || execGroup.equals(DEFAULT_EXEC_GROUP_NAME)) { - return execProperties.row(DEFAULT_EXEC_GROUP_NAME); - } - - // Use a HashMap to build here because we expect duplicate keys to happen - // (and rewrite previous entries). - Map targetAndGroupProperties = - new HashMap<>(execProperties.row(DEFAULT_EXEC_GROUP_NAME)); - targetAndGroupProperties.putAll(execProperties.row(execGroup)); - return ImmutableMap.copyOf(targetAndGroupProperties); - } - - /** An error for when the user tries to access an non-existent exec group */ - public static final class InvalidExecGroupException extends Exception - implements SaneAnalysisException { - InvalidExecGroupException(String message) { - super(message); - } - - @Override - public DetailedExitCode getDetailedExitCode() { - return DetailedExitCode.of( - FailureDetail.newBuilder() - .setMessage(getMessage()) - .setAnalysis(Analysis.newBuilder().setCode(Code.EXEC_GROUP_MISSING)) - .build()); - } - } - private void checkAttributeIsDependency(String attributeName) { Attribute attributeDefinition = attributes.getAttributeDefinition(attributeName); if (attributeDefinition == null) { @@ -1736,6 +1606,7 @@ public static final class Builder implements RuleErrorConsumer { private ImmutableMap aspectAttributes; private ImmutableList aspects; private ToolchainCollection toolchainContexts; + private ExecGroupCollection.Builder execGroupCollectionBuilder; private ImmutableMap rawExecProperties; private ConstraintSemantics constraintSemantics; private ImmutableSet requiredConfigFragments = ImmutableSet.of(); @@ -1779,14 +1650,6 @@ public RuleContext build() throws InvalidExecGroupException { ListMultimap targetMap = createTargetMap(); ListMultimap filesetEntryMap = createFilesetEntryMap(target.getAssociatedRule(), configConditions); - if (rawExecProperties == null) { - if (!attributes.has(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)) { - rawExecProperties = ImmutableMap.of(); - } else { - rawExecProperties = - ImmutableMap.copyOf(attributes.get(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)); - } - } return new RuleContext( this, attributes, @@ -1798,10 +1661,26 @@ public RuleContext build() throws InvalidExecGroupException { actionOwnerSymbol, aspectAttributes != null ? aspectAttributes : ImmutableMap.of(), toolchainContexts, + createExecGroupCollection(execGroupCollectionBuilder, attributes), constraintSemantics, requiredConfigFragments); } + private ExecGroupCollection createExecGroupCollection( + ExecGroupCollection.Builder execGroupCollectionBuilder, AttributeMap attributes) + throws InvalidExecGroupException { + if (rawExecProperties == null) { + if (!attributes.has(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)) { + rawExecProperties = ImmutableMap.of(); + } else { + rawExecProperties = + ImmutableMap.copyOf(attributes.get(RuleClass.EXEC_PROPERTIES, Type.STRING_DICT)); + } + } + + return execGroupCollectionBuilder.build(toolchainContexts, rawExecProperties); + } + private void checkAttributesNonEmpty(AttributeMap attributes) { for (String attributeName : attributes.getAttributeNames()) { Attribute attr = attributes.getAttributeDefinition(attributeName); @@ -1889,6 +1768,12 @@ public Builder setToolchainContexts( return this; } + public Builder setExecGroupCollectionBuilder( + ExecGroupCollection.Builder execGroupCollectionBuilder) { + this.execGroupCollectionBuilder = execGroupCollectionBuilder; + return this; + } + /** * Warning: if you set the exec properties using this method any exec_properties attribute value * will be ignored in favor of this value. diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index 3a02fd6f387b87..b0a072d9e59574 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -74,6 +74,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", "//third_party:auto_value", "//third_party:guava", + "//third_party:jsr305", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java b/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java index a4de2bf17ea9bf..ec56d4854cfe0f 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java +++ b/src/main/java/com/google/devtools/build/lib/packages/ExecGroup.java @@ -15,11 +15,11 @@ package com.google.devtools.build.lib.packages; import com.google.auto.value.AutoValue; -import com.google.common.annotations.VisibleForTesting; 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. */ @AutoValue @@ -27,36 +27,43 @@ public abstract class ExecGroup implements ExecGroupApi { // 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