From 40bf169dfad2a3285d255c9100450b29be63202c Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Thu, 23 Mar 2017 18:26:08 +0000 Subject: [PATCH] Use SpawnInputExpander in the remote spawn strategy to expand runfiles trees This fixes remote test execution. Fixes #1593. -- PiperOrigin-RevId: 151030133 MOS_MIGRATED_REVID=151030133 --- .../build/lib/remote/RemoteSpawnStrategy.java | 28 ++++++++--- .../build/lib/remote/TreeNodeRepository.java | 15 ++++-- src/test/shell/bazel/remote_execution_test.sh | 46 ++++++++++++++----- 3 files changed, 65 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java index c4f788b36cd057..c8a86430808004 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java @@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionStatusMessage; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionStrategy; @@ -33,6 +32,7 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.exec.SpawnInputExpander; import com.google.devtools.build.lib.remote.ContentDigests.ActionKey; import com.google.devtools.build.lib.remote.RemoteProtocol.Action; import com.google.devtools.build.lib.remote.RemoteProtocol.ActionResult; @@ -43,9 +43,11 @@ import com.google.devtools.build.lib.remote.RemoteProtocol.ExecutionStatus; import com.google.devtools.build.lib.remote.RemoteProtocol.Platform; import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode; +import com.google.devtools.build.lib.rules.fileset.FilesetActionContext; import com.google.devtools.build.lib.standalone.StandaloneSpawnStrategy; import com.google.devtools.build.lib.util.io.FileOutErr; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; import com.google.protobuf.TextFormat; import com.google.protobuf.TextFormat.ParseException; import io.grpc.StatusRuntimeException; @@ -54,6 +56,7 @@ import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.SortedMap; import java.util.TreeSet; /** @@ -71,6 +74,7 @@ final class RemoteSpawnStrategy implements SpawnActionContext { private final RemoteOptions options; // TODO(olaola): This will be set on a per-action basis instead. private final Platform platform; + private final SpawnInputExpander spawnInputExpander = new SpawnInputExpander(/*strict=*/false); RemoteSpawnStrategy( Map clientEnv, @@ -135,7 +139,14 @@ private void execLocally( if (options.remoteLocalExecUploadResults && actionCache != null && actionKey != null) { ArrayList outputFiles = new ArrayList<>(); for (ActionInput output : spawn.getOutputFiles()) { - outputFiles.add(execRoot.getRelative(output.getExecPathString())); + Path outputFile = execRoot.getRelative(output.getExecPathString()); + // Ignore non-existent files. + // TODO(ulfjack): This is not ideal - in general, all spawn strategies should stat the + // output files and return a list of existing files. We shouldn't re-stat the files here. + if (!outputFile.exists()) { + continue; + } + outputFiles.add(outputFile); } try { ActionResult.Builder result = ActionResult.newBuilder(); @@ -223,10 +234,13 @@ public void exec(Spawn spawn, ActionExecutionContext actionExecutionContext) try { // Temporary hack: the TreeNodeRepository should be created and maintained upstream! TreeNodeRepository repository = new TreeNodeRepository(execRoot); - List inputs = - ActionInputHelper.expandArtifacts( - spawn.getInputFiles(), actionExecutionContext.getArtifactExpander()); - TreeNode inputRoot = repository.buildFromActionInputs(inputs); + SortedMap inputMap = + spawnInputExpander.getInputMapping( + spawn, + actionExecutionContext.getArtifactExpander(), + actionExecutionContext.getActionInputFileCache(), + actionExecutionContext.getExecutor().getContext(FilesetActionContext.class)); + TreeNode inputRoot = repository.buildFromActionInputs(inputMap); repository.computeMerkleDigests(inputRoot); Command command = buildCommand(spawn.getArguments(), spawn.getEnvironment()); Action action = @@ -261,7 +275,7 @@ public void exec(Spawn spawn, ActionExecutionContext actionExecutionContext) ExecuteRequest.newBuilder() .setAction(action) .setAcceptCached(acceptCachedResult) - .setTotalInputFileCount(inputs.size()) + .setTotalInputFileCount(inputMap.size()) .setTimeoutMillis(1000 * Spawns.getTimeoutSeconds(spawn, 120)); // TODO(olaola): set sensible local and remote timouts. ExecuteReply reply = workExecutor.executeRemotely(request.build()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java index 3ecf6fe24b60e2..8f663019866109 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java +++ b/src/main/java/com/google/devtools/build/lib/remote/TreeNodeRepository.java @@ -38,6 +38,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.SortedMap; import java.util.TreeMap; import javax.annotation.Nullable; @@ -217,16 +218,20 @@ public boolean apply(TreeNode node) { }); } + public TreeNode buildFromActionInputs(Iterable inputs) { + TreeMap sortedMap = new TreeMap<>(); + for (ActionInput input : inputs) { + sortedMap.put(new PathFragment(input.getExecPathString()), input); + } + return buildFromActionInputs(sortedMap); + } + /** * This function is a temporary and highly inefficient hack! It builds the tree from a ready list * of input files. TODO(olaola): switch to creating and maintaining the TreeNodeRepository based * on the build graph structure. */ - public TreeNode buildFromActionInputs(Iterable actionInputs) { - TreeMap sortedMap = new TreeMap<>(); - for (ActionInput input : actionInputs) { - sortedMap.put(new PathFragment(input.getExecPathString()), input); - } + public TreeNode buildFromActionInputs(SortedMap sortedMap) { ImmutableList.Builder> segments = ImmutableList.builder(); for (PathFragment path : sortedMap.keySet()) { segments.add(path.getSegments()); diff --git a/src/test/shell/bazel/remote_execution_test.sh b/src/test/shell/bazel/remote_execution_test.sh index a33805037fd499..9726ad07ed3c07 100755 --- a/src/test/shell/bazel/remote_execution_test.sh +++ b/src/test/shell/bazel/remote_execution_test.sh @@ -23,18 +23,6 @@ source "${CURRENT_DIR}/../integration_test_setup.sh" \ || { echo "integration_test_setup.sh not found!" >&2; exit 1; } function set_up() { - mkdir -p a - cat > a/BUILD < a/test.cc < -int main() { std::cout << "Hello world!" << std::endl; return 0; } -EOF work_path=$(mktemp -d ${TEST_TMPDIR}/remote.XXXXXXXX) pid_file=$(mktemp -u ${TEST_TMPDIR}/remote.XXXXXXXX) worker_port=$(pick_random_unused_tcp_port) || fail "no port found" @@ -64,6 +52,18 @@ function tear_down() { } function test_cc_binary() { + mkdir -p a + cat > a/BUILD < a/test.cc < +int main() { std::cout << "Hello world!" << std::endl; return 0; } +EOF bazel build //a:test >& $TEST_log \ || fail "Failed to build //a:test without remote execution" cp bazel-bin/a/test ${TEST_TMPDIR}/test_expected @@ -79,6 +79,28 @@ function test_cc_binary() { || fail "Remote execution generated different result" } +function test_cc_test() { + mkdir -p a + cat > a/BUILD < a/test.cc < +int main() { std::cout << "Hello test!" << std::endl; return 0; } +EOF + bazel --host_jvm_args=-Dbazel.DigestFunction=SHA1 test \ + --spawn_strategy=remote \ + --remote_worker=localhost:${worker_port} \ + --remote_cache=localhost:${worker_port} \ + --test_output=errors \ + //a:test >& $TEST_log \ + || fail "Failed to run //a:test with remote execution" +} + # TODO(alpha): Add a test that fails remote execution when remote worker # supports sandbox.