From 0d55507712ff64fc4045c20ecad5712d5a6e8c4e Mon Sep 17 00:00:00 2001 From: ulfjack Date: Tue, 19 Mar 2019 05:32:53 -0700 Subject: [PATCH] Inline SpawnGccStrategy It is the only remaining implementation of CppCompileActionContext, which is hereby deleted. This is in preparation for making C++ action execution async. Progress on #6394. PiperOrigin-RevId: 239171395 --- .../lib/bazel/rules/BazelStrategyModule.java | 2 - .../build/lib/rules/cpp/CppCompileAction.java | 86 +++++++++++- .../rules/cpp/CppCompileActionContext.java | 45 ------- .../lib/rules/cpp/CppCompileActionResult.java | 24 +++- .../lib/rules/cpp/FakeCppCompileAction.java | 7 +- .../build/lib/rules/cpp/SpawnGccStrategy.java | 126 ------------------ .../lib/standalone/StandaloneModule.java | 2 - 7 files changed, 103 insertions(+), 189 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionContext.java delete mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java index 90cc493ffe526b..c86f31a7e84d2b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.remote.RemoteModule; import com.google.devtools.build.lib.remote.RemoteOptions; import com.google.devtools.build.lib.rules.android.WriteAdbArgsActionContext; -import com.google.devtools.build.lib.rules.cpp.CppCompileActionContext; import com.google.devtools.build.lib.rules.cpp.CppIncludeExtractionContext; import com.google.devtools.build.lib.rules.cpp.CppIncludeScanningContext; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -95,7 +94,6 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB } builder - .addStrategyByContext(CppCompileActionContext.class, "") .addStrategyByContext(CppIncludeExtractionContext.class, "") .addStrategyByContext(CppIncludeScanningContext.class, "") .addStrategyByContext(FileWriteActionContext.class, "") 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 bb442ada072c17..0d66294fddb0b5 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 @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.ActionEnvironment; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionExecutionException; +import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionLookupData; import com.google.devtools.build.lib.actions.ActionLookupValue; @@ -39,11 +40,15 @@ import com.google.devtools.build.lib.actions.CommandAction; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput; +import com.google.devtools.build.lib.actions.EnvironmentalExecException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionInfoSpecifier; import com.google.devtools.build.lib.actions.ExecutionRequirements; import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType; import com.google.devtools.build.lib.actions.ResourceSet; +import com.google.devtools.build.lib.actions.SimpleSpawn; +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.actions.extra.CppCompileInfo; import com.google.devtools.build.lib.actions.extra.EnvironmentVariable; @@ -59,7 +64,7 @@ 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.CppCompileActionContext.Reply; +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; @@ -1132,8 +1137,6 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) StandardCharsets.ISO_8859_1); } - CppCompileActionContext.Reply reply; - if (!shouldScanDotdFiles()) { updateActionInputs(NestedSetBuilder.wrap(Order.STABLE_ORDER, additionalInputs)); } @@ -1185,12 +1188,10 @@ public void close() throws IOException { } } + CppCompileActionResult.Reply reply; List spawnResults; try (Defer ignored = new Defer()) { - CppCompileActionResult cppCompileActionResult = - actionExecutionContext - .getContext(CppCompileActionContext.class) - .execWithReply(this, spawnContext); + CppCompileActionResult cppCompileActionResult = execWithReply(spawnContext); reply = cppCompileActionResult.contextReply(); spawnResults = cppCompileActionResult.spawnResults(); } catch (ExecException e) { @@ -1250,6 +1251,77 @@ public void close() throws IOException { return ActionResult.create(spawnResults); } + protected CppCompileActionResult execWithReply(ActionExecutionContext actionExecutionContext) + throws ExecException, InterruptedException { + // Intentionally not adding {@link CppCompileAction#inputsForInvalidation}, those are not needed + // for execution. + ImmutableList.Builder inputsBuilder = + new ImmutableList.Builder() + .addAll(getMandatoryInputs()) + .addAll(getAdditionalInputs()); + if (getParamFileActionInput() != null) { + inputsBuilder.add(getParamFileActionInput()); + } + ImmutableList inputs = inputsBuilder.build(); + clearAdditionalInputs(); + + ImmutableMap executionInfo = getExecutionInfo(); + ImmutableList.Builder outputs = + ImmutableList.builderWithExpectedSize(getOutputs().size() + 1); + outputs.addAll(getOutputs()); + if (getDotdFile() != null && useInMemoryDotdFiles()) { + outputs.add(getDotdFile()); + /* + * CppCompileAction does dotd file scanning locally inside the Bazel process and thus + * requires the dotd file contents to be available locally. In remote execution, we + * generally don't want to stage all remote outputs on the local file system and thus + * we need to tell the remote strategy (if any) to at least make the .d file available + * in-memory. We can do that via + * {@link ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS}. + */ + executionInfo = + ImmutableMap.builderWithExpectedSize(executionInfo.size() + 1) + .putAll(executionInfo) + .put( + ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS, + getDotdFile().getExecPathString()) + .build(); + } + + Spawn spawn = + new SimpleSpawn( + this, + ImmutableList.copyOf(getArguments()), + getEnvironment(actionExecutionContext.getClientEnv()), + executionInfo, + inputs, + outputs.build(), + estimateResourceConsumptionLocal()); + + SpawnActionContext context = actionExecutionContext.getContext(SpawnActionContext.class); + List 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(); + } + @VisibleForTesting public NestedSet discoverInputsFromShowIncludesFilters( Path execRoot, diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionContext.java deleted file mode 100644 index 804caf11d68c23..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionContext.java +++ /dev/null @@ -1,45 +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.rules.cpp; - -import com.google.devtools.build.lib.actions.ActionContext; -import com.google.devtools.build.lib.actions.ActionContextMarker; -import com.google.devtools.build.lib.actions.ActionExecutionContext; -import com.google.devtools.build.lib.actions.ExecException; -import java.io.IOException; - -/** - * Context for compiling plain C++. - */ -@ActionContextMarker(name = "C++") -public interface CppCompileActionContext extends ActionContext { - /** - * Reply for the execution of a C++ compilation. - */ - public interface Reply { - /** - * Returns the contents of the .d file. - */ - byte[] getContents() throws IOException; - } - - /** - * Executes the given action and return the reply of the executor. - * - * @return a CppCompileActionResult with information resulting from the action's execution - */ - CppCompileActionResult execWithReply( - CppCompileAction action, ActionExecutionContext actionExecutionContext) - throws ExecException, InterruptedException; -} 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 index 57daf406b9595a..076fff7360994b 100644 --- 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 @@ -17,12 +17,32 @@ 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 spawnResults(); @@ -34,7 +54,7 @@ public abstract class CppCompileActionResult { * statements are actually required.) */ @Nullable - public abstract CppCompileActionContext.Reply contextReply(); + public abstract Reply contextReply(); /** Returns a builder that can be used to construct a {@link CppCompileActionResult} object. */ public static Builder builder() { @@ -52,7 +72,7 @@ public abstract static class Builder { public abstract Builder setSpawnResults(List spawnResults); /** Sets the CppCompileActionContext.Reply for the action. */ - public abstract Builder setContextReply(CppCompileActionContext.Reply reply); + public abstract Builder setContextReply(Reply reply); abstract CppCompileActionResult 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 502ac6de034850..cfe0cf9e95b0ae 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 @@ -131,12 +131,9 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext) // 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()); - CppCompileActionContext context = - actionExecutionContext.getContext(CppCompileActionContext.class); - CppCompileActionContext.Reply reply = null; + CppCompileActionResult.Reply reply = null; try { - CppCompileActionResult cppCompileActionResult = - context.execWithReply(this, actionExecutionContext); + CppCompileActionResult cppCompileActionResult = execWithReply(actionExecutionContext); reply = cppCompileActionResult.contextReply(); spawnResults = cppCompileActionResult.spawnResults(); } catch (ExecException e) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java deleted file mode 100644 index 842b250661baf6..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java +++ /dev/null @@ -1,126 +0,0 @@ -// Copyright 2015 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.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.common.io.ByteStreams; -import com.google.devtools.build.lib.actions.ActionExecutionContext; -import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.EnvironmentalExecException; -import com.google.devtools.build.lib.actions.ExecException; -import com.google.devtools.build.lib.actions.ExecutionRequirements; -import com.google.devtools.build.lib.actions.ExecutionStrategy; -import com.google.devtools.build.lib.actions.SimpleSpawn; -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 java.io.IOException; -import java.io.InputStream; -import java.util.List; - -/** - * A context for C++ compilation that calls into a {@link SpawnActionContext}. - */ -@ExecutionStrategy( - contextType = CppCompileActionContext.class, - name = {"spawn"} -) -public class SpawnGccStrategy implements CppCompileActionContext { - private static class InMemoryFile implements CppCompileActionContext.Reply { - private final byte[] contents; - - InMemoryFile(byte[] contents) { - this.contents = contents; - } - - @Override - public byte[] getContents() { - return contents; - } - } - - @Override - public CppCompileActionResult execWithReply( - CppCompileAction action, ActionExecutionContext actionExecutionContext) - throws ExecException, InterruptedException { - // Intentionally not adding {@link CppCompileAction#inputsForInvalidation}, those are not needed - // for execution. - ImmutableList.Builder inputsBuilder = - new ImmutableList.Builder() - .addAll(action.getMandatoryInputs()) - .addAll(action.getAdditionalInputs()); - if (action.getParamFileActionInput() != null) { - inputsBuilder.add(action.getParamFileActionInput()); - } - ImmutableList inputs = inputsBuilder.build(); - action.clearAdditionalInputs(); - - ImmutableMap executionInfo = action.getExecutionInfo(); - ImmutableList.Builder outputs = - ImmutableList.builderWithExpectedSize(action.getOutputs().size() + 1); - outputs.addAll(action.getOutputs()); - if (action.getDotdFile() != null && action.useInMemoryDotdFiles()) { - outputs.add(action.getDotdFile()); - /* - * CppCompileAction does dotd file scanning locally inside the Bazel process and thus - * requires the dotd file contents to be available locally. In remote execution, we - * generally don't want to stage all remote outputs on the local file system and thus - * we need to tell the remote strategy (if any) to at least make the .d file available - * in-memory. We can do that via - * {@link ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS}. - */ - executionInfo = - ImmutableMap.builderWithExpectedSize(executionInfo.size() + 1) - .putAll(executionInfo) - .put( - ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS, - action.getDotdFile().getExecPathString()) - .build(); - } - - Spawn spawn = - new SimpleSpawn( - action, - ImmutableList.copyOf(action.getArguments()), - action.getEnvironment(actionExecutionContext.getClientEnv()), - executionInfo, - inputs, - outputs.build(), - action.estimateResourceConsumptionLocal()); - - SpawnActionContext context = actionExecutionContext.getContext(SpawnActionContext.class); - List 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 (action.getDotdFile() != null) { - InputStream in = spawnResult.getInMemoryOutput(action.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 InMemoryFile(contents)); - } - } - return cppCompileActionResultBuilder.build(); - } -} diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java index 1c44b3a0fe0279..bc08cbc92bd219 100644 --- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java +++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneModule.java @@ -31,7 +31,6 @@ import com.google.devtools.build.lib.exec.local.LocalSpawnRunner; import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider; import com.google.devtools.build.lib.exec.local.WindowsLocalEnvProvider; -import com.google.devtools.build.lib.rules.cpp.SpawnGccStrategy; import com.google.devtools.build.lib.rules.test.ExclusiveTestStrategy; import com.google.devtools.build.lib.runtime.BlazeModule; import com.google.devtools.build.lib.runtime.CommandEnvironment; @@ -62,7 +61,6 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB // last one from strategies list will be used builder.addActionContext( new StandaloneSpawnStrategy(env.getExecRoot(), createLocalRunner(env))); - builder.addActionContext(new SpawnGccStrategy()); builder.addActionContext(testStrategy); builder.addActionContext(new ExclusiveTestStrategy(testStrategy)); builder.addActionContext(new FileWriteStrategy());