Skip to content

Commit

Permalink
Make all sandboxed strategies work with Filesets on their inputs.
Browse files Browse the repository at this point in the history 
This was broken by a556969 and partially fixed by 70691f2 . Only
partially, because in my infinite wisdom, I didn't realize that it's not only
the Linux sandbox that can have this problem.

To avoid further issues, I now plumb the package roots to every sandboxed spawn
runner, even the Windows one, even though Bazel doesn't have Filesets.

RELNOTES: None.
PiperOrigin-RevId: 495271813
Change-Id: I20626c3863bf56018c7c72ac4a21f7d02e3ffc34
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Dec 14, 2022
1 parent a0c054a commit 1829883
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 8 deletions.
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import java.io.BufferedWriter;
import java.io.File;
import java.io.IOException;
Expand Down Expand Up @@ -102,6 +103,7 @@ private static boolean computeIsSupported() throws InterruptedException {

private final SandboxHelpers helpers;
private final Path execRoot;
private final ImmutableList<Root> packageRoots;
private final boolean allowNetwork;
private final ProcessWrapper processWrapper;
private final Path sandboxBase;
Expand Down Expand Up @@ -139,6 +141,7 @@ private static boolean computeIsSupported() throws InterruptedException {
super(cmdEnv);
this.helpers = helpers;
this.execRoot = cmdEnv.getExecRoot();
this.packageRoots = cmdEnv.getPackageLocator().getPathEntries();
this.allowNetwork = helpers.shouldAllowNetwork(cmdEnv.getOptions());
this.alwaysWritableDirs = getAlwaysWritableDirs(cmdEnv.getRuntime().getFileSystem());
this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv);
Expand Down Expand Up @@ -236,7 +239,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
execRoot,
execRoot,
ImmutableList.of(),
packageRoots,
null);
SandboxOutputs outputs = helpers.getOutputs(spawn);

Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.google.devtools.build.lib.util.ProcessUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand Down Expand Up @@ -142,6 +143,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)

private final SandboxHelpers helpers;
private final Path execRoot;
private final ImmutableList<Root> packageRoots;
private final boolean allowNetwork;
private final Path dockerClient;
private final ProcessWrapper processWrapper;
Expand Down Expand Up @@ -179,6 +181,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv, Path dockerClient)
super(cmdEnv);
this.helpers = helpers;
this.execRoot = cmdEnv.getExecRoot();
this.packageRoots = cmdEnv.getPackageLocator().getPathEntries();
this.allowNetwork = helpers.shouldAllowNetwork(cmdEnv.getOptions());
this.dockerClient = dockerClient;
this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv);
Expand Down Expand Up @@ -225,7 +228,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
execRoot,
execRoot,
ImmutableList.of(),
packageRoots,
null);
SandboxOutputs outputs = helpers.getOutputs(spawn);

Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
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 com.google.devtools.build.lib.vfs.Root;
import java.io.IOException;
import java.time.Duration;
import javax.annotation.Nullable;
Expand All @@ -42,6 +43,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
private final SandboxHelpers helpers;
private final ProcessWrapper processWrapper;
private final Path execRoot;
private final ImmutableList<Root> packageRoots;
private final Path sandboxBase;
private final LocalEnvProvider localEnvProvider;
@Nullable private final SandboxfsProcess sandboxfsProcess;
Expand Down Expand Up @@ -69,6 +71,7 @@ public static boolean isSupported(CommandEnvironment cmdEnv) {
this.helpers = helpers;
this.processWrapper = ProcessWrapper.fromCommandEnvironment(cmdEnv);
this.execRoot = cmdEnv.getExecRoot();
this.packageRoots = cmdEnv.getPackageLocator().getPathEntries();
this.localEnvProvider = LocalEnvProvider.forCurrentOs(cmdEnv.getClientEnv());
this.sandboxBase = sandboxBase;
this.sandboxfsProcess = sandboxfsProcess;
Expand Down Expand Up @@ -115,7 +118,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
execRoot,
execRoot,
ImmutableList.of(),
packageRoots,
null);
SandboxOutputs outputs = helpers.getOutputs(spawn);

Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import java.io.IOException;
import java.time.Duration;

Expand All @@ -35,6 +36,7 @@ final class WindowsSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {

private final SandboxHelpers helpers;
private final Path execRoot;
private final ImmutableList<Root> packageRoots;
private final PathFragment windowsSandbox;
private final LocalEnvProvider localEnvProvider;
private final Duration timeoutKillDelay;
Expand All @@ -55,6 +57,7 @@ final class WindowsSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
super(cmdEnv);
this.helpers = helpers;
this.execRoot = cmdEnv.getExecRoot();
this.packageRoots = cmdEnv.getPackageLocator().getPathEntries();
this.windowsSandbox = windowsSandboxPath;
this.timeoutKillDelay = timeoutKillDelay;
this.localEnvProvider = new WindowsLocalEnvProvider(cmdEnv.getClientEnv());
Expand All @@ -75,7 +78,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
execRoot,
execRoot,
ImmutableList.of(),
packageRoots,
null);

ImmutableSet.Builder<Path> writablePaths = ImmutableSet.builder();
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ public void registerSpawnStrategies(
new WorkerSpawnRunner(
new SandboxHelpers(),
env.getExecRoot(),
env.getPackageLocator().getPathEntries(),
workerPool,
env.getReporter(),
localEnvProvider,
Expand Down
6 changes: 5 additions & 1 deletion src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.XattrProvider;
import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest;
import com.google.devtools.build.lib.worker.WorkerProtocol.WorkResponse;
Expand Down Expand Up @@ -90,6 +91,7 @@ final class WorkerSpawnRunner implements SpawnRunner {

private final SandboxHelpers helpers;
private final Path execRoot;
private final ImmutableList<Root> packageRoots;
private final WorkerPool workers;
private final ExtendedEventHandler reporter;
private final BinTools binTools;
Expand All @@ -104,6 +106,7 @@ final class WorkerSpawnRunner implements SpawnRunner {
public WorkerSpawnRunner(
SandboxHelpers helpers,
Path execRoot,
ImmutableList<Root> packageRoots,
WorkerPool workers,
ExtendedEventHandler reporter,
LocalEnvProvider localEnvProvider,
Expand All @@ -116,6 +119,7 @@ public WorkerSpawnRunner(
Clock clock) {
this.helpers = helpers;
this.execRoot = execRoot;
this.packageRoots = packageRoots;
this.workers = checkNotNull(workers);
this.reporter = reporter;
this.binTools = binTools;
Expand Down Expand Up @@ -191,7 +195,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
execRoot,
execRoot,
ImmutableList.of(),
packageRoots,
null);
}
SandboxOutputs outputs = helpers.getOutputs(spawn);
Expand Down
13 changes: 10 additions & 3 deletions src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept
new WorkerSpawnRunner(
new SandboxHelpers(),
fs.getPath("/execRoot"),
ImmutableList.of(),
createWorkerPool(),
reporter,
localEnvProvider,
Expand Down Expand Up @@ -198,6 +199,7 @@ public void testExecInWorker_virtualInputs_doesntQueryInputFileCache()
new WorkerSpawnRunner(
new SandboxHelpers(),
execRoot,
ImmutableList.of(),
createWorkerPool(),
reporter,
localEnvProvider,
Expand Down Expand Up @@ -258,12 +260,13 @@ public void testExecInWorker_finishesAsyncOnInterrupt()
new WorkerSpawnRunner(
new SandboxHelpers(),
fs.getPath("/execRoot"),
ImmutableList.of(),
createWorkerPool(),
reporter,
localEnvProvider,
/* binTools= */ null,
resourceManager,
/* runfilesTreeUpdater=*/ null,
/* runfilesTreeUpdater= */ null,
options,
metricsCollector,
SyscallCache.NO_CACHE,
Expand Down Expand Up @@ -309,12 +312,13 @@ public void testExecInWorker_sendsCancelMessageOnInterrupt()
new WorkerSpawnRunner(
new SandboxHelpers(),
fs.getPath("/execRoot"),
ImmutableList.of(),
createWorkerPool(),
reporter,
localEnvProvider,
/* binTools= */ null,
resourceManager,
/* runfilesTreeUpdater=*/ null,
/* runfilesTreeUpdater= */ null,
workerOptions,
metricsCollector,
SyscallCache.NO_CACHE,
Expand Down Expand Up @@ -375,12 +379,13 @@ public void testExecInWorker_unsandboxedDiesOnInterrupt()
new WorkerSpawnRunner(
new SandboxHelpers(),
fs.getPath("/execRoot"),
ImmutableList.of(),
createWorkerPool(),
reporter,
localEnvProvider,
/* binTools= */ null,
resourceManager,
/* runfilesTreeUpdater=*/ null,
/* runfilesTreeUpdater= */ null,
workerOptions,
metricsCollector,
SyscallCache.NO_CACHE,
Expand Down Expand Up @@ -427,6 +432,7 @@ public void testExecInWorker_noMultiplexWithDynamic()
new WorkerSpawnRunner(
new SandboxHelpers(),
fs.getPath("/execRoot"),
ImmutableList.of(),
createWorkerPool(),
reporter,
localEnvProvider,
Expand Down Expand Up @@ -476,6 +482,7 @@ private void assertRecordedResponsethrowsException(
new WorkerSpawnRunner(
new SandboxHelpers(),
fs.getPath("/execRoot"),
ImmutableList.of(),
createWorkerPool(),
reporter,
localEnvProvider,
Expand Down

0 comments on commit 1829883

Please sign in to comment.