Change how we roll out the process-wrapper's wait fix (again).
https://github.com/bazelbuild/bazel/commit/59183b6abb2994ff021dbeafc7f3c136f0218caa added a very generic --process_wrapper_extra_flags flag to
make this possible, allowing us to pass a flag to the process-wrapper to
enable the bug fix without having to add custom code in Bazel. While a
nice idea, this had the nasty side-effect of making the process-wrapper's
command line interface public and part of our API.
Undo that and instead add a --experimental_process_wrapper_wait_fix flag
specifically for this rollout, marked experimental so that we can remove
it once we are done. Note that this still benefits from the internal
changes done to support "extra flags", as we reuse that machinery to pass
this around.
Part of https://github.com/bazelbuild/bazel/issues/10245.
RELNOTES[INC]: This removes the short-lived --process_wrapper_extra_flags
flag, which was introduced primarily to roll out a bug fix. Unfortunately,
this made us inadvertently expose all of the process-wrapper's command line
interface to the public, which should not have happened. Given the corner
case of the utility of this flag, the lack of documentation for it, and the
fact that it only appeared in a single release, we are treating this as a
bug instead of a backwards compatibility breakage.
PiperOrigin-RevId: 314939927
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
index 1561630..660b0bd 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalExecutionOptions.java
@@ -14,14 +14,12 @@
package com.google.devtools.build.lib.exec.local;
import com.google.devtools.common.options.Converters;
-import com.google.devtools.common.options.Converters.StringConverter;
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.OptionsBase;
import com.google.devtools.common.options.RegexPatternOption;
import java.time.Duration;
-import java.util.List;
/**
* Local execution options.
@@ -29,18 +27,6 @@
public class LocalExecutionOptions extends OptionsBase {
@Option(
- name = "process_wrapper_extra_flags",
- defaultValue = "null",
- documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
- effectTags = {OptionEffectTag.EXECUTION},
- converter = StringConverter.class,
- allowMultiple = true,
- help =
- "Extra flags to pass to the process-wrapper. These are appended to the invocation "
- + "constructed by Bazel, so this can be used to override any computed defaults.")
- public List<String> processWrapperExtraFlags;
-
- @Option(
name = "local_termination_grace_seconds",
oldName = "local_sigkill_grace_seconds",
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
@@ -85,6 +71,14 @@
+ "for this explicit cancellation.")
public boolean localLockfreeOutput;
+ @Option(
+ name = "experimental_process_wrapper_wait_fix",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.EXECUTION},
+ help = "Helper to roll out the process-wrapper's --wait_fix bug fix in a controlled manner.")
+ public boolean processWrapperWaitFix;
+
public Duration getLocalSigkillGraceSeconds() {
// TODO(ulfjack): Change localSigkillGraceSeconds type to Duration.
return Duration.ofSeconds(localSigkillGraceSeconds);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java b/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java
index bd35a27..73dba33 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/ProcessWrapper.java
@@ -58,12 +58,17 @@
public static ProcessWrapper fromCommandEnvironment(CommandEnvironment cmdEnv) {
LocalExecutionOptions options = cmdEnv.getOptions().getOptions(LocalExecutionOptions.class);
Duration killDelay = options == null ? null : options.getLocalSigkillGraceSeconds();
- List<String> extraFlags =
- options == null ? ImmutableList.of() : options.processWrapperExtraFlags;
+
+ ImmutableList.Builder<String> extraFlags = ImmutableList.builder();
+ if (options != null) {
+ if (options.processWrapperWaitFix) {
+ extraFlags.add("--wait_fix");
+ }
+ }
Path path = cmdEnv.getBlazeWorkspace().getBinTools().getEmbeddedPath(BIN_BASENAME);
if (OS.isPosixCompatible() && path != null && path.exists()) {
- return new ProcessWrapper(path, killDelay, extraFlags);
+ return new ProcessWrapper(path, killDelay, extraFlags.build());
} else {
return null;
}
diff --git a/src/main/tools/process-wrapper-options.cc b/src/main/tools/process-wrapper-options.cc
index 516668e..837fd26 100644
--- a/src/main/tools/process-wrapper-options.cc
+++ b/src/main/tools/process-wrapper-options.cc
@@ -64,7 +64,7 @@
{"stderr", required_argument, 0, 'e'},
{"stats", required_argument, 0, 's'},
{"debug", no_argument, 0, 'd'},
- {"wait_fix", optional_argument, 0, 'W'},
+ {"wait_fix", no_argument, 0, 'W'},
{0, 0, 0, 0}};
extern char *optarg;
extern int optind, optopt;
@@ -111,22 +111,7 @@
opt.debug = true;
break;
case 'W':
- // Allows for a controlled rollout of the "wait for process group" fix.
- // The optional argument can be used to "undo" the application of -W in
- // case things go wrong by passing the following flag to the build:
- // --process_wrapper_extra_flags=--wait_fix=no
- if (optarg != nullptr) {
- if (std::strcmp(optarg, "yes") == 0) {
- opt.wait_fix = true;
- } else if (std::strcmp(optarg, "no") == 0) {
- opt.wait_fix = false;
- } else {
- Usage(args.front(), "Argument to -W, if present, must be yes or no",
- optopt, optind);
- }
- } else {
- opt.wait_fix = true;
- }
+ opt.wait_fix = true;
break;
case '?':
Usage(args.front(), "Unrecognized argument: -%c (%d)", optopt, optind);
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 99f7731..ca2bd3b 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
@@ -314,9 +314,7 @@
private static ProcessWrapper makeProcessWrapper(FileSystem fs, LocalExecutionOptions options) {
return new ProcessWrapper(
- fs.getPath("/process-wrapper"),
- options.getLocalSigkillGraceSeconds(),
- options.processWrapperExtraFlags);
+ fs.getPath("/process-wrapper"), options.getLocalSigkillGraceSeconds(), ImmutableList.of());
}
/**
