Reinstate legacy worker flag file behaviour when not using --experime…

…ntal_worker_strict_flagfiles. PiperOrigin-RevId: 461538135 Change-Id: Id25c99bb09bb7954f2f047d784791f645f6e9b82
bazelbuild · Jul 18, 2022 · be0da98 · be0da98
1 parent 86b800e
commit be0da98
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 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
@@ -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
@@ -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
@@ -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