Remove references to MiddlemanType.AGGREGATING_MIDDLEMAN.
Replace some occurrences with RUNFILES_MIDDLEMAN, and remove code where applicable.
PiperOrigin-RevId: 382262465
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index c10b1fc..3322a4f 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -95,7 +95,6 @@
"//src/main/java/com/google/devtools/build/lib/ssd:srcs",
"//src/main/java/com/google/devtools/build/lib/standalone:srcs",
"//src/main/java/com/google/devtools/build/lib/supplier:srcs",
- "//src/main/java/com/google/devtools/build/lib/testing/actions:srcs",
"//src/main/java/com/google/devtools/build/lib/testing/common:srcs",
"//src/main/java/com/google/devtools/build/lib/unix:srcs",
"//src/main/java/com/google/devtools/build/lib/unsafe:srcs",
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index 9d17a22..fa92053 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -100,9 +100,8 @@
* <li>A directory of unknown contents, but not a TreeArtifact. This is a legacy facility and
* should not be used by any new rule implementations. In particular, the file system cache
* integrity checks fail for directories.
- * <li>An 'aggregating middleman' special Artifact, which may be expanded using a {@link
- * ArtifactExpander} at Action execution time. This is used by a handful of rules to save
- * memory.
+ * <li>A middleman special Artifact, which may be expanded using a {@link ArtifactExpander} at
+ * Action execution time. This is used by a handful of rules to save memory.
* <li>A 'constant metadata' special Artifact. These represent real files, changes to which are
* ignored by the build system. They are useful for files which change frequently but do not
* affect the result of a build, such as timestamp files.
@@ -184,9 +183,8 @@
/**
* Returns a {@link SkyKey} that, when built, will produce this artifact. For source artifacts and
- * generated artifacts that may aggregate other artifacts (middleman, since they may be
- * aggregating middlemen, and tree), returns the artifact itself. For normal generated artifacts,
- * returns the key of the generating action.
+ * generated artifacts that may aggregate other artifacts, returns the artifact itself. For normal
+ * generated artifacts, returns the key of the generating action.
*
* <p>Callers should use this method (or the related ones below) in preference to directly
* requesting an {@link Artifact} to be built by Skyframe, since ordinary derived artifacts should
@@ -225,8 +223,8 @@
/**
* Expands the given artifact, and populates "output" with the result.
*
- * <p>{@code artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()} must be true.
- * Only aggregating middlemen and tree artifacts are expanded.
+ * <p>{@code artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()} must be true. Only
+ * middlemen and tree artifacts are expanded.
*/
void expand(Artifact artifact, Collection<? super Artifact> output);
@@ -1491,10 +1489,7 @@
return Joiner.on(delimiter).join(toRootRelativePaths(artifacts));
}
- /**
- * Adds a collection of artifacts to a given collection, with {@link
- * MiddlemanType#AGGREGATING_MIDDLEMAN} middleman actions expanded once.
- */
+ /** Adds a collection of artifacts to a given collection, with middleman actions expanded once. */
static void addExpandedArtifacts(
Iterable<Artifact> artifacts,
Collection<? super Artifact> output,
diff --git a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java
index 53d81e0..84b08f3 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/MiddlemanType.java
@@ -19,9 +19,6 @@
/** A normal action. */
NORMAL,
- /** A normal middleman, which just encapsulates a list of artifacts. */
- AGGREGATING_MIDDLEMAN,
-
/**
* A middleman that denotes a scheduling dependency.
*
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
index 4e6afaf..adc5992 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
@@ -75,9 +75,13 @@
Environment env,
MetadataConsumerForMetrics consumer)
throws InterruptedException {
- if (value instanceof AggregatingArtifactValue) {
- AggregatingArtifactValue aggregatingValue = (AggregatingArtifactValue) value;
- for (Pair<Artifact, FileArtifactValue> entry : aggregatingValue.getFileArtifacts()) {
+ if (value instanceof RunfilesArtifactValue) {
+ // Note: we don't expand the .runfiles/MANIFEST file into the inputs. The reason for that
+ // being that the MANIFEST file contains absolute paths that don't work with remote execution.
+ // Instead, the way the SpawnInputExpander expands runfiles is via the Runfiles class
+ // which contains all artifacts in the runfiles tree minus the MANIFEST file.
+ RunfilesArtifactValue runfilesArtifactValue = (RunfilesArtifactValue) value;
+ for (Pair<Artifact, FileArtifactValue> entry : runfilesArtifactValue.getFileArtifacts()) {
Artifact artifact = entry.first;
inputMap.put(artifact, entry.getSecond(), /*depOwner=*/ key);
if (artifact.isFileset()) {
@@ -91,7 +95,7 @@
consumer.accumulate(entry.getSecond());
}
}
- for (Pair<Artifact, TreeArtifactValue> entry : aggregatingValue.getTreeArtifacts()) {
+ for (Pair<Artifact, TreeArtifactValue> entry : runfilesArtifactValue.getTreeArtifacts()) {
expandTreeArtifactAndPopulateArtifactData(
entry.getFirst(),
Preconditions.checkNotNull(entry.getSecond()),
@@ -103,20 +107,7 @@
}
// We have to cache the "digest" of the aggregating value itself, because the action cache
// checker may want it.
- inputMap.put(key, aggregatingValue.getMetadata(), /*depOwner=*/ key);
- // While not obvious at all this code exists to ensure that we don't expand the
- // .runfiles/MANIFEST file into the inputs. The reason for that being that the MANIFEST
- // file contains absolute paths that don't work with remote execution.
- // Instead, the way the SpawnInputExpander expands runfiles is via the Runfiles class
- // which contains all artifacts in the runfiles tree minus the MANIFEST file.
- // TODO(buchgr): Clean this up and get rid of the RunfilesArtifactValue type.
- if (!(value instanceof RunfilesArtifactValue)) {
- ImmutableList.Builder<Artifact> expansionBuilder = ImmutableList.builder();
- for (Pair<Artifact, FileArtifactValue> pair : aggregatingValue.getFileArtifacts()) {
- expansionBuilder.add(Preconditions.checkNotNull(pair.getFirst()));
- }
- expandedArtifacts.put(key, expansionBuilder.build());
- }
+ inputMap.put(key, runfilesArtifactValue.getMetadata(), /*depOwner=*/ key);
} else if (value instanceof TreeArtifactValue) {
TreeArtifactValue treeArtifactValue = (TreeArtifactValue) value;
expandTreeArtifactAndPopulateArtifactData(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java
deleted file mode 100644
index e9978fb..0000000
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AggregatingArtifactValue.java
+++ /dev/null
@@ -1,85 +0,0 @@
-// Copyright 2014 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.skyframe;
-
-import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.util.Pair;
-import com.google.devtools.build.skyframe.SkyValue;
-import java.util.Collection;
-
-/** Value for aggregating artifacts, which must be expanded to a set of other artifacts. */
-class AggregatingArtifactValue implements SkyValue {
- private final ImmutableList<Pair<Artifact, FileArtifactValue>> fileInputs;
- private final ImmutableList<Pair<Artifact, TreeArtifactValue>> directoryInputs;
- private final FileArtifactValue metadata;
-
- AggregatingArtifactValue(
- ImmutableList<Pair<Artifact, FileArtifactValue>> fileInputs,
- ImmutableList<Pair<Artifact, TreeArtifactValue>> directoryInputs,
- FileArtifactValue metadata) {
- this.fileInputs = Preconditions.checkNotNull(fileInputs);
- this.directoryInputs = Preconditions.checkNotNull(directoryInputs);
- this.metadata = Preconditions.checkNotNull(metadata);
- }
-
- /** Returns the none tree artifacts that this artifact expands to, together with their data. */
- Collection<Pair<Artifact, FileArtifactValue>> getFileArtifacts() {
- return fileInputs;
- }
-
- /**
- * Returns the tree artifacts that this artifact expands to, together with the information
- * to which artifacts the tree artifacts expand to.
- */
- Collection<Pair<Artifact, TreeArtifactValue>> getTreeArtifacts() {
- return directoryInputs;
- }
-
- /** Returns the data of the artifact for this value, as computed by the action cache checker. */
- FileArtifactValue getMetadata() {
- return metadata;
- }
-
- @SuppressWarnings("EqualsGetClass") // RunfilesArtifactValue not equal to Aggregating.
- @Override
- public boolean equals(Object o) {
- if (this == o) {
- return true;
- }
- if (o == null || getClass() != o.getClass()) {
- return false;
- }
- AggregatingArtifactValue that = (AggregatingArtifactValue) o;
- return metadata.equals(that.metadata)
- && fileInputs.equals(that.fileInputs)
- && directoryInputs.equals(that.directoryInputs);
- }
-
- @Override
- public int hashCode() {
- return 31 * 31 * directoryInputs.hashCode() + 31 * fileInputs.hashCode() + metadata.hashCode();
- }
-
- @Override
- public String toString() {
- return MoreObjects.toStringHelper(this)
- .add("fileInputs", fileInputs)
- .add("directoryInputs", directoryInputs)
- .toString();
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index e9d01a91..fb64dae 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import static com.google.devtools.build.lib.actions.MiddlemanType.RUNFILES_MIDDLEMAN;
+
import com.google.common.base.MoreObjects;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
@@ -32,7 +34,6 @@
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.actions.FilesetTraversalParams.DirectTraversalRoot;
import com.google.devtools.build.lib.actions.FilesetTraversalParams.PackageBoundaryMode;
-import com.google.devtools.build.lib.actions.MiddlemanType;
import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
@@ -143,7 +144,7 @@
artifactDependencies);
FileArtifactValue individualMetadata = actionValue.getExistingFileArtifactValue(artifact);
if (isAggregatingValue(action)) {
- return createAggregatingValue(artifact, action, individualMetadata, env);
+ return createRunfilesArtifactValue(artifact, action, individualMetadata, env);
}
return individualMetadata;
}
@@ -340,7 +341,7 @@
}
@Nullable
- private static AggregatingArtifactValue createAggregatingValue(
+ private static RunfilesArtifactValue createRunfilesArtifactValue(
Artifact artifact,
ActionAnalysisMetadata action,
FileArtifactValue value,
@@ -367,9 +368,9 @@
} else if (inputValue instanceof TreeArtifactValue) {
directoryInputsBuilder.add(Pair.of(input, (TreeArtifactValue) inputValue));
} else {
- // We do not recurse in aggregating middleman artifacts.
+ // We do not recurse in middleman artifacts.
Preconditions.checkState(
- !(inputValue instanceof AggregatingArtifactValue),
+ !(inputValue instanceof RunfilesArtifactValue),
"%s %s %s",
artifact,
action,
@@ -386,24 +387,16 @@
Comparator.comparing(pair -> pair.getFirst().getExecPathString()),
directoryInputsBuilder.build());
- return (action.getActionType() == MiddlemanType.AGGREGATING_MIDDLEMAN)
- ? new AggregatingArtifactValue(fileInputs, directoryInputs, value)
- : new RunfilesArtifactValue(fileInputs, directoryInputs, value);
+ return new RunfilesArtifactValue(fileInputs, directoryInputs, value);
}
/**
* Returns whether this value needs to contain the data of all its inputs. Currently only tests to
- * see if the action is an aggregating or runfiles middleman action. However, may include Fileset
- * artifacts in the future.
+ * see if the action is a runfiles middleman action. However, may include Fileset artifacts in the
+ * future.
*/
private static boolean isAggregatingValue(ActionAnalysisMetadata action) {
- switch (action.getActionType()) {
- case AGGREGATING_MIDDLEMAN:
- case RUNFILES_MIDDLEMAN:
- return true;
- default:
- return false;
- }
+ return action.getActionType() == RUNFILES_MIDDLEMAN;
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 52c40f2..b6a9f23 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -421,7 +421,6 @@
srcs = ["ActionInputMapHelper.java"],
deps = [
":action_execution_value",
- ":aggregating_artifact_value",
":metadata_consumer_for_metrics",
":runfiles_artifact_value",
":tree_artifact_value",
@@ -756,8 +755,8 @@
)
java_library(
- name = "aggregating_artifact_value",
- srcs = ["AggregatingArtifactValue.java"],
+ name = "runfiles_artifact_value",
+ srcs = ["RunfilesArtifactValue.java"],
deps = [
":tree_artifact_value",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
@@ -789,7 +788,6 @@
deps = [
":action_execution_value",
":action_template_expansion_value",
- ":aggregating_artifact_value",
":coverage_report_value",
":detailed_exceptions",
":metadata_consumer_for_metrics",
@@ -2264,19 +2262,6 @@
)
java_library(
- name = "runfiles_artifact_value",
- srcs = ["RunfilesArtifactValue.java"],
- deps = [
- ":aggregating_artifact_value",
- ":tree_artifact_value",
- "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
- "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
- "//src/main/java/com/google/devtools/build/lib/util",
- "//third_party:guava",
- ],
-)
-
-java_library(
name = "sane_analysis_exception",
srcs = ["SaneAnalysisException.java"],
deps = [
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
index 5d78d76..afe3c78 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RunfilesArtifactValue.java
@@ -1,4 +1,4 @@
-// Copyright 2018 The Bazel Authors. All rights reserved.
+// Copyright 2014 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.
@@ -13,17 +13,73 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.skyframe.SkyValue;
+import java.util.Collection;
/** The artifacts behind a runfiles middleman. */
-final class RunfilesArtifactValue extends AggregatingArtifactValue {
+class RunfilesArtifactValue implements SkyValue {
+ private final ImmutableList<Pair<Artifact, FileArtifactValue>> fileInputs;
+ private final ImmutableList<Pair<Artifact, TreeArtifactValue>> directoryInputs;
+ private final FileArtifactValue metadata;
+
RunfilesArtifactValue(
ImmutableList<Pair<Artifact, FileArtifactValue>> fileInputs,
ImmutableList<Pair<Artifact, TreeArtifactValue>> directoryInputs,
FileArtifactValue metadata) {
- super(fileInputs, directoryInputs, metadata);
+ this.fileInputs = Preconditions.checkNotNull(fileInputs);
+ this.directoryInputs = Preconditions.checkNotNull(directoryInputs);
+ this.metadata = Preconditions.checkNotNull(metadata);
+ }
+
+ /** Returns the none tree artifacts that this artifact expands to, together with their data. */
+ Collection<Pair<Artifact, FileArtifactValue>> getFileArtifacts() {
+ return fileInputs;
+ }
+
+ /**
+ * Returns the tree artifacts that this artifact expands to, together with the information to
+ * which artifacts the tree artifacts expand to.
+ */
+ Collection<Pair<Artifact, TreeArtifactValue>> getTreeArtifacts() {
+ return directoryInputs;
+ }
+
+ /** Returns the data of the artifact for this value, as computed by the action cache checker. */
+ FileArtifactValue getMetadata() {
+ return metadata;
+ }
+
+ @SuppressWarnings("EqualsGetClass") // RunfilesArtifactValue not equal to Aggregating.
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ RunfilesArtifactValue that = (RunfilesArtifactValue) o;
+ return metadata.equals(that.metadata)
+ && fileInputs.equals(that.fileInputs)
+ && directoryInputs.equals(that.directoryInputs);
+ }
+
+ @Override
+ public int hashCode() {
+ return 31 * 31 * directoryInputs.hashCode() + 31 * fileInputs.hashCode() + metadata.hashCode();
+ }
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("fileInputs", fileInputs)
+ .add("directoryInputs", directoryInputs)
+ .toString();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/testing/actions/ArtifactExpanderHelper.java b/src/main/java/com/google/devtools/build/lib/testing/actions/ArtifactExpanderHelper.java
deleted file mode 100644
index 4cfa40a..0000000
--- a/src/main/java/com/google/devtools/build/lib/testing/actions/ArtifactExpanderHelper.java
+++ /dev/null
@@ -1,47 +0,0 @@
-// 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.testing.actions;
-
-import com.google.common.base.Functions;
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.actions.ActionGraph;
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
-import com.google.devtools.build.lib.actions.MiddlemanType;
-
-/** Helper utility to create {@link ArtifactExpander} instances. */
-public final class ArtifactExpanderHelper {
- private ArtifactExpanderHelper() {}
-
- public static ArtifactExpander actionGraphArtifactExpander(ActionGraph actionGraph) {
- return (mm, output) -> {
- // Skyframe is stricter in that it checks that "mm" is a input of the action, because
- // it cannot expand arbitrary middlemen without access to a global action graph.
- // We could check this constraint here too, but it seems unnecessary. This code is
- // going away anyway.
- Preconditions.checkArgument(mm.isMiddlemanArtifact(), "%s is not a middleman artifact", mm);
- ActionAnalysisMetadata middlemanAction = actionGraph.getGeneratingAction(mm);
- Preconditions.checkState(middlemanAction != null, mm);
- // TODO(bazel-team): Consider expanding recursively or throwing an exception here.
- // Most likely, this code will cause silent errors if we ever have a middleman that
- // contains a middleman.
- if (middlemanAction.getActionType() == MiddlemanType.AGGREGATING_MIDDLEMAN) {
- Artifact.addNonMiddlemanArtifacts(
- middlemanAction.getInputs().toList(), output, Functions.identity());
- }
- };
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/testing/actions/BUILD b/src/main/java/com/google/devtools/build/lib/testing/actions/BUILD
deleted file mode 100644
index b86183e..0000000
--- a/src/main/java/com/google/devtools/build/lib/testing/actions/BUILD
+++ /dev/null
@@ -1,26 +0,0 @@
-load("@rules_java//java:defs.bzl", "java_library")
-
-package(
- default_testonly = True,
- default_visibility = ["//src:__subpackages__"],
-)
-
-licenses(["notice"])
-
-filegroup(
- name = "srcs",
- testonly = False,
- srcs = glob(["**"]),
- visibility = ["//src:__subpackages__"],
-)
-
-java_library(
- name = "artifact_expander_helper",
- srcs = ["ArtifactExpanderHelper.java"],
- deps = [
- "//src/main/java/com/google/devtools/build/lib/actions",
- "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
- "//src/main/java/com/google/devtools/build/lib/actions:middleman_type",
- "//third_party:guava",
- ],
-)
