Turn ProcessWrapperUtil helper functions into a ProcessWrapper class.
This will make it easier to carry the process-wrapper configuration around.
In particular, I want to add the ability to pass arbitrary extra flags to
the tool, and carrying them around explicitly and separately from the
process-wrapper path adds a lot of noise everywhere.
This also cleans up the way we gain access to the process-wrapper binary
and the checks to see if it is supported by homogenizing all logic into
a single place and removing OS-specific checks from a variety of places.
Part of https://github.com/bazelbuild/bazel/issues/10245.
RELNOTES: None.
PiperOrigin-RevId: 310908570
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
index d75521e..416be8b 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
@@ -162,8 +162,8 @@
environment,
ImmutableMap.of("FOO", "BAR"),
downloader,
- null,
1.0,
+ /*processWrapper=*/ null,
new HashMap<>(),
starlarkSemantics,
repoRemoteExecutor);
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/BUILD b/src/test/java/com/google/devtools/build/lib/exec/local/BUILD
index fa14ab8..c6b9e86 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/BUILD
@@ -21,6 +21,7 @@
"//src/test/shell/integration:spend_cpu_time",
],
deps = [
+ "//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
"//src/main/java/com/google/devtools/build/lib/exec:bin_tools",
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
index 6bde2b5..5a0158e 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -47,6 +47,7 @@
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.exec.util.SpawnBuilder;
+import com.google.devtools.build.lib.runtime.ProcessWrapper;
import com.google.devtools.build.lib.shell.JavaSubprocessFactory;
import com.google.devtools.build.lib.shell.Subprocess;
import com.google.devtools.build.lib.shell.SubprocessBuilder;
@@ -58,7 +59,6 @@
import com.google.devtools.build.lib.unix.UnixFileSystem;
import com.google.devtools.build.lib.util.NetUtil;
import com.google.devtools.build.lib.util.OS;
-import com.google.devtools.build.lib.util.OsUtils;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.FileSystem;
@@ -99,30 +99,21 @@
/** Unit tests for {@link LocalSpawnRunner}. */
@RunWith(JUnit4.class)
public class LocalSpawnRunnerTest {
- private static final boolean USE_WRAPPER = true;
- private static final boolean NO_WRAPPER = false;
private static class TestedLocalSpawnRunner extends LocalSpawnRunner {
public TestedLocalSpawnRunner(
Path execRoot,
- Path embeddedBin,
LocalExecutionOptions localExecutionOptions,
ResourceManager resourceManager,
- boolean useProcessWrapper,
- OS localOs,
+ ProcessWrapper processWrapper,
LocalEnvProvider localEnvProvider) {
super(
execRoot,
localExecutionOptions,
resourceManager,
- useProcessWrapper,
- localOs,
localEnvProvider,
- useProcessWrapper
- ? BinTools.forEmbeddedBin(
- embeddedBin,
- ImmutableList.of("process-wrapper" + OsUtils.executableExtension(localOs)))
- : null,
+ /*binTools=*/ null,
+ processWrapper,
Mockito.mock(RunfilesTreeUpdater.class));
}
@@ -321,6 +312,10 @@
return new InMemoryFileSystem();
}
+ private static ProcessWrapper makeProcessWrapper(FileSystem fs) {
+ return new ProcessWrapper(fs.getPath("/process-wrapper"));
+ }
+
/**
* Enables real execution by default.
*
@@ -351,11 +346,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -372,11 +365,7 @@
assertThat(captor.getValue().getArgv())
.containsExactlyElementsIn(
ImmutableList.of(
- "/embedded_bin/process-wrapper",
- "--timeout=123",
- "--kill_delay=456",
- "/bin/echo",
- "Hi!"));
+ "/process-wrapper", "--timeout=123", "--kill_delay=456", "/bin/echo", "Hi!"));
assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
assertThat(captor.getValue().getTimeoutMillis()).isEqualTo(0);
assertThat(captor.getValue().getStdout()).isEqualTo(StreamAction.REDIRECT);
@@ -408,11 +397,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
execRoot,
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
ParamFileActionInput paramFileActionInput =
new ParamFileActionInput(
@@ -462,11 +449,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- NO_WRAPPER,
- OS.LINUX,
+ /*processWrapper=*/ null,
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -507,11 +492,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
@@ -528,11 +511,7 @@
.containsExactlyElementsIn(
ImmutableList.of(
// process-wrapper timeout grace_time stdout stderr
- "/embedded_bin/process-wrapper",
- "--timeout=0",
- "--kill_delay=15",
- "/bin/echo",
- "Hi!"));
+ "/process-wrapper", "--timeout=0", "--kill_delay=15", "/bin/echo", "Hi!"));
assertThat(captor.getValue().getEnv()).containsExactly("VARIABLE", "value");
assertThat(captor.getValue().getStdout()).isEqualTo(StreamAction.REDIRECT);
assertThat(captor.getValue().getStdoutFile()).isEqualTo(new File("/out/stdout"));
@@ -555,11 +534,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
assertThat(fs.getPath("/out").createDirectory()).isTrue();
@@ -591,11 +568,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
@@ -642,11 +617,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -671,10 +644,9 @@
tempDir,
Options.getDefaults(LocalExecutionOptions.class),
resourceManager,
- /*useProcessWrapper=*/ false,
- OS.LINUX,
LocalEnvProvider.forCurrentOs(ImmutableMap.of()),
/*binTools=*/ null,
+ /*processWrapper=*/ null,
Mockito.mock(RunfilesTreeUpdater.class));
FileOutErr fileOutErr =
new FileOutErr(tempDir.getRelative("stdout"), tempDir.getRelative("stderr"));
@@ -755,11 +727,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -782,11 +752,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -813,11 +781,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
+ makeProcessWrapper(fs),
localEnvProvider);
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
@@ -833,51 +799,6 @@
matches("^/execroot/tmp[0-9a-fA-F]+_[0-9a-fA-F]+/work$"));
}
- @Test
- public void useCorrectExtensionOnWindows() throws Exception {
- // TODO(#3536): Make this test work on Windows.
- // The Command API implicitly absolutizes the path, and we get weird paths on Windows:
- // T:\execroot\execroot\_bin\process-wrapper.exe
- assumeTrue(OS.getCurrent() != OS.WINDOWS);
-
- FileSystem fs = setupEnvironmentForFakeExecution();
-
- SubprocessFactory factory = mock(SubprocessFactory.class);
- ArgumentCaptor<SubprocessBuilder> captor = ArgumentCaptor.forClass(SubprocessBuilder.class);
- when(factory.create(captor.capture())).thenReturn(new FinishedSubprocess(0));
- SubprocessBuilder.setDefaultSubprocessFactory(factory);
-
- LocalExecutionOptions options = Options.getDefaults(LocalExecutionOptions.class);
- options.localSigkillGraceSeconds = 654;
- LocalSpawnRunner runner =
- new TestedLocalSpawnRunner(
- fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
- options,
- resourceManager,
- USE_WRAPPER,
- OS.WINDOWS,
- LocalSpawnRunnerTest::keepLocalEnvUnchanged);
-
- FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
- SpawnExecutionContextForTesting policy = new SpawnExecutionContextForTesting(fileOutErr);
- policy.timeoutMillis = 321 * 1000L;
- assertThat(fs.getPath("/execroot").createDirectory()).isTrue();
- SpawnResult result = runner.execAsync(SIMPLE_SPAWN, policy).get();
- verify(factory).create(any(SubprocessBuilder.class));
- assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS);
-
- assertThat(captor.getValue().getArgv())
- .containsExactlyElementsIn(
- ImmutableList.of(
- // process-wrapper timeout grace_time stdout stderr
- "/embedded_bin/process-wrapper.exe",
- "--timeout=321",
- "--kill_delay=654",
- "/bin/echo",
- "Hi!"));
- }
-
/**
* Copies the {@code process-wrapper} tool into the path under the temporary execRoot where the
* {@link LocalSpawnRunner} expects to find it.
@@ -978,7 +899,8 @@
Path embeddedBinaries = getTemporaryEmbeddedBin(fs);
BinTools binTools = BinTools.forEmbeddedBin(embeddedBinaries,
ImmutableList.of("process-wrapper"));
- copyProcessWrapperIntoExecRoot(binTools.getEmbeddedPath("process-wrapper"));
+ Path processWrapperPath = binTools.getEmbeddedPath("process-wrapper");
+ copyProcessWrapperIntoExecRoot(processWrapperPath);
Path cpuTimeSpenderPath = copyCpuTimeSpenderIntoExecRoot(execRoot);
LocalSpawnRunner runner =
@@ -986,10 +908,9 @@
execRoot,
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
LocalSpawnRunnerTest::keepLocalEnvUnchanged,
binTools,
+ new ProcessWrapper(processWrapperPath),
Mockito.mock(RunfilesTreeUpdater.class));
Spawn spawn =
@@ -1042,7 +963,8 @@
Path embeddedBinaries = getTemporaryEmbeddedBin(fs);
BinTools binTools = BinTools.forEmbeddedBin(embeddedBinaries,
ImmutableList.of("process-wrapper"));
- copyProcessWrapperIntoExecRoot(binTools.getEmbeddedPath("process-wrapper"));
+ Path processWrapperPath = binTools.getEmbeddedPath("process-wrapper");
+ copyProcessWrapperIntoExecRoot(processWrapperPath);
Path cpuTimeSpenderPath = copyCpuTimeSpenderIntoExecRoot(execRoot);
LocalSpawnRunner runner =
@@ -1050,10 +972,9 @@
execRoot,
options,
resourceManager,
- USE_WRAPPER,
- OS.LINUX,
LocalSpawnRunnerTest::keepLocalEnvUnchanged,
binTools,
+ new ProcessWrapper(processWrapperPath),
Mockito.mock(RunfilesTreeUpdater.class));
Spawn spawn =
@@ -1102,11 +1023,9 @@
LocalSpawnRunner runner =
new TestedLocalSpawnRunner(
fs.getPath("/execroot"),
- fs.getPath("/embedded_bin"),
Options.getDefaults(LocalExecutionOptions.class),
resourceManager,
- NO_WRAPPER,
- OS.LINUX,
+ /*processWrapper=*/ null,
LocalSpawnRunnerTest::keepLocalEnvUnchanged);
FileOutErr fileOutErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperTest.java
similarity index 72%
rename from src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java
rename to src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperTest.java
index ff1fdd5..63b0746 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/ProcessWrapperTest.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.io.IOException;
import java.time.Duration;
import java.util.List;
import org.junit.Before;
@@ -27,35 +28,41 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Unit tests for {@link ProcessWrapperUtil}. */
+/** Unit tests for {@link ProcessWrapper}. */
@RunWith(JUnit4.class)
-public final class ProcessWrapperUtilTest {
+public final class ProcessWrapperTest {
+
private FileSystem testFS;
@Before
- public final void createFileSystem() {
+ public void setUp() {
testFS = new InMemoryFileSystem();
}
- @Test
- public void testProcessWrapperCommandLineBuilder_BuildsWithoutOptionalArguments() {
- String processWrapperPath = "process-wrapper";
+ private ProcessWrapper getProcessWrapper(String path) throws IOException {
+ Path processWrapperPath = testFS.getPath(path);
+ processWrapperPath.getParentDirectory().createDirectoryAndParents();
+ processWrapperPath.getOutputStream().close();
+ return new ProcessWrapper(processWrapperPath);
+ }
+ @Test
+ public void testProcessWrapperCommandLineBuilder_BuildsWithoutOptionalArguments()
+ throws IOException {
ImmutableList<String> commandArguments = ImmutableList.of("echo", "hello, world");
ImmutableList<String> expectedCommandLine =
- ImmutableList.<String>builder().add(processWrapperPath).addAll(commandArguments).build();
+ ImmutableList.<String>builder().add("/some/bin/path").addAll(commandArguments).build();
- List<String> commandLine =
- ProcessWrapperUtil.commandLineBuilder(processWrapperPath, commandArguments).build();
+ ProcessWrapper processWrapper = getProcessWrapper("/some/bin/path");
+ List<String> commandLine = processWrapper.commandLineBuilder(commandArguments).build();
assertThat(commandLine).containsExactlyElementsIn(expectedCommandLine).inOrder();
}
@Test
- public void testProcessWrapperCommandLineBuilder_BuildsWithOptionalArguments() {
- String processWrapperPath = "process-wrapper";
-
+ public void testProcessWrapperCommandLineBuilder_BuildsWithOptionalArguments()
+ throws IOException {
ImmutableList<String> commandArguments = ImmutableList.of("echo", "hello, world");
Duration timeout = Duration.ofSeconds(10);
@@ -66,7 +73,7 @@
ImmutableList<String> expectedCommandLine =
ImmutableList.<String>builder()
- .add(processWrapperPath)
+ .add("/path/to/process-wrapper")
.add("--timeout=" + timeout.getSeconds())
.add("--kill_delay=" + killDelay.getSeconds())
.add("--stdout=" + stdoutPath)
@@ -75,8 +82,10 @@
.addAll(commandArguments)
.build();
+ ProcessWrapper processWrapper = getProcessWrapper("/path/to/process-wrapper");
List<String> commandLine =
- ProcessWrapperUtil.commandLineBuilder(processWrapperPath, commandArguments)
+ processWrapper
+ .commandLineBuilder(commandArguments)
.setTimeout(timeout)
.setKillDelay(killDelay)
.setStdoutPath(stdoutPath)
diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java
index a7c32c1..e6e87e8 100644
--- a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java
@@ -17,7 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.runtime.ProcessWrapperUtil;
+import com.google.devtools.build.lib.runtime.ProcessWrapper;
import com.google.devtools.build.lib.testutil.BlazeTestUtils;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.testutil.TestUtils;
@@ -43,8 +43,11 @@
testFS = new UnixFileSystem(DigestHashFunction.getDefaultUnchecked());
}
- private String getProcessWrapperPath() {
- return BlazeTestUtils.runfilesDir() + "/" + TestConstants.PROCESS_WRAPPER_PATH;
+ private ProcessWrapper getProcessWrapper() {
+ return new ProcessWrapper(
+ testFS
+ .getPath(BlazeTestUtils.runfilesDir())
+ .getRelative(TestConstants.PROCESS_WRAPPER_PATH));
}
private String getCpuTimeSpenderPath() {
@@ -66,8 +69,7 @@
public void testProcessWrappedCommand_Echo() throws Exception {
ImmutableList<String> commandArguments = ImmutableList.of("echo", "even drones can fly away");
- List<String> fullCommandLine =
- ProcessWrapperUtil.commandLineBuilder(getProcessWrapperPath(), commandArguments).build();
+ List<String> fullCommandLine = getProcessWrapper().commandLineBuilder(commandArguments).build();
Command command = new Command(fullCommandLine.toArray(new String[0]));
CommandResult commandResult = command.execute();
@@ -88,7 +90,8 @@
Path statisticsFilePath = outputDir.getRelative("stats.out");
List<String> fullCommandLine =
- ProcessWrapperUtil.commandLineBuilder(getProcessWrapperPath(), commandArguments)
+ getProcessWrapper()
+ .commandLineBuilder(commandArguments)
.setStatisticsPath(statisticsFilePath)
.build();
diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
index ff66eb2..32154dc 100644
--- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
@@ -150,6 +150,7 @@
resourceManager,
(env, binTools1, fallbackTmpDir) -> ImmutableMap.copyOf(env),
binTools,
+ /*processWrapper=*/ null,
Mockito.mock(RunfilesTreeUpdater.class)));
this.executor =
new TestExecutorBuilder(fileSystem, directories, binTools)
