From 6b88e159d97df777716d5fb3eb137f305882602d Mon Sep 17 00:00:00 2001 From: Yue Gan Date: Tue, 12 Apr 2016 14:10:27 +0000 Subject: More detailed error message for sandbox failures in test, when --sandbox_debug and --verbose_failures are on. See discussion in #1049. RELNOTES: -- MOS_MIGRATED_REVID=119635080 --- .../devtools/build/lib/actions/SpawnActionContext.java | 7 +++++++ .../devtools/build/lib/remote/RemoteSpawnStrategy.java | 5 +++++ .../build/lib/rules/test/StandaloneTestStrategy.java | 9 +++++++-- .../build/lib/sandbox/LinuxSandboxedStrategy.java | 11 +++++++++-- .../build/lib/standalone/StandaloneSpawnStrategy.java | 5 +++++ .../devtools/build/lib/worker/WorkerSpawnStrategy.java | 5 +++++ src/test/shell/bazel/bazel_sandboxing_test.sh | 16 ++++++++++++++++ 7 files changed, 54 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java index 5e20f85684..e0deb85505 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java +++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnActionContext.java @@ -33,4 +33,11 @@ public interface SpawnActionContext extends Executor.ActionContext { * execute spawns remotely, i.e., force remote execution. */ boolean willExecuteRemotely(boolean remotable); + + /** + * If an ExecException should be rethrown by the strategy that executed this. + * Currently only works for LinuxSandboxedStrategy: + * If true, will throw ExecException and give reproduction instruction for sandbox. + */ + boolean shouldPropagateExecException(); } 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 b5f4326ba2..f688fd2191 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 @@ -259,4 +259,9 @@ final class RemoteSpawnStrategy implements SpawnActionContext { // Returning true here just helps to estimate the cost of this computation is zero. return remotable; } + + @Override + public boolean shouldPropagateExecException() { + return false; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java index a474521544..a12084a813 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.actions.ResourceManager; import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle; 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.TestExecException; import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl; import com.google.devtools.build.lib.analysis.config.BinTools; @@ -166,13 +167,14 @@ public class StandaloneTestStrategy extends TestStrategy { private TestResultData execute( ActionExecutionContext actionExecutionContext, Spawn spawn, TestRunnerAction action) - throws TestExecException, InterruptedException { + throws ExecException, InterruptedException { Executor executor = actionExecutionContext.getExecutor(); Closeable streamed = null; Path testLogPath = action.getTestLog().getPath(); TestResultData.Builder builder = TestResultData.newBuilder(); long startTime = executor.getClock().currentTimeMillis(); + SpawnActionContext spawnActionContext = executor.getSpawnActionContext(action.getMnemonic()); try { try { if (executionOptions.testOutput.equals(TestOutputFormat.STREAMED)) { @@ -180,7 +182,7 @@ public class StandaloneTestStrategy extends TestStrategy { Reporter.outErrForReporter( actionExecutionContext.getExecutor().getEventHandler()), testLogPath); } - executor.getSpawnActionContext(action.getMnemonic()).exec(spawn, actionExecutionContext); + spawnActionContext.exec(spawn, actionExecutionContext); builder.setTestPassed(true) .setStatus(BlazeTestStatus.PASSED) @@ -195,6 +197,9 @@ public class StandaloneTestStrategy extends TestStrategy { .setTestPassed(false) .setStatus(e.hasTimedOut() ? BlazeTestStatus.TIMEOUT : BlazeTestStatus.FAILED) .addFailedLogs(testLogPath.getPathString()); + if (spawnActionContext.shouldPropagateExecException()) { + throw e; + } } finally { long duration = executor.getClock().currentTimeMillis() - startTime; builder.addTestTimes(duration); diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java index 99d7183182..4aabf17897 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java @@ -68,8 +68,10 @@ import java.util.concurrent.atomic.AtomicInteger; /** * Strategy that uses sandboxing to execute a process. */ -@ExecutionStrategy(name = {"sandboxed"}, - contextType = SpawnActionContext.class) +@ExecutionStrategy( + name = {"sandboxed"}, + contextType = SpawnActionContext.class +) public class LinuxSandboxedStrategy implements SpawnActionContext { private final ExecutorService backgroundWorkers; @@ -684,4 +686,9 @@ public class LinuxSandboxedStrategy implements SpawnActionContext { public String toString() { return "sandboxed"; } + + @Override + public boolean shouldPropagateExecException() { + return verboseFailures && sandboxDebug; + } } diff --git a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java index 9c269633c9..6f009af507 100644 --- a/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategy.java @@ -196,4 +196,9 @@ public class StandaloneSpawnStrategy implements SpawnActionContext { } return AppleHostInfo.getSdkRoot(execRoot, developerDir, iosSdkVersion, appleSdkPlatform); } + + @Override + public boolean shouldPropagateExecException() { + return false; + } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java index 3433e98706..dd07b80013 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java @@ -269,4 +269,9 @@ public final class WorkerSpawnStrategy implements SpawnActionContext { public boolean willExecuteRemotely(boolean remotable) { return false; } + + @Override + public boolean shouldPropagateExecException() { + return false; + } } diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh index d0f9089764..27097bc9c1 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -418,6 +418,22 @@ function test_sandbox_add_path_workspace_child() { expect_log "Mounting subdirectory of WORKSPACE or OUTPUTBASE to sandbox is not allowed" } +function test_sandbox_fail_command() { + mkdir -p "javatests/orange" + echo "java_test(name = 'Orange', srcs = ['Orange.java'])" > javatests/orange/BUILD + cat > javatests/orange/Orange.java <& $TEST_log \ + && fail "Expected failure" || true + + expect_log "Sandboxed execution failed, which may be legitimate" +} + # The test shouldn't fail if the environment doesn't support running it. check_supported_platform || exit 0 check_sandbox_allowed || exit 0 -- cgit v1.2.3