From be0da98839785ea83bb53a1fee7b2b2e0a11ec81 Mon Sep 17 00:00:00 2001
From: Googler <larsrc@google.com>
Date: Sun, 17 Jul 2022 23:04:01 -0700
Subject: [PATCH] Reinstate legacy worker flag file behaviour when not using
--experimental_worker_strict_flagfiles.
PiperOrigin-RevId: 461538135
Change-Id: Id25c99bb09bb7954f2f047d784791f645f6e9b82
---
.../build/lib/worker/WorkerParser.java | 39 +++++++++++++------
.../build/lib/worker/WorkerSpawnRunner.java | 1 -
.../build/lib/worker/WorkerParserTest.java | 7 ++--
3 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java
index d729d48fd1ffb8..ff09a7b5dfe64f 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java
@@ -51,9 +51,17 @@ class WorkerParser {
private static final String REASON_NO_FINAL_FLAGFILE =
"because the command-line arguments does not end with a @flagfile or --flagfile= argument";
- /** Pattern for @flagfile.txt and --flagfile=flagfile.txt. This doesn't handle @@-escapes. */
+ /**
+ * Pattern for @flagfile.txt and --flagfile=flagfile.txt. This doesn't handle @@-escapes, those
+ * are checked for separately.
+ */
private static final Pattern FLAG_FILE_PATTERN = Pattern.compile("(?:@|--?flagfile=)(.+)");
+ /**
+ * Legacy pattern for @flagfile.txt and --flagfile=flagfile.txt. This doesn't handle @@-escapes.
+ */
+ private static final Pattern LEGACY_FLAG_FILE_PATTERN = Pattern.compile("(?:@|--?flagfile=)(.+)");
+
/** The global execRoot. */
private final Path execRoot;
@@ -151,6 +159,10 @@ private static boolean isFlagFileArg(String arg) {
return FLAG_FILE_PATTERN.matcher(arg).matches() && !arg.startsWith("@@");
}
+ private static boolean isLegacyFlagFileArg(String arg) {
+ return LEGACY_FLAG_FILE_PATTERN.matcher(arg).matches();
+ }
+
/**
* Splits the command-line arguments of the {@code Spawn} into the part that is used to start the
* persistent worker ({@code workerArgs}) and the part that goes into the {@code WorkRequest}
@@ -160,11 +172,11 @@ private static boolean isFlagFileArg(String arg) {
ImmutableList<String> splitSpawnArgsIntoWorkerArgsAndFlagFiles(
Spawn spawn, List<String> flagFiles) throws UserExecException {
ImmutableList.Builder<String> workerArgs = ImmutableList.builder();
+ ImmutableList<String> args = spawn.getArguments();
+ if (args.isEmpty()) {
+ throwFlagFileFailure(REASON_NO_FLAGFILE, spawn);
+ }
if (workerOptions.strictFlagfiles) {
- ImmutableList<String> args = spawn.getArguments();
- if (args.isEmpty()) {
- throwFlagFileFailure(REASON_NO_FLAGFILE, spawn);
- }
if (!isFlagFileArg(Iterables.getLast(args))) {
throwFlagFileFailure(REASON_NO_FINAL_FLAGFILE, spawn);
}
@@ -177,17 +189,16 @@ ImmutableList<String> splitSpawnArgsIntoWorkerArgsAndFlagFiles(
}
}
} else {
- for (String arg : spawn.getArguments()) {
- if (isFlagFileArg(arg)) {
+ for (String arg : args) {
+ if (isLegacyFlagFileArg(arg)) {
flagFiles.add(arg);
} else {
workerArgs.add(arg);
}
}
- }
-
- if (flagFiles.isEmpty()) {
- throwFlagFileFailure(REASON_NO_FLAGFILE, spawn);
+ if (flagFiles.isEmpty()) {
+ throwFlagFileFailure(REASON_NO_FLAGFILE, spawn);
+ }
}
ImmutableList.Builder<String> mnemonicFlags = ImmutableList.builder();
@@ -200,9 +211,12 @@ ImmutableList<String> splitSpawnArgsIntoWorkerArgsAndFlagFiles(
}
private void throwFlagFileFailure(String reason, Spawn spawn) throws UserExecException {
+ String message =
+ String.format(
+ ERROR_MESSAGE_PREFIX + reason + "%n%s", spawn.getMnemonic(), spawn.getArguments());
throw new UserExecException(
FailureDetails.FailureDetail.newBuilder()
- .setMessage(String.format(ERROR_MESSAGE_PREFIX + reason, spawn.getMnemonic()))
+ .setMessage(message)
.setWorker(
FailureDetails.Worker.newBuilder().setCode(FailureDetails.Worker.Code.NO_FLAGFILE))
.build());
@@ -227,3 +241,4 @@ public List<String> getFlagFiles() {
}
}
}
+
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
index ff9cb0f4b6b489..65b79b10e89f1e 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
@@ -44,7 +44,6 @@
import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
import com.google.devtools.build.lib.exec.SpawnExecutingEvent;
import com.google.devtools.build.lib.exec.SpawnRunner;
-import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.exec.SpawnSchedulingEvent;
import com.google.devtools.build.lib.exec.local.LocalEnvProvider;
import com.google.devtools.build.lib.profiler.Profiler;
diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerParserTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerParserTest.java
index afdce4dd9fccdb..df8c2459f49d21 100644
--- a/src/test/java/com/google/devtools/build/lib/worker/WorkerParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerParserTest.java
@@ -175,10 +175,11 @@ public void splitSpawnArgsIntoWorkerArgsAndFlagFiles_addsFlagFiles() throws User
List<String> flagFiles = new ArrayList<>();
ImmutableList<String> args = parser.splitSpawnArgsIntoWorkerArgsAndFlagFiles(spawn, flagFiles);
- assertThat(args)
- .containsExactly("--foo", "@@escaped", "--final", "--persistent_worker")
+ assertThat(args).containsExactly("--foo", "--final", "--persistent_worker").inOrder();
+ // Yes, the legacy implementation allows multiple flagfiles and ignores escape sequences.
+ assertThat(flagFiles)
+ .containsExactly("--flagfile=bar", "@@escaped", "@bar", "@bartoo")
.inOrder();
- assertThat(flagFiles).containsExactly("--flagfile=bar", "@bar", "@bartoo").inOrder();
}
@Test