Turn --experimental_use_sandboxfs into a tristate (default still "no").
This is to allow automatically using sandboxfs if it is installed in the
system (when the flag is set to "auto").
The rationale for this is to eventually allow "auto" to become the default
value for this flag so that users can trivially gain a speed-up by
installing the necessary dependency on their system.
RELNOTES: None.
PiperOrigin-RevId: 240172952
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java b/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java
index 3765625..199568a 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/RealSandboxfsProcess.java
@@ -19,13 +19,16 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.io.ByteStreams;
import com.google.devtools.build.lib.shell.Subprocess;
import com.google.devtools.build.lib.shell.SubprocessBuilder;
+import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.BufferedReader;
import java.io.BufferedWriter;
+import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
@@ -93,6 +96,65 @@
}
/**
+ * Checks if the given sandboxfs binary is available and is valid.
+ *
+ * @param binary path to the sandboxfs binary that will later be used in the {@link #mount} call.
+ * @return true if the binary looks good, false otherwise
+ * @throws IOException if there is a problem trying to start the subprocess
+ */
+ static boolean isAvailable(PathFragment binary) {
+ Subprocess process;
+ try {
+ process =
+ new SubprocessBuilder()
+ .setArgv(binary.getPathString(), "--version")
+ .setStdout(StreamAction.STREAM)
+ .redirectErrorStream(true)
+ .start();
+ } catch (IOException e) {
+ log.warning("sandboxfs binary at " + binary + " seems to be missing; got error " + e);
+ return false;
+ }
+
+ ByteArrayOutputStream outErrBytes = new ByteArrayOutputStream();
+ try {
+ ByteStreams.copy(process.getInputStream(), outErrBytes);
+ } catch (IOException e) {
+ try {
+ outErrBytes.write(("Failed to read stdout: " + e).getBytes());
+ } catch (IOException e2) {
+ // Should not really have happened. There is nothing we can do.
+ }
+ }
+ String outErr = outErrBytes.toString().replaceFirst("\n$", "");
+
+ int exitCode = waitForProcess(process);
+ if (exitCode == 0) {
+ // TODO(jmmv): Validate the version number and ensure we support it. Would be nice to reuse
+ // the DottedVersion logic from the Apple rules.
+ if (outErr.matches("sandboxfs .*")) {
+ return true;
+ } else {
+ log.warning(
+ "sandboxfs binary at "
+ + binary
+ + " exists but doesn't seem valid; output was: "
+ + outErr);
+ return false;
+ }
+ } else {
+ log.warning(
+ "sandboxfs binary at "
+ + binary
+ + " returned non-zero exit code "
+ + exitCode
+ + " and output "
+ + outErr);
+ return false;
+ }
+ }
+
+ /**
* Mounts a new sandboxfs instance.
*
* <p>The root of the file system instance is left unmapped which means that it remains as
@@ -164,21 +226,18 @@
}
/**
- * Destroys a process and waits for it to exit.
+ * Waits for a process to terminate.
*
- * @param process the process to destroy.
+ * @param process the process to wait for
+ * @return the exit code of the terminated process
*/
- // TODO(jmmv): This is adapted from Worker.java. Should probably replace both with a new variant
- // of Uninterruptibles.callUninterruptibly that takes a lambda instead of a callable.
- private static void destroyProcess(Subprocess process) {
- process.destroy();
-
+ private static int waitForProcess(Subprocess process) {
boolean interrupted = false;
try {
while (true) {
try {
process.waitFor();
- return;
+ break;
} catch (InterruptedException ie) {
interrupted = true;
}
@@ -188,6 +247,19 @@
Thread.currentThread().interrupt();
}
}
+ return process.exitValue();
+ }
+
+ /**
+ * Destroys a process and waits for it to exit.
+ *
+ * @param process the process to destroy
+ */
+ // TODO(jmmv): This is adapted from Worker.java. Should probably replace both with a new variant
+ // of Uninterruptibles.callUninterruptibly that takes a lambda instead of a callable.
+ private static void destroyProcess(Subprocess process) {
+ process.destroy();
+ waitForProcess(process);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
index d368a08..f2e17f9 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxModule.java
@@ -37,6 +37,8 @@
import com.google.devtools.build.lib.exec.local.LocalExecutionOptions;
import com.google.devtools.build.lib.exec.local.LocalSpawnRunner;
import com.google.devtools.build.lib.exec.local.PosixLocalEnvProvider;
+import com.google.devtools.build.lib.profiler.Profiler;
+import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
@@ -46,6 +48,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.TriState;
import java.io.File;
import java.io.IOException;
import java.time.Duration;
@@ -117,6 +120,38 @@
}
}
+ /**
+ * Returns true if sandboxfs should be used for this build.
+ *
+ * <p>If the user set the use of sandboxfs as optional, this only returns true if the configured
+ * sandboxfs binary is present and valid. If the user requested the use of sandboxfs as mandatory,
+ * this throws an error if the binary is not valid.
+ *
+ * @param requested whether sandboxfs use was requested or not
+ * @param binary path of the sandboxfs binary to use
+ * @return true if sandboxfs can and should be used; false otherwise
+ * @throws IOException if there are problems trying to determine the status of sandboxfs
+ */
+ private boolean shouldUseSandboxfs(TriState requested, PathFragment binary) throws IOException {
+ switch (requested) {
+ case AUTO:
+ return RealSandboxfsProcess.isAvailable(binary);
+
+ case NO:
+ return false;
+
+ case YES:
+ if (!RealSandboxfsProcess.isAvailable(binary)) {
+ throw new IOException(
+ "sandboxfs explicitly requested but \""
+ + binary
+ + "\" could not be found or is not valid");
+ }
+ return true;
+ }
+ throw new IllegalStateException("Not reachable");
+ }
+
private void setup(CommandEnvironment cmdEnv, ExecutorBuilder builder)
throws IOException {
SandboxOptions options = checkNotNull(env.getOptions().getOptions(SandboxOptions.class));
@@ -147,8 +182,13 @@
sandboxBase.deleteTree();
}
+ PathFragment sandboxfsPath = PathFragment.create(options.sandboxfsPath);
+ boolean useSandboxfs;
+ try (SilentCloseable c = Profiler.instance().profile("shouldUseSandboxfs")) {
+ useSandboxfs = shouldUseSandboxfs(options.useSandboxfs, sandboxfsPath);
+ }
sandboxBase.createDirectoryAndParents();
- if (options.useSandboxfs) {
+ if (useSandboxfs) {
mountPoint.createDirectory();
Path logFile = sandboxBase.getRelative("sandboxfs.log");
@@ -156,9 +196,7 @@
if (options.sandboxDebug) {
env.getReporter().handle(Event.info("Mounting sandboxfs instance on " + mountPoint));
}
- sandboxfsProcess =
- RealSandboxfsProcess.mount(
- PathFragment.create(options.sandboxfsPath), mountPoint, logFile);
+ sandboxfsProcess = RealSandboxfsProcess.mount(sandboxfsPath, mountPoint, logFile);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
index b4d5561..93eb689 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
@@ -21,12 +21,14 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.Converter;
+import com.google.devtools.common.options.Converters.TriStateConverter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.devtools.common.options.OptionMetadataTag;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParsingException;
+import com.google.devtools.common.options.TriState;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
@@ -174,15 +176,17 @@
public List<ImmutableMap.Entry<String, String>> sandboxAdditionalMounts;
@Option(
- name = "experimental_use_sandboxfs",
- defaultValue = "false",
- documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
- effectTags = {OptionEffectTag.UNKNOWN},
- help =
- "Use sandboxfs to create the actions' execroot directories instead of building a symlink "
- + "tree."
- )
- public boolean useSandboxfs;
+ name = "experimental_use_sandboxfs",
+ converter = TriStateConverter.class,
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+ effectTags = {OptionEffectTag.UNKNOWN},
+ help =
+ "Use sandboxfs to create the actions' execroot directories instead of building a symlink "
+ + "tree. If \"yes\", the binary provided by --experimental_sandboxfs_path must be "
+ + "valid and correspond to a supported version of sandboxfs. If \"auto\", the binary "
+ + "may be missing or not compatible.")
+ public TriState useSandboxfs;
@Option(
name = "experimental_sandboxfs_path",
diff --git a/src/test/shell/integration/sandboxfs_test.sh b/src/test/shell/integration/sandboxfs_test.sh
index 63cc0ff..3fc6ea0 100755
--- a/src/test/shell/integration/sandboxfs_test.sh
+++ b/src/test/shell/integration/sandboxfs_test.sh
@@ -25,10 +25,11 @@
)
# Creates a fake sandboxfs process in "path" that logs interactions with it in
-# the given "log" file.
+# the given "log" file and reports the given "version".
function create_fake_sandboxfs() {
local path="${1}"; shift
local log="${1}"; shift
+ local version="${1}"; shift
cat >"${path}" <<EOF
#! /bin/sh
@@ -39,6 +40,11 @@
echo "PID: \${$}" >>"${log}"
echo "ARGS: \${*}" >>"${log}"
+if [ "\${1}" = --version ]; then
+ echo "${version}"
+ exit 0
+fi
+
while read line; do
echo "Received: \${line}" >>"${log}"
if [ -z "\${line}" ]; then
@@ -64,7 +70,7 @@
function test_default_sandboxfs_from_path() {
mkdir -p fake-tools
- create_fake_sandboxfs fake-tools/sandboxfs "$(pwd)/log"
+ create_fake_sandboxfs fake-tools/sandboxfs "$(pwd)/log" "sandboxfs 0.0.0"
PATH="$(pwd)/fake-tools:${PATH}"; export PATH
create_hello_package
@@ -97,7 +103,62 @@
--experimental_sandboxfs_path="/non-existent/sandboxfs" \
//hello >"${TEST_log}" 2>&1 && fail "Build succeeded but should have failed"
- expect_log "Failed to initialize sandbox: .*Cannot run .*/non-existent/"
+ expect_log "/non-existent/sandboxfs.*not be found"
+}
+
+function test_explicit_sandboxfs_is_invalid() {
+ mkdir -p fake-tools
+ create_hello_package
+ do_build() {
+ bazel build \
+ "${DISABLE_SANDBOX_ARGS[@]}" \
+ --experimental_use_sandboxfs=yes \
+ --experimental_sandboxfs_path="$(pwd)/fake-tools/sandboxfs" \
+ //hello
+ }
+
+ # Try with a binary that prints an invalid version string.
+ create_fake_sandboxfs fake-tools/sandboxfs "$(pwd)/log" "not-sandboxfs 0.0.0"
+ do_build >"${TEST_log}" 2>&1 && fail "Build should have failed"
+
+ # Now try with a valid binary just to ensure our test scenario works.
+ create_fake_sandboxfs fake-tools/sandboxfs "$(pwd)/log" "sandboxfs 0.0.0"
+ do_build >"${TEST_log}" 2>&1 || fail "Build should have succeeded"
+ sed -e 's,^,SANDBOXFS: ,' log >>"${TEST_log}"
+
+ grep -q "Terminated" log \
+ || fail "sandboxfs process was not terminated (not executed?)"
+}
+
+function test_use_sandboxfs_if_present() {
+ # This test relies on a PATH change that is only recognized when the server
+ # first starts up, so ensure there are no Bazel servers left behind.
+ #
+ # TODO(philwo): This is awful. The testing infrastructure should ensure
+ # that tests cannot pollute each other's state, but at the moment that's not
+ # the case.
+ bazel shutdown
+
+ mkdir -p fake-tools
+ PATH="$(pwd)/fake-tools:${PATH}"; export PATH
+ create_hello_package
+ do_build() {
+ bazel build \
+ "${DISABLE_SANDBOX_ARGS[@]}" \
+ --experimental_use_sandboxfs=auto \
+ //hello
+ }
+
+ # Try with sandboxfs not in the PATH.
+ do_build >"${TEST_log}" 2>&1 || fail "Build should have succeeded"
+ [[ ! -f log ]] || echo "sandboxfs was used but should not have"
+
+ # Now try with sandboxfs in the PATH to ensure our test scenario works.
+ create_fake_sandboxfs fake-tools/sandboxfs "$(pwd)/log" "sandboxfs 0.0.0"
+ do_build >"${TEST_log}" 2>&1 || fail "Build should have succeeded"
+ sed -e 's,^,SANDBOXFS: ,' log >>"${TEST_log}"
+ grep -q "Terminated" log \
+ || fail "sandboxfs process was not terminated (not executed?)"
}
# Runs a build of the given target using a fake sandboxfs that captures its
@@ -105,7 +166,7 @@
function build_with_fake_sandboxfs() {
local log="${1}"; shift
- create_fake_sandboxfs fake-sandboxfs.sh "${log}"
+ create_fake_sandboxfs fake-sandboxfs.sh "${log}" "sandboxfs 0.0.0"
local ret=0
bazel build \
