Inline CppCompileAction.execWithReply
This allows us to remove CppCompileActionResult, and also simplifies the
code.
This is in preparation for async C++ compile action execution.
Progress on #6394.
PiperOrigin-RevId: 239366249
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 0d66294..a2c1a67 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -64,7 +64,6 @@
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.rules.cpp.CcCommon.CoptsFilter;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
-import com.google.devtools.build.lib.rules.cpp.CppCompileActionResult.Reply;
import com.google.devtools.build.lib.rules.cpp.IncludeScanner.IncludeScanningHeaderData;
import com.google.devtools.build.lib.skyframe.ActionExecutionValue;
import com.google.devtools.build.lib.util.DependencySet;
@@ -1188,12 +1187,15 @@
}
}
- CppCompileActionResult.Reply reply;
List<SpawnResult> spawnResults;
+ byte[] dotDContents;
try (Defer ignored = new Defer()) {
- CppCompileActionResult cppCompileActionResult = execWithReply(spawnContext);
- reply = cppCompileActionResult.contextReply();
- spawnResults = cppCompileActionResult.spawnResults();
+ Spawn spawn = createSpawn(actionExecutionContext.getClientEnv());
+ SpawnActionContext context = actionExecutionContext.getContext(SpawnActionContext.class);
+ spawnResults = context.exec(spawn, spawnContext);
+ // SpawnActionContext guarantees that the first list entry exists and corresponds to the
+ // executed spawn.
+ dotDContents = getDotDContents(spawnResults.get(0));
} catch (ExecException e) {
throw e.toActionExecutionException(
"C++ compilation of rule '" + getOwner().getLabel() + "'",
@@ -1226,9 +1228,12 @@
} else {
discoveredInputs =
discoverInputsFromDotdFiles(
- actionExecutionContext, execRoot, scanningContext.getArtifactResolver(), reply);
+ actionExecutionContext,
+ execRoot,
+ scanningContext.getArtifactResolver(),
+ dotDContents);
}
- reply = null; // Clear in-memory .d files early.
+ dotDContents = null; // Garbage collect in-memory .d contents.
if (discoversInputs()) {
// Post-execute "include scanning", which modifies the action inputs to match what the compile
@@ -1251,8 +1256,21 @@
return ActionResult.create(spawnResults);
}
- protected CppCompileActionResult execWithReply(ActionExecutionContext actionExecutionContext)
- throws ExecException, InterruptedException {
+ protected byte[] getDotDContents(SpawnResult spawnResult) throws EnvironmentalExecException {
+ if (getDotdFile() != null) {
+ InputStream in = spawnResult.getInMemoryOutput(getDotdFile());
+ if (in != null) {
+ try {
+ return ByteStreams.toByteArray(in);
+ } catch (IOException e) {
+ throw new EnvironmentalExecException("Reading in-memory .d file failed", e);
+ }
+ }
+ }
+ return null;
+ }
+
+ protected Spawn createSpawn(Map<String, String> clientEnv) {
// Intentionally not adding {@link CppCompileAction#inputsForInvalidation}, those are not needed
// for execution.
ImmutableList.Builder<ActionInput> inputsBuilder =
@@ -1263,7 +1281,6 @@
inputsBuilder.add(getParamFileActionInput());
}
ImmutableList<ActionInput> inputs = inputsBuilder.build();
- clearAdditionalInputs();
ImmutableMap<String, String> executionInfo = getExecutionInfo();
ImmutableList.Builder<ActionInput> outputs =
@@ -1288,38 +1305,14 @@
.build();
}
- Spawn spawn =
- new SimpleSpawn(
- this,
- ImmutableList.copyOf(getArguments()),
- getEnvironment(actionExecutionContext.getClientEnv()),
- executionInfo,
- inputs,
- outputs.build(),
- estimateResourceConsumptionLocal());
-
- SpawnActionContext context = actionExecutionContext.getContext(SpawnActionContext.class);
- List<SpawnResult> spawnResults = context.exec(spawn, actionExecutionContext);
- // The SpawnActionContext guarantees that the first list entry is the successful one.
- SpawnResult spawnResult = spawnResults.get(0);
-
- CppCompileActionResult.Builder cppCompileActionResultBuilder =
- CppCompileActionResult.builder().setSpawnResults(spawnResults);
-
- if (getDotdFile() != null) {
- InputStream in = spawnResult.getInMemoryOutput(getDotdFile());
- if (in != null) {
- byte[] contents;
- try {
- contents = ByteStreams.toByteArray(in);
- } catch (IOException e) {
- throw new EnvironmentalExecException("Reading in-memory .d file failed", e);
- }
- cppCompileActionResultBuilder.setContextReply(
- new CppCompileActionResult.InMemoryFile(contents));
- }
- }
- return cppCompileActionResultBuilder.build();
+ return new SimpleSpawn(
+ this,
+ ImmutableList.copyOf(getArguments()),
+ getEnvironment(clientEnv),
+ executionInfo,
+ inputs,
+ outputs.build(),
+ estimateResourceConsumptionLocal());
}
@VisibleForTesting
@@ -1355,7 +1348,7 @@
ActionExecutionContext actionExecutionContext,
Path execRoot,
ArtifactResolver artifactResolver,
- Reply reply)
+ byte[] dotDContents)
throws ActionExecutionException {
if (!needsDotdInputPruning || getDotdFile() == null) {
return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
@@ -1365,7 +1358,7 @@
.setAction(this)
.setSourceFile(getSourceFile())
.setDependencies(
- processDepset(actionExecutionContext, execRoot, reply).getDependencies())
+ processDepset(actionExecutionContext, execRoot, dotDContents).getDependencies())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedinputs(getAllowedDerivedInputs());
@@ -1377,12 +1370,12 @@
}
public DependencySet processDepset(
- ActionExecutionContext actionExecutionContext, Path execRoot, Reply reply)
+ ActionExecutionContext actionExecutionContext, Path execRoot, byte[] dotDContents)
throws ActionExecutionException {
try {
DependencySet depSet = new DependencySet(execRoot);
- if (reply != null && cppConfiguration.getInmemoryDotdFiles()) {
- return depSet.process(reply.getContents());
+ if (dotDContents != null && cppConfiguration.getInmemoryDotdFiles()) {
+ return depSet.process(dotDContents);
}
return depSet.read(actionExecutionContext.getInputPath(getDotdFile()));
} catch (IOException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionResult.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionResult.java
deleted file mode 100644
index 076fff7..0000000
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionResult.java
+++ /dev/null
@@ -1,88 +0,0 @@
-// Copyright 2017 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.rules.cpp;
-
-import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.actions.SpawnResult;
-import java.io.IOException;
-import java.util.List;
-import javax.annotation.Nullable;
-
-/** Contains information about the result of a CppCompileAction's execution. */
-@AutoValue
-public abstract class CppCompileActionResult {
- /** Reply for the execution of a C++ compilation. */
- public interface Reply {
- /** Returns the contents of the .d file. */
- byte[] getContents() throws IOException;
- }
-
- /** A simple in-memory implementation of Reply. */
- public static class InMemoryFile implements Reply {
- private final byte[] contents;
-
- public InMemoryFile(byte[] contents) {
- this.contents = contents;
- }
-
- @Override
- public byte[] getContents() {
- return contents;
- }
- }
-
- /** Returns the SpawnResults created by the action, if any. */
- public abstract List<SpawnResult> spawnResults();
-
- /**
- * Gets the optional CppCompileActionContext.Reply for the action.
- *
- * <p>Could be null if there is no reply (e.g. if there is no .d file documenting which #include
- * statements are actually required.)
- */
- @Nullable
- public abstract Reply contextReply();
-
- /** Returns a builder that can be used to construct a {@link CppCompileActionResult} object. */
- public static Builder builder() {
- return new AutoValue_CppCompileActionResult.Builder();
- }
-
- /** Builder for a {@link CppCompileActionResult} instance, which is immutable once built. */
- @AutoValue.Builder
- public abstract static class Builder {
-
- /** Returns the SpawnResults for the action, if any. */
- abstract List<SpawnResult> spawnResults();
-
- /** Sets the SpawnResults for the action. */
- public abstract Builder setSpawnResults(List<SpawnResult> spawnResults);
-
- /** Sets the CppCompileActionContext.Reply for the action. */
- public abstract Builder setContextReply(Reply reply);
-
- abstract CppCompileActionResult realBuild();
-
- /**
- * Returns an immutable CppCompileActionResult object.
- *
- * <p>The list of SpawnResults is also made immutable here.
- */
- public CppCompileActionResult build() {
- return this.setSpawnResults(ImmutableList.copyOf(spawnResults())).realBuild();
- }
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
index cfe0cf9..b916960 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -29,6 +29,8 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ResourceSet;
+import com.google.devtools.build.lib.actions.Spawn;
+import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -131,16 +133,20 @@
// First, do a normal compilation, to generate the ".d" file. The generated object file is built
// to a temporary location (tempOutputFile) and ignored afterwards.
logger.info("Generating " + getDotdFile());
- CppCompileActionResult.Reply reply = null;
+ byte[] dotDContents = null;
try {
- CppCompileActionResult cppCompileActionResult = execWithReply(actionExecutionContext);
- reply = cppCompileActionResult.contextReply();
- spawnResults = cppCompileActionResult.spawnResults();
+ Spawn spawn = createSpawn(actionExecutionContext.getClientEnv());
+ SpawnActionContext context = actionExecutionContext.getContext(SpawnActionContext.class);
+ spawnResults = context.exec(spawn, actionExecutionContext);
+ // The SpawnActionContext guarantees that the first list entry is the successful one.
+ dotDContents = getDotDContents(spawnResults.get(0));
} catch (ExecException e) {
throw e.toActionExecutionException(
"C++ compilation of rule '" + getOwner().getLabel() + "'",
actionExecutionContext.getVerboseFailures(),
this);
+ } finally {
+ clearAdditionalInputs();
}
CppIncludeExtractionContext scanningContext =
actionExecutionContext.getContext(CppIncludeExtractionContext.class);
@@ -155,7 +161,7 @@
.setAction(this)
.setSourceFile(getSourceFile())
.setDependencies(
- processDepset(actionExecutionContext, execRoot, reply).getDependencies())
+ processDepset(actionExecutionContext, execRoot, dotDContents).getDependencies())
.setPermittedSystemIncludePrefixes(getPermittedSystemIncludePrefixes(execRoot))
.setAllowedDerivedinputs(getAllowedDerivedInputs());
@@ -169,7 +175,7 @@
.discoverInputsFromDependencies(execRoot, scanningContext.getArtifactResolver());
}
- reply = null; // Clear in-memory .d files early.
+ dotDContents = null; // Garbage collect in-memory .d contents.
// Even cc_fake_binary rules need to properly declare their dependencies...
// In fact, they need to declare their dependencies even more than cc_binary rules do.
