From 72496b5e50b4cb2a1d4dc4b990849bc90580cb21 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 8 Jun 2023 15:04:07 -0700 Subject: [PATCH] Updates getConfiguredTargetMapForTesting to use new prerequisite computation. Add PrerequisitesProducer, which is intended for eventual use in production. PiperOrigin-RevId: 538901131 Change-Id: Idb84f410aab607089330b469248509da983e9293 --- .../producers/AspectMergerForTesting.java | 144 ---------- .../producers/AttributeConfiguration.java | 76 +++++ .../build/lib/analysis/producers/BUILD | 1 + .../DependencyEvaluatorForTesting.java | 148 ++++++++++ .../producers/PrerequisiteParameters.java | 71 +++++ .../producers/PrerequisitesProducer.java | 261 ++++++++++++++++++ .../lib/skyframe/ConfiguredTargetAndData.java | 11 + .../build/lib/skyframe/SkyframeExecutor.java | 194 ++++--------- .../analysis/util/BuildViewForTesting.java | 18 +- 9 files changed, 642 insertions(+), 282 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/analysis/producers/AspectMergerForTesting.java create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/producers/AttributeConfiguration.java create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/producers/DependencyEvaluatorForTesting.java create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisiteParameters.java create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisitesProducer.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/AspectMergerForTesting.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/AspectMergerForTesting.java deleted file mode 100644 index c9b636c2434dd8..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/AspectMergerForTesting.java +++ /dev/null @@ -1,144 +0,0 @@ -// Copyright 2023 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.producers; - -import static com.google.devtools.build.lib.analysis.AspectResolutionHelpers.computeAspectCollection; -import static com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData.SPLIT_DEP_ORDERING; - -import com.google.devtools.build.lib.analysis.AspectCollection; -import com.google.devtools.build.lib.analysis.DuplicateException; -import com.google.devtools.build.lib.analysis.InconsistentAspectOrderException; -import com.google.devtools.build.lib.analysis.PartiallyResolvedDependency; -import com.google.devtools.build.lib.events.ExtendedEventHandler; -import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; -import com.google.devtools.build.lib.util.OrderedSetMultimap; -import com.google.devtools.build.skyframe.state.StateMachine; -import java.util.Arrays; -import java.util.Set; - -/** Computes aspect values and merges them. */ -// TODO(b/261521010): This temporary scaffolding exists to reduce the changelist size. Delete this. -public final class AspectMergerForTesting implements StateMachine { - /** Receives output of {@link AspectMerger}. */ - public interface ResultSink { - void acceptAspectMergerResult( - OrderedSetMultimap map); - - void acceptAspectMergerError(InconsistentAspectOrderException error); - - void acceptAspectMergerError(DuplicateException error); - } - - // -------------------- Input -------------------- - private final OrderedSetMultimap - dependencies; - - // -------------------- Output -------------------- - private final ResultSink sink; - - // -------------------- Internal State -------------------- - /** - * Staging area for output, aligning with {@link #dependencies}. - * - *

While performance doesn't matter in this test code, it needs to fit interfaces that are used - * in production code. ConfiguredTargetFunction is highly-cpu constrained and intensive. - * - *

Using indices and depending on the stable iteration order of `dependencies` saves us - * allocation, GC, scanning and hashing costs that would be associated with constructing a - * parallel map with the same keys. - * - *

    - *
  1. The first index corresponds with {@link PartiallyResolvedDependency} key of {@link - * #dependencies}. - *
  2. The second index corresponds with the multiple entries per key. - *
- */ - private final ConfiguredTargetAndData[][] result; - - private boolean hasError = false; - - public AspectMergerForTesting( - OrderedSetMultimap dependencies, - ResultSink sink) { - this.dependencies = dependencies; - this.sink = sink; - this.result = new ConfiguredTargetAndData[dependencies.keySet().size()][]; - } - - @Override - public StateMachine step(Tasks tasks, ExtendedEventHandler listener) { - int keyIndex = 0; - for (PartiallyResolvedDependency key : dependencies.keySet()) { - Set targets = dependencies.get(key); - - AspectCollection aspects; - try { - // Using any target to calculate aspects is fine because the filtering is based on Target - // properties and all values for a given key share a common one. - aspects = computeAspectCollection(targets.iterator().next(), key.getPropagatingAspects()); - } catch (InconsistentAspectOrderException e) { - sink.acceptAspectMergerError(e); - return DONE; - } - - var targetResult = new ConfiguredTargetAndData[targets.size()]; - result[keyIndex] = targetResult; - - var mergedTargetSink = - new ConfiguredAspectProducer.ResultSink() { - @Override - public void acceptConfiguredAspectMergedTarget( - int outputIndex, ConfiguredTargetAndData mergedTarget) { - targetResult[outputIndex] = mergedTarget; - } - - @Override - public void acceptConfiguredAspectError(DuplicateException error) { - hasError = true; - sink.acceptAspectMergerError(error); - } - }; - - int targetIndex = 0; - for (ConfiguredTargetAndData target : targets) { - tasks.enqueue( - new ConfiguredAspectProducer( - aspects, target, mergedTargetSink, targetIndex, /* transitivePackages= */ null)); - targetIndex++; - } - keyIndex++; - } - return this::emitResult; - } - - private StateMachine emitResult(Tasks tasks, ExtendedEventHandler listener) { - if (hasError) { - return DONE; - } - - var mergedDependencies = - OrderedSetMultimap.create(); - int keyIndex = 0; - for (PartiallyResolvedDependency key : dependencies.keySet()) { - ConfiguredTargetAndData[] targets = result[keyIndex]; - if (targets.length > 1) { - Arrays.sort(targets, SPLIT_DEP_ORDERING); - } - mergedDependencies.putAll(key, Arrays.asList(targets)); - ++keyIndex; - } - sink.acceptAspectMergerResult(mergedDependencies); - return DONE; - } -} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/AttributeConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/AttributeConfiguration.java new file mode 100644 index 00000000000000..c927e3c38467a0 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/AttributeConfiguration.java @@ -0,0 +1,76 @@ +// Copyright 2023 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.producers; + +import com.google.auto.value.AutoOneOf; +import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.packages.PackageGroup; +import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; + +@AutoOneOf(AttributeConfiguration.Kind.class) +abstract class AttributeConfiguration { + enum Kind { + /** + * This is a visibility dependency. + * + *

Visibility dependencies have null configuration as they are not configurable. Once the + * target is known, it should be verified to be a {@link PackageGroup}. + */ + VISIBILITY, + /** + * There is a single configuration. + * + *

This can be the result of a patch transition or no transition at all. + */ + UNARY, + /** + * There is a split transition. + * + *

It's possible for there to be only one entry. + */ + SPLIT + } + + abstract Kind kind(); + + abstract void visibility(); + + abstract BuildConfigurationKey unary(); + + abstract ImmutableMap split(); + + public int count() { + switch (kind()) { + case VISIBILITY: + case UNARY: + return 1; + case SPLIT: + return split().size(); + } + throw new IllegalStateException("unreachable"); + } + + static AttributeConfiguration ofVisibility() { + return AutoOneOf_AttributeConfiguration.visibility(); + } + + static AttributeConfiguration ofUnary(BuildConfigurationKey key) { + return AutoOneOf_AttributeConfiguration.unary(key); + } + + static AttributeConfiguration ofSplit( + ImmutableMap configurations) { + return AutoOneOf_AttributeConfiguration.split(configurations); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD index 93a02204f46e7a..1c801b92ca6930 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/BUILD @@ -45,6 +45,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils", "//src/main/java/com/google/devtools/build/lib/buildeventstream", + "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/causes", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect/nestedset", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/DependencyEvaluatorForTesting.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/DependencyEvaluatorForTesting.java new file mode 100644 index 00000000000000..02fb0bb5002986 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/DependencyEvaluatorForTesting.java @@ -0,0 +1,148 @@ +// Copyright 2023 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.producers; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ListMultimap; +import com.google.devtools.build.lib.analysis.BaseDependencySpecification; +import com.google.devtools.build.lib.analysis.InvalidVisibilityDependencyException; +import com.google.devtools.build.lib.analysis.PartiallyResolvedDependency; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException; +import com.google.devtools.build.lib.util.OrderedSetMultimap; +import com.google.devtools.build.skyframe.state.StateMachine; +import com.google.devtools.build.skyframe.state.StateMachine.Tasks; +import java.util.Arrays; +import java.util.List; + +/** Computes aspect values and merges them. */ +// TODO(b/261521010): This temporary scaffolding exists to reduce the changelist size. Delete this. +public final class DependencyEvaluatorForTesting implements StateMachine { + /** Receives output of {@link DependencyEvaluatorForTesting}. */ + public interface ResultSink { + void acceptDependencyEvaluatorResult( + OrderedSetMultimap result); + + void acceptDependencyEvaluatorError(InvalidVisibilityDependencyException error); + + void acceptDependencyEvaluatorError(ConfiguredValueCreationException error); + + void acceptDependencyEvaluatorError(DependencyEvaluationException error); + } + + // -------------------- Input -------------------- + private final PrerequisiteParameters parameters; + private final ImmutableSet dependencies; + private final ListMultimap configurations; + + // -------------------- Output -------------------- + private final ResultSink sink; + + // -------------------- Internal State -------------------- + private final OrderedSetMultimap + prerequisiteValues = OrderedSetMultimap.create(); + private boolean hasError = false; + + public DependencyEvaluatorForTesting( + PrerequisiteParameters parameters, + ImmutableSet dependencies, + ListMultimap configurations, + ResultSink sink) { + this.parameters = parameters; + this.dependencies = dependencies; + this.configurations = configurations; + this.sink = sink; + } + + @Override + public StateMachine step(Tasks tasks, ExtendedEventHandler listener) { + for (PartiallyResolvedDependency key : dependencies) { + if (!configurations.containsKey(key)) { + // If we couldn't compute a configuration for this target, the target was in error (e.g. + // it couldn't be loaded). Exclude it from the results. + continue; + } + + List depConfigs = configurations.get(key); + // Constructs a fake attributeConfiguration for testing based on how many configurations there + // are. This inference is only needed in testing. + AttributeConfiguration attributeConfiguration; + if (depConfigs.size() == 1) { + BuildConfigurationValue configuration = depConfigs.get(0); + if (configuration == null) { + // If the configuration is null, it was explicitly added as the result of a + // `NullTransition` by SkyframeExecutor.getConfigurations. The only way this can happen + // for a PartiallyResolvedDependency (which is determined without inspecting child + // targets) is for visibility dependencies. + attributeConfiguration = AttributeConfiguration.ofVisibility(); + } else { + attributeConfiguration = AttributeConfiguration.ofUnary(configuration.getKey()); + } + } else { + var split = ImmutableMap.builder(); + for (int i = 0; i < depConfigs.size(); i++) { + // Uses some fake transition keys. + split.put(Integer.toString(i), depConfigs.get(i).getKey()); + } + attributeConfiguration = AttributeConfiguration.ofSplit(split.buildOrThrow()); + } + + tasks.enqueue( + new PrerequisitesProducer( + parameters, + key.getLabel(), + key.getExecutionPlatformLabel(), + attributeConfiguration, + key.getPropagatingAspects(), + new PrerequisitesProducer.ResultSink() { + @Override + public void acceptPrerequisitesValue(ConfiguredTargetAndData[] prerequisites) { + prerequisiteValues.putAll(key, Arrays.asList(prerequisites)); + } + + @Override + public void acceptPrerequisitesError(InvalidVisibilityDependencyException error) { + hasError = true; + sink.acceptDependencyEvaluatorError(error); + } + + @Override + public void acceptPrerequisitesCreationError( + ConfiguredValueCreationException error) { + hasError = true; + sink.acceptDependencyEvaluatorError(error); + } + + @Override + public void acceptPrerequisitesAspectError(DependencyEvaluationException error) { + hasError = true; + sink.acceptDependencyEvaluatorError(error); + } + })); + } + return this::emitResult; + } + + private StateMachine emitResult(Tasks tasks, ExtendedEventHandler listener) { + if (!hasError) { + sink.acceptDependencyEvaluatorResult(prerequisiteValues); + } + return DONE; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisiteParameters.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisiteParameters.java new file mode 100644 index 00000000000000..3967692def2bac --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisiteParameters.java @@ -0,0 +1,71 @@ +// Copyright 2023 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.producers; + +import static com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil.configurationId; + +import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; +import javax.annotation.Nullable; +import net.starlark.java.syntax.Location; + +/** Common parameters for computing prerequisites. */ +public final class PrerequisiteParameters { + private final ConfiguredTargetKey configuredTargetKey; + @Nullable private final Rule associatedRule; + + private final TransitiveDependencyState transitiveState; + + public PrerequisiteParameters( + ConfiguredTargetKey configuredTargetKey, + @Nullable Rule associatedRule, + TransitiveDependencyState transitiveState) { + this.configuredTargetKey = configuredTargetKey; + this.associatedRule = associatedRule; + this.transitiveState = transitiveState; + } + + public Label label() { + return configuredTargetKey.getLabel(); + } + + @Nullable + public Rule associatedRule() { + return associatedRule; + } + + @Nullable + public BuildConfigurationKey configurationKey() { + return configuredTargetKey.getConfigurationKey(); + } + + @Nullable + public Location location() { + if (associatedRule == null) { + return null; + } + return associatedRule.getLocation(); + } + + public BuildEventId eventId() { + return configurationId(configurationKey()); + } + + public TransitiveDependencyState transitiveState() { + return transitiveState; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisitesProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisitesProducer.java new file mode 100644 index 00000000000000..52a0f6529d647c --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/PrerequisitesProducer.java @@ -0,0 +1,261 @@ +// Copyright 2023 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.producers; + +import static com.google.common.base.Preconditions.checkState; +import static com.google.devtools.build.lib.analysis.AspectResolutionHelpers.computeAspectCollection; +import static com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData.SPLIT_DEP_ORDERING; +import static java.util.Arrays.sort; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.analysis.AspectCollection; +import com.google.devtools.build.lib.analysis.DuplicateException; +import com.google.devtools.build.lib.analysis.InconsistentAspectOrderException; +import com.google.devtools.build.lib.analysis.InvalidVisibilityDependencyException; +import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.events.ExtendedEventHandler; +import com.google.devtools.build.lib.packages.Aspect; +import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; +import com.google.devtools.build.lib.skyframe.ConfiguredValueCreationException; +import com.google.devtools.build.skyframe.state.StateMachine; +import com.google.devtools.build.skyframe.state.StateMachine.Tasks; +import java.util.Map; +import javax.annotation.Nullable; + +/** + * Computes requested prerequisite(s), applying any requested aspects. + * + *

A dependency is specified by a {@link Label} and an execution platform {@link Label} if it is + * a toolchain. + * + *

Its configuration is determined by an {@link AttributeConfiguration}, which may be split and + * result in multiple outputs. + * + *

Computes any specified aspects, applying the appropriate filtering, and merges them into the + * resulting values. + */ +final class PrerequisitesProducer + implements StateMachine, + ConfiguredTargetAndDataProducer.ResultSink, + ConfiguredAspectProducer.ResultSink { + interface ResultSink { + void acceptPrerequisitesValue(ConfiguredTargetAndData[] prerequisites); + + void acceptPrerequisitesError(InvalidVisibilityDependencyException error); + + void acceptPrerequisitesCreationError(ConfiguredValueCreationException error); + + void acceptPrerequisitesAspectError(DependencyEvaluationException error); + } + + // -------------------- Input -------------------- + private final PrerequisiteParameters parameters; + private final Label label; + @Nullable // Non-null for toolchain prerequisites. + private final Label executionPlatformLabel; + private final AttributeConfiguration configuration; + private final ImmutableList propagatingAspects; + + // -------------------- Output -------------------- + private final ResultSink sink; + + // -------------------- Internal State -------------------- + private ConfiguredTargetAndData[] configuredTargets; + private boolean hasError; + + PrerequisitesProducer( + PrerequisiteParameters parameters, + Label label, + @Nullable Label executionPlatformLabel, + AttributeConfiguration configuration, + ImmutableList propagatingAspects, + ResultSink sink) { + this.parameters = parameters; + this.label = label; + this.executionPlatformLabel = executionPlatformLabel; + this.configuration = configuration; + this.propagatingAspects = propagatingAspects; + this.sink = sink; + + // size > 0 guaranteed by contract of SplitTransition. + int size = configuration.count(); + this.configuredTargets = new ConfiguredTargetAndData[size]; + } + + @Override + public StateMachine step(Tasks tasks, ExtendedEventHandler listener) { + switch (configuration.kind()) { + case VISIBILITY: + tasks.enqueue( + new ConfiguredTargetAndDataProducer( + getPrerequisiteKey(/* configurationKey= */ null), + /* transitionKey= */ null, + parameters.transitiveState(), + (ConfiguredTargetAndDataProducer.ResultSink) this, + /* outputIndex= */ 0)); + break; + case UNARY: + tasks.enqueue( + new ConfiguredTargetAndDataProducer( + getPrerequisiteKey(configuration.unary()), + /* transitionKey= */ null, + parameters.transitiveState(), + (ConfiguredTargetAndDataProducer.ResultSink) this, + /* outputIndex= */ 0)); + break; + case SPLIT: + int index = 0; + for (Map.Entry entry : configuration.split().entrySet()) { + tasks.enqueue( + new ConfiguredTargetAndDataProducer( + getPrerequisiteKey(entry.getValue()), + /* transitionKey= */ entry.getKey(), + parameters.transitiveState(), + (ConfiguredTargetAndDataProducer.ResultSink) this, + index)); + ++index; + } + break; + } + return this::computeConfiguredAspects; + } + + @Override + public void acceptConfiguredTargetAndData(ConfiguredTargetAndData value, int index) { + configuredTargets[index] = value; + } + + @Override + public void acceptConfiguredTargetAndDataError(InvalidVisibilityDependencyException error) { + hasError = true; + sink.acceptPrerequisitesError(error); + } + + @Override + public void acceptConfiguredTargetAndDataError(ConfiguredValueCreationException error) { + hasError = true; + sink.acceptPrerequisitesCreationError(error); + } + + private StateMachine computeConfiguredAspects(Tasks tasks, ExtendedEventHandler listener) { + if (hasError) { + return DONE; + } + + cleanupValues(); + + AspectCollection aspects; + try { + // All configured targets in the set have the same underlying target so using an arbitrary one + // for aspect filtering is safe. + aspects = computeAspectCollection(configuredTargets[0], propagatingAspects); + } catch (InconsistentAspectOrderException e) { + sink.acceptPrerequisitesAspectError(new DependencyEvaluationException(e)); + return DONE; + } + + if (aspects.isEmpty()) { // Short circuits if there are no aspects. + sink.acceptPrerequisitesValue(configuredTargets); + return DONE; + } + + NestedSetBuilder transitivePackages = + parameters.transitiveState().transitivePackages(); + for (int i = 0; i < configuredTargets.length; ++i) { + ConfiguredTargetAndData target = configuredTargets[i]; + configuredTargets[i] = null; + tasks.enqueue( + new ConfiguredAspectProducer( + aspects, target, (ConfiguredAspectProducer.ResultSink) this, i, transitivePackages)); + } + return this::emitMergedTargets; + } + + @Override + public void acceptConfiguredAspectMergedTarget( + int outputIndex, ConfiguredTargetAndData mergedTarget) { + configuredTargets[outputIndex] = mergedTarget; + } + + @Override + public void acceptConfiguredAspectError(DuplicateException error) { + hasError = true; + sink.acceptPrerequisitesAspectError( + new DependencyEvaluationException( + new ConfiguredValueCreationException( + parameters.location(), + error.getMessage(), + parameters.label(), + parameters.eventId(), + /* rootCauses= */ null, + /* detailedExitCode= */ null), + /* depReportedOwnError= */ false)); + } + + private StateMachine emitMergedTargets(Tasks tasks, ExtendedEventHandler listener) { + if (!hasError) { + sink.acceptPrerequisitesValue(configuredTargets); + } + return DONE; + } + + private ConfiguredTargetKey getPrerequisiteKey(@Nullable BuildConfigurationKey configurationKey) { + var key = ConfiguredTargetKey.builder().setLabel(label).setConfigurationKey(configurationKey); + if (executionPlatformLabel != null) { + key.setExecutionPlatformLabel(executionPlatformLabel); + } + return key.build(); + } + + private void cleanupValues() { + if (configuredTargets.length == 1) { + // Clears the transition keys if there was no effective transition. + BuildConfigurationKey fromConfigurationKey = parameters.configurationKey(); + BuildConfigurationValue transitionedConfiguration = configuredTargets[0].getConfiguration(); + // `fromConfigurationKey` == null implies `transitionedConfiguration` == null. + if (fromConfigurationKey == null + || (transitionedConfiguration != null + && fromConfigurationKey + .getOptions() + .equals(transitionedConfiguration.getOptions()))) { + configuredTargets[0] = configuredTargets[0].copyWithClearedTransitionKeys(); + } + return; + } + // Otherwise, there was a split transition. Aggregates the transition keys if the resulting + // configurations are null. + if (configuredTargets[0].getConfiguration() == null) { + var keys = new ImmutableList.Builder(); + keys.addAll(configuredTargets[0].getTransitionKeys()); + for (int i = 1; i < configuredTargets.length; ++i) { + checkState( + configuredTargets[i].getConfiguration() == null, + "inconsistent split transition result from %s to %s", + parameters.label(), + label); + keys.addAll(configuredTargets[i].getTransitionKeys()); + } + configuredTargets = + new ConfiguredTargetAndData[] {configuredTargets[0].copyWithTransitionKeys(keys.build())}; + return; + } + sort(configuredTargets, SPLIT_DEP_ORDERING); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java index c9e8ca40970151..25f82053d2ed3f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetAndData.java @@ -301,6 +301,17 @@ public TestTimeout getTestTimeout() { return TestTimeout.getTestTimeout(rule); } + public ConfiguredTargetAndData copyWithClearedTransitionKeys() { + if (transitionKeys.isEmpty()) { + return this; + } + return copyWithTransitionKeys(ImmutableList.of()); + } + + public ConfiguredTargetAndData copyWithTransitionKeys(ImmutableList keys) { + return new ConfiguredTargetAndData(configuredTarget, target, configuration, keys); + } + /** * This should only be used in testing. * diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index a13c735103bb5a..596ef4805c5be0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -90,8 +90,6 @@ import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; import com.google.devtools.build.lib.analysis.Dependency; import com.google.devtools.build.lib.analysis.DependencyKind; -import com.google.devtools.build.lib.analysis.DuplicateException; -import com.google.devtools.build.lib.analysis.InconsistentAspectOrderException; import com.google.devtools.build.lib.analysis.InvalidVisibilityDependencyException; import com.google.devtools.build.lib.analysis.PartiallyResolvedDependency; import com.google.devtools.build.lib.analysis.PlatformOptions; @@ -105,13 +103,15 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.ConfigurationResolver; import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.config.transitions.NullTransition; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics; -import com.google.devtools.build.lib.analysis.producers.AspectMergerForTesting; import com.google.devtools.build.lib.analysis.producers.ConfiguredTargetAndDataProducer; +import com.google.devtools.build.lib.analysis.producers.DependencyEvaluatorForTesting; +import com.google.devtools.build.lib.analysis.producers.PrerequisiteParameters; import com.google.devtools.build.lib.analysis.producers.TransitiveDependencyState; import com.google.devtools.build.lib.analysis.starlark.StarlarkBuildSettingsDetailsValue; import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition; @@ -146,12 +146,12 @@ import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.BuildFileName; import com.google.devtools.build.lib.packages.NoSuchPackageException; -import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.Package.Builder.PackageSettings; import com.google.devtools.build.lib.packages.Package.ConfigSettingVisibilityPolicy; import com.google.devtools.build.lib.packages.PackageFactory; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.packages.Target; @@ -3926,14 +3926,16 @@ public void acceptConfiguredTargetAndDataError( public OrderedSetMultimap getConfiguredTargetMapForTesting( ExtendedEventHandler eventHandler, - BuildConfigurationKey configurationKey, + ConfiguredTargetKey parentKey, + @Nullable Rule associatedRule, OrderedSetMultimap dependencies) throws InvalidConfigurationException, InterruptedException { checkActive(); // Duplicates may be present if the same dependency occurs under different DependencyKind // values. var dependencyKeys = ImmutableSet.copyOf(dependencies.values()); - BuildConfigurationValue configuration = getConfiguration(eventHandler, configurationKey); + BuildConfigurationValue configuration = + getConfiguration(eventHandler, parentKey.getConfigurationKey()); ListMultimap configs; if (configuration != null) { configs = @@ -3946,143 +3948,57 @@ public void acceptConfiguredTargetAndDataError( } } - final List skyKeys = new ArrayList<>(); - for (PartiallyResolvedDependency key : dependencyKeys) { - if (!configs.containsKey(key)) { - // If we couldn't compute a configuration for this target, the target was in error (e.g. - // it couldn't be loaded). Exclude it from the results. - continue; - } - for (BuildConfigurationValue depConfig : configs.get(key)) { - ConfiguredTargetKey configuredTargetKey = - ConfiguredTargetKey.builder() - .setLabel(key.getLabel()) - .setConfiguration(depConfig) - .build(); - skyKeys.add(configuredTargetKey.toKey()); - } - skyKeys.add(key.getLabel().getPackageIdentifier()); - } - - EvaluationResult result = evaluateSkyKeys(eventHandler, skyKeys); - - var cts = OrderedSetMultimap.create(); - - // Logic copied from ConfiguredTargetFunction#computeDependencies. - Set aliasPackagesToFetch = new HashSet<>(); - var aliasKeysToRedo = new ArrayList(); - EvaluationResult aliasPackageValues = null; - Iterable keysToProcess = dependencyKeys; - for (int i = 0; i < 2; i++) { - for (PartiallyResolvedDependency key : keysToProcess) { - if (!configs.containsKey(key)) { - // If we couldn't compute a configuration for this target, the target was in error (e.g. - // it couldn't be loaded). Exclude it from the results. - continue; - } - for (BuildConfigurationValue depConfig : configs.get(key)) { - ConfiguredTargetKey configuredTargetKey = - ConfiguredTargetKey.builder() - .setLabel(key.getLabel()) - .setConfiguration(depConfig) - .build(); - if (result.get(configuredTargetKey.toKey()) == null) { - continue; - } + NestedSetBuilder transitiveRootCauses = NestedSetBuilder.stableOrder(); + var sink = + new DependencyEvaluatorForTesting.ResultSink() { + private OrderedSetMultimap result; + private InvalidVisibilityDependencyException visibilityError; + private ConfiguredValueCreationException creationError; + private DependencyEvaluationException dependencyError; - ConfiguredTarget configuredTarget = - ((ConfiguredTargetValue) result.get(configuredTargetKey.toKey())) - .getConfiguredTarget(); - Label label = configuredTarget.getLabel(); - SkyKey packageKey = label.getPackageIdentifier(); - PackageValue packageValue; - if (i == 0) { - packageValue = (PackageValue) result.get(packageKey); - if (packageValue == null) { - aliasPackagesToFetch.add(packageKey); - aliasKeysToRedo.add(key); - continue; - } - } else { - packageValue = - (PackageValue) checkNotNull(aliasPackageValues.get(packageKey), packageKey); + @Override + public void acceptDependencyEvaluatorResult( + OrderedSetMultimap result) { + this.result = result; } - BuildConfigurationKey configKey = configuredTarget.getConfigurationKey(); - BuildConfigurationValue resolvedConfig = depConfig; - if (configKey == null) { - // Unfortunately, it's possible to get a configured target with a null configuration - // when depConfig is non-null, so we need to explicitly override it in that case. - resolvedConfig = null; - } else if (!configKey.equals(depConfig.getKey())) { - resolvedConfig = getConfiguration(eventHandler, configuredTarget.getConfigurationKey()); - } - try { - cts.put( - key, - new ConfiguredTargetAndData( - configuredTarget, - packageValue.getPackage().getTarget(configuredTarget.getLabel().getName()), - resolvedConfig, - /* transitionKeys= */ ImmutableList.of())); - } catch (NoSuchTargetException e) { - throw new IllegalStateException( - String.format("Error creating %s", configuredTarget.getLabel()), e); - } - } - } - if (aliasKeysToRedo.isEmpty()) { - break; - } - aliasPackageValues = evaluateSkyKeys(eventHandler, aliasPackagesToFetch); - keysToProcess = aliasKeysToRedo; - } - - // We ignore the return value and exceptions here because tests effectively run with - // --keep_going, and the loading-phase-error bit is only needed if we're constructing a - // SkyframeAnalysisResult. - try { - var unused = - SkyframeErrorProcessor.processAnalysisErrors( - result, - cyclesReporter, - eventHandler, - /* keepGoing= */ true, - /* eventBus= */ null, - bugReporter); - } catch (ViewCreationFailedException ignored) { - // Ignored. - } - - var aspectMergerSink = - new AspectMergerForTesting.ResultSink() { - private OrderedSetMultimap - mergedTargets; - private InconsistentAspectOrderException aspectOrderException; - private DuplicateException duplicateException; - @Override - public void acceptAspectMergerResult( - OrderedSetMultimap map) { - this.mergedTargets = map; + public void acceptDependencyEvaluatorError(InvalidVisibilityDependencyException error) { + this.visibilityError = error; } @Override - public void acceptAspectMergerError(InconsistentAspectOrderException error) { - this.aspectOrderException = error; + public void acceptDependencyEvaluatorError(ConfiguredValueCreationException error) { + this.creationError = error; } @Override - public void acceptAspectMergerError(DuplicateException error) { - this.duplicateException = error; + public void acceptDependencyEvaluatorError(DependencyEvaluationException error) { + this.dependencyError = error; } }; - result = - StateMachineEvaluatorForTesting.run( - new AspectMergerForTesting(cts, aspectMergerSink), - memoizingEvaluator, - getEvaluationContextForTesting(eventHandler)); + + EvaluationResult result; + try (var closer = new EnableAnalysisScope()) { + result = + StateMachineEvaluatorForTesting.run( + new DependencyEvaluatorForTesting( + new PrerequisiteParameters( + parentKey, + associatedRule, + TransitiveDependencyState.createForTesting( + transitiveRootCauses, /* transitivePackages= */ null)), + dependencyKeys, + configs, + sink), + memoizingEvaluator, + getEvaluationContextForTesting(eventHandler)); + } + if (result != null) { + // We ignore the return value and exceptions here because tests effectively run with + // --keep_going, and the loading-phase-error bit is only needed if we're constructing a + // SkyframeAnalysisResult. try { var unused = SkyframeErrorProcessor.processAnalysisErrors( @@ -4096,15 +4012,23 @@ public void acceptAspectMergerError(DuplicateException error) { // Ignored. } } - if (aspectMergerSink.aspectOrderException != null) { - throw new IllegalStateException(aspectMergerSink.aspectOrderException); + + if (!transitiveRootCauses.isEmpty()) { + throw new IllegalStateException("expected empty: " + transitiveRootCauses.build().toList()); + } + if (sink.visibilityError != null) { + throw new IllegalStateException(sink.visibilityError); } - if (aspectMergerSink.duplicateException != null) { - throw new IllegalStateException(aspectMergerSink.duplicateException); + if (sink.creationError != null) { + throw new IllegalStateException(sink.creationError); } + if (sink.dependencyError != null) { + throw new IllegalStateException(sink.dependencyError); + } + var output = OrderedSetMultimap.create(); for (Map.Entry entry : dependencies.entries()) { - output.putAll(entry.getKey(), aspectMergerSink.mergedTargets.get(entry.getValue())); + output.putAll(entry.getKey(), sink.result.get(entry.getValue())); } return output; } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index 9eac233d756cbf..65f3dac1db651f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java @@ -280,7 +280,11 @@ public Collection getDirectPrerequisitesForTesting( ToolchainCollection unloadedToolchainCollection = state.dependencyContext.unloadedToolchainContexts(); return getPrerequisiteMapForTesting( - eventHandler, configuredTarget, unloadedToolchainCollection.asToolchainContexts()) + eventHandler, + configuredTarget, + // Cast is safe because prepareDependencyContext always populates this with a Rule. + (Rule) state.targetAndConfiguration.getTarget(), + unloadedToolchainCollection.asToolchainContexts()) .values(); } @@ -400,6 +404,7 @@ private ImmutableMap getConfigurableAttributeKeys private OrderedSetMultimap getPrerequisiteMapForTesting( final ExtendedEventHandler eventHandler, ConfiguredTarget target, + @Nullable Rule associatedRule, @Nullable ToolchainCollection toolchainContexts) throws DependencyResolver.Failure, InvalidConfigurationException, @@ -410,7 +415,10 @@ private OrderedSetMultimap getPrerequis getDirectPrerequisiteDependenciesForTesting(eventHandler, target, toolchainContexts); return skyframeExecutor.getConfiguredTargetMapForTesting( - eventHandler, target.getConfigurationKey(), depNodeNames); + eventHandler, + ConfiguredTargetKey.fromConfiguredTarget(target), + associatedRule, + depNodeNames); } /** @@ -490,7 +498,11 @@ public RuleContext getRuleContextForTesting( OrderedSetMultimap prerequisiteMap = getPrerequisiteMapForTesting( - eventHandler, configuredTarget, unloadedToolchainCollection.asToolchainContexts()); + eventHandler, + configuredTarget, + // Cast is safe because prepareDependencyContext always populates this with a Rule. + (Rule) target, + unloadedToolchainCollection.asToolchainContexts()); String targetDescription = target.toString(); ToolchainCollection.Builder resolvedToolchainContext =