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 splitSpawnArgsIntoWorkerArgsAndFlagFiles( Spawn spawn, List flagFiles) throws UserExecException { ImmutableList.Builder workerArgs = ImmutableList.builder(); + ImmutableList args = spawn.getArguments(); + if (args.isEmpty()) { + throwFlagFileFailure(REASON_NO_FLAGFILE, spawn); + } if (workerOptions.strictFlagfiles) { - ImmutableList 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 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 mnemonicFlags = ImmutableList.builder(); @@ -200,9 +211,12 @@ ImmutableList 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 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 flagFiles = new ArrayList<>(); ImmutableList 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