Skip to content

Commit 7d9d23c

Browse files
coeuvrecopybara-github
coeuvre
authored and
copybara-github
committed
Correctly set default subprocess factory when loading class SubprocessBuilder.
Currently, there are two subprocess factories: `JavaSubprocessFactory` and `WindowsSubprocessFactory`. The default factory is set to `JavaSubprocessFactory` when loading class `SubprocessBuidler`. We call `setDefaultSubprocessFactory` to set the default factory, when creating BlazeRuntime, by checking the OS, which set the default factory to WindowsSubprocessFactory on Windows. However, for integration tests written in Java, `setDefaultSubprocessFactory` is not called. So we use `JavaSubprocessFactory` even on Windows for these tests which doesn't correctly terminate process tree. This CL fixes that by calling `setDefaultSubprocessFactory` when loading class `SubprocessBuilder`. PiperOrigin-RevId: 511810605 Change-Id: I55ca32caa0f81160e027780d671bf46a6bd6c266
1 parent 22ae3a6 commit 7d9d23c

File tree

8 files changed

+40
-31
lines changed

8 files changed

+40
-31
lines changed

‎src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.google.common.io.CharStreams;
2424
import com.google.devtools.build.lib.profiler.Profiler;
2525
import com.google.devtools.build.lib.profiler.SilentCloseable;
26+
import com.google.devtools.build.lib.shell.JavaSubprocessFactory;
2627
import com.google.devtools.build.lib.shell.Subprocess;
2728
import com.google.devtools.build.lib.shell.SubprocessBuilder;
2829
import com.google.devtools.build.lib.vfs.Path;
@@ -145,7 +146,9 @@ private Subprocess spawnSubprocess(CredentialHelperEnvironment environment, Stri
145146
Preconditions.checkNotNull(environment);
146147
Preconditions.checkNotNull(args);
147148

148-
return new SubprocessBuilder()
149+
// Force using JavaSubprocessFactory on Windows, because for some reasons,
150+
// WindowsSubprocessFactory cannot redirect stdin to subprocess.
151+
return new SubprocessBuilder(JavaSubprocessFactory.INSTANCE)
149152
.setArgv(ImmutableList.<String>builder().add(path.getPathString()).add(args).build())
150153
.setWorkingDirectory(environment.getWorkspacePath().getPathFile())
151154
.setEnv(environment.getClientEnvironment())

‎src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,6 @@
8484
import com.google.devtools.build.lib.server.RPCServer;
8585
import com.google.devtools.build.lib.server.ShutdownHooks;
8686
import com.google.devtools.build.lib.server.signal.InterruptSignalHandler;
87-
import com.google.devtools.build.lib.shell.JavaSubprocessFactory;
88-
import com.google.devtools.build.lib.shell.SubprocessBuilder;
89-
import com.google.devtools.build.lib.shell.SubprocessFactory;
9087
import com.google.devtools.build.lib.util.AbruptExitException;
9188
import com.google.devtools.build.lib.util.CustomExitCodePublisher;
9289
import com.google.devtools.build.lib.util.CustomFailureDetailPublisher;
@@ -95,7 +92,6 @@
9592
import com.google.devtools.build.lib.util.ExitCode;
9693
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
9794
import com.google.devtools.build.lib.util.LoggingUtil;
98-
import com.google.devtools.build.lib.util.OS;
9995
import com.google.devtools.build.lib.util.Pair;
10096
import com.google.devtools.build.lib.util.ProcessUtils;
10197
import com.google.devtools.build.lib.util.TestType;
@@ -105,7 +101,6 @@
105101
import com.google.devtools.build.lib.vfs.FileSystemUtils;
106102
import com.google.devtools.build.lib.vfs.Path;
107103
import com.google.devtools.build.lib.vfs.PathFragment;
108-
import com.google.devtools.build.lib.windows.WindowsSubprocessFactory;
109104
import com.google.devtools.build.lib.worker.WorkerMetricsCollector;
110105
import com.google.devtools.common.options.CommandNameCache;
111106
import com.google.devtools.common.options.InvocationPolicyParser;
@@ -1090,14 +1085,6 @@ public void run() {
10901085
}
10911086
}
10921087

1093-
private static SubprocessFactory subprocessFactoryImplementation() {
1094-
if (JniLoader.isJniAvailable() && OS.getCurrent() == OS.WINDOWS) {
1095-
return WindowsSubprocessFactory.INSTANCE;
1096-
} else {
1097-
return JavaSubprocessFactory.INSTANCE;
1098-
}
1099-
}
1100-
11011088
/**
11021089
* Parses the command line arguments into a {@link OptionsParser} object.
11031090
*
@@ -1257,8 +1244,6 @@ private static BlazeRuntime newRuntime(
12571244
return null;
12581245
});
12591246

1260-
SubprocessBuilder.setDefaultSubprocessFactory(subprocessFactoryImplementation());
1261-
12621247
Path outputUserRootPath = fs.getPath(outputUserRoot);
12631248
Path installBasePath = fs.getPath(installBase);
12641249
Path outputBasePath = fs.getPath(outputBase);

‎src/main/java/com/google/devtools/build/lib/shell/BUILD‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,12 @@ java_library(
1717
name = "shell",
1818
srcs = glob(["*.java"]),
1919
deps = [
20+
"//src/main/java/com/google/devtools/build/lib/jni",
2021
"//src/main/java/com/google/devtools/build/lib/util:describable_execution_unit",
22+
"//src/main/java/com/google/devtools/build/lib/util:os",
2123
"//src/main/java/com/google/devtools/build/lib/vfs",
24+
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
25+
"//src/main/java/com/google/devtools/build/lib/windows:processes",
2226
"//src/main/protobuf:execution_statistics_java_proto",
2327
"//third_party:auto_value",
2428
"//third_party:flogger",
@@ -36,7 +40,13 @@ bootstrap_java_library(
3640
exclude = ["ExecutionStatistics.java"],
3741
),
3842
jars = [
43+
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
44+
"//src/main/java/com/google/devtools/build/lib/jni",
3945
"//src/main/java/com/google/devtools/build/lib/util:describable_execution_unit",
46+
"//src/main/java/com/google/devtools/build/lib/util:filetype",
47+
"//src/main/java/com/google/devtools/build/lib/util:os",
48+
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
49+
"//src/main/java/com/google/devtools/build/lib/windows:processes",
4050
"//third_party:auto_value-jars",
4151
"//third_party:flogger-jars",
4252
"//third_party:bootstrap_guava_and_error_prone-jars",

‎src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,12 @@
1414

1515
package com.google.devtools.build.lib.shell;
1616

17+
import com.google.common.annotations.VisibleForTesting;
1718
import com.google.common.base.Preconditions;
1819
import com.google.common.collect.ImmutableList;
1920
import com.google.common.collect.ImmutableMap;
21+
import com.google.devtools.build.lib.jni.JniLoader;
22+
import com.google.devtools.build.lib.util.OS;
2023
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2124
import java.io.File;
2225
import java.io.IOException;
@@ -52,14 +55,24 @@ public enum StreamAction {
5255
private long timeoutMillis;
5356
private boolean redirectErrorStream;
5457

55-
static SubprocessFactory defaultFactory = JavaSubprocessFactory.INSTANCE;
58+
static SubprocessFactory defaultFactory = subprocessFactoryImplementation();
59+
60+
private static SubprocessFactory subprocessFactoryImplementation() {
61+
if (JniLoader.isJniAvailable() && OS.getCurrent() == OS.WINDOWS) {
62+
return WindowsSubprocessFactory.INSTANCE;
63+
} else {
64+
return JavaSubprocessFactory.INSTANCE;
65+
}
66+
}
5667

5768
/**
5869
* Sets the default factory class for creating subprocesses. Passing {@code null} resets it to the
5970
* initial state.
6071
*/
72+
@VisibleForTesting
6173
public static void setDefaultSubprocessFactory(SubprocessFactory factory) {
62-
SubprocessBuilder.defaultFactory = factory != null ? factory : JavaSubprocessFactory.INSTANCE;
74+
SubprocessBuilder.defaultFactory =
75+
factory != null ? factory : subprocessFactoryImplementation();
6376
}
6477

6578
public SubprocessBuilder() {

src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java renamed to src/main/java/com/google/devtools/build/lib/shell/WindowsSubprocess.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
package com.google.devtools.build.lib.windows;
15+
package com.google.devtools.build.lib.shell;
1616

1717
import com.google.common.base.Throwables;
18-
import com.google.devtools.build.lib.shell.Subprocess;
18+
import com.google.devtools.build.lib.windows.WindowsProcesses;
1919
import java.io.IOException;
2020
import java.io.InputStream;
2121
import java.io.OutputStream;

src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java renamed to src/main/java/com/google/devtools/build/lib/shell/WindowsSubprocessFactory.java

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,11 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
package com.google.devtools.build.lib.windows;
15+
package com.google.devtools.build.lib.shell;
1616

17-
import com.google.devtools.build.lib.shell.ShellUtils;
18-
import com.google.devtools.build.lib.shell.Subprocess;
19-
import com.google.devtools.build.lib.shell.SubprocessBuilder;
2017
import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction;
21-
import com.google.devtools.build.lib.shell.SubprocessFactory;
2218
import com.google.devtools.build.lib.vfs.PathFragment;
19+
import com.google.devtools.build.lib.windows.WindowsProcesses;
2320
import java.io.File;
2421
import java.io.IOException;
2522
import java.nio.charset.StandardCharsets;
@@ -44,6 +41,10 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
4441
: "";
4542
byte[] env = convertEnvToNative(builder.getEnv());
4643

44+
String cwd = null;
45+
if (builder.getWorkingDirectory() != null) {
46+
cwd = builder.getWorkingDirectory().getPath();
47+
}
4748
String stdoutPath = getRedirectPath(builder.getStdout(), builder.getStdoutFile());
4849
String stderrPath = getRedirectPath(builder.getStderr(), builder.getStderrFile());
4950

@@ -52,7 +53,7 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
5253
argv0,
5354
argvRest,
5455
env,
55-
builder.getWorkingDirectory().getPath(),
56+
cwd,
5657
stdoutPath,
5758
stderrPath,
5859
builder.redirectErrorStream());

‎src/main/java/com/google/devtools/build/lib/windows/BUILD‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,18 +30,13 @@ java_library(
3030
name = "windows",
3131
srcs = [
3232
"WindowsFileSystem.java",
33-
"WindowsSubprocess.java",
34-
"WindowsSubprocessFactory.java",
3533
],
3634
deps = [
3735
":file",
38-
":processes",
3936
"//src/main/java/com/google/devtools/build/lib/concurrent",
4037
"//src/main/java/com/google/devtools/build/lib/profiler",
41-
"//src/main/java/com/google/devtools/build/lib/shell",
4238
"//src/main/java/com/google/devtools/build/lib/vfs",
4339
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
4440
"//third_party:guava",
45-
"//third_party:jsr305",
4641
],
4742
)

‎src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import com.google.devtools.build.lib.shell.ShellUtils;
2424
import com.google.devtools.build.lib.shell.Subprocess;
2525
import com.google.devtools.build.lib.shell.SubprocessBuilder;
26+
import com.google.devtools.build.lib.shell.WindowsSubprocess;
27+
import com.google.devtools.build.lib.shell.WindowsSubprocessFactory;
2628
import com.google.devtools.build.lib.testutil.TestSpec;
2729
import com.google.devtools.build.lib.util.OS;
2830
import com.google.devtools.build.runfiles.Runfiles;

0 commit comments

Comments
 (0)