Skip to content

Commit cb2cd9f

Browse files
larsrc-googlecopybara-github
larsrc-google
authored and
copybara-github
committed
Adds flag --experimental_worker_strict_flagfiles that checks if the worker argument list conforms to the spec.
The spec calls for only one flag file, at the end of the argument list. However, the implementation allows for multiple flag files with startup arguments possible between or after. This has led to subtle bugs. We could also make the spec conform to the implementation, but there are fewer surprises possible with the spec, and it makes it much clearer that there is a distinction between startup args and per-request args. We would like to make it even more explicit, and possibly at the same time allow differing flags for workers and non-workers (e.g. for JIT/GC tweaking), but that would be a larger change. PiperOrigin-RevId: 458415814 Change-Id: I7eacfa7e6344348f66b6c83139c3bcd893bdd70b
1 parent 7892749 commit cb2cd9f

5 files changed

Lines changed: 192 additions & 25 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,4 +195,15 @@ public String getTypeDescription() {
195195
+ " per work request. Only workers that have the 'supports-multiplex-sandboxing' "
196196
+ "execution requirement will be sandboxed.")
197197
public boolean multiplexSandboxing;
198+
199+
@Option(
200+
name = "experimental_worker_strict_flagfiles",
201+
defaultValue = "false",
202+
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
203+
effectTags = {OptionEffectTag.EXECUTION},
204+
help =
205+
"If enabled, actions arguments for workers that do not follow the worker specification"
206+
+ " will cause an error. Worker arguments must have exactly one @flagfile argument"
207+
+ " as the last of its list of arguments.")
208+
public boolean strictFlagfiles;
198209
}

‎src/main/java/com/google/devtools/build/lib/worker/WorkerParser.java‎

Lines changed: 45 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@
1515
package com.google.devtools.build.lib.worker;
1616

1717
import com.google.common.annotations.VisibleForTesting;
18-
import com.google.common.base.MoreObjects;
1918
import com.google.common.collect.ImmutableList;
2019
import com.google.common.collect.ImmutableMap;
20+
import com.google.common.collect.Iterables;
2121
import com.google.common.hash.HashCode;
2222
import com.google.devtools.build.lib.actions.ExecException;
2323
import com.google.devtools.build.lib.actions.ExecutionRequirements.WorkerProtocolFormat;
@@ -42,13 +42,17 @@
4242
* and a separate list of flag files. The result is encapsulated as a {@link WorkerConfig}.
4343
*/
4444
class WorkerParser {
45-
public static final String ERROR_MESSAGE_PREFIX =
45+
private static final String ERROR_MESSAGE_PREFIX =
4646
"Worker strategy cannot execute this %s action, ";
47-
public static final String REASON_NO_FLAGFILE =
48-
"because the command-line arguments do not contain at least one @flagfile or --flagfile=";
47+
private static final String REASON_NO_FLAGFILE =
48+
"because the command-line arguments do not contain exactly one @flagfile or --flagfile=";
49+
private static final String REASON_EXCESS_FLAGFILE =
50+
"because the command-line arguments has a @flagfile or --flagfile= argument before the end";
51+
private static final String REASON_NO_FINAL_FLAGFILE =
52+
"because the command-line arguments does not end with a @flagfile or --flagfile= argument";
4953

5054
/** Pattern for @flagfile.txt and --flagfile=flagfile.txt */
51-
private static final Pattern FLAG_FILE_PATTERN = Pattern.compile("(?:@|--?flagfile=)(.+)");
55+
private static final Pattern FLAG_FILE_PATTERN = Pattern.compile("(?:@[^@]|--?flagfile=.)(.*)");
5256

5357
/** The global execRoot. */
5458
private final Path execRoot;
@@ -148,26 +152,38 @@ static WorkerKey createWorkerKey(
148152
* persistent worker ({@code workerArgs}) and the part that goes into the {@code WorkRequest}
149153
* protobuf ({@code flagFiles}).
150154
*/
151-
private ImmutableList<String> splitSpawnArgsIntoWorkerArgsAndFlagFiles(
155+
@VisibleForTesting
156+
ImmutableList<String> splitSpawnArgsIntoWorkerArgsAndFlagFiles(
152157
Spawn spawn, List<String> flagFiles) throws UserExecException {
153158
ImmutableList.Builder<String> workerArgs = ImmutableList.builder();
154-
for (String arg : spawn.getArguments()) {
155-
if (FLAG_FILE_PATTERN.matcher(arg).matches()) {
156-
flagFiles.add(arg);
157-
} else {
158-
workerArgs.add(arg);
159+
if (workerOptions.strictFlagfiles) {
160+
ImmutableList<String> args = spawn.getArguments();
161+
if (args.isEmpty()) {
162+
throwFlagFileFailure(REASON_NO_FLAGFILE, spawn);
163+
}
164+
if (!FLAG_FILE_PATTERN.matcher(Iterables.getLast(args)).matches()) {
165+
throwFlagFileFailure(REASON_NO_FINAL_FLAGFILE, spawn);
166+
}
167+
flagFiles.add(Iterables.getLast(args));
168+
for (int i = 0; i < args.size() - 1; i++) {
169+
if (FLAG_FILE_PATTERN.matcher(args.get(i)).matches()) {
170+
throwFlagFileFailure(REASON_EXCESS_FLAGFILE, spawn);
171+
} else {
172+
workerArgs.add(args.get(i));
173+
}
174+
}
175+
} else {
176+
for (String arg : spawn.getArguments()) {
177+
if (FLAG_FILE_PATTERN.matcher(arg).matches()) {
178+
flagFiles.add(arg);
179+
} else {
180+
workerArgs.add(arg);
181+
}
159182
}
160183
}
161184

162185
if (flagFiles.isEmpty()) {
163-
throw new UserExecException(
164-
FailureDetails.FailureDetail.newBuilder()
165-
.setMessage(
166-
String.format(ERROR_MESSAGE_PREFIX + REASON_NO_FLAGFILE, spawn.getMnemonic()))
167-
.setWorker(
168-
FailureDetails.Worker.newBuilder()
169-
.setCode(FailureDetails.Worker.Code.NO_FLAGFILE))
170-
.build());
186+
throwFlagFileFailure(REASON_NO_FLAGFILE, spawn);
171187
}
172188

173189
ImmutableList.Builder<String> mnemonicFlags = ImmutableList.builder();
@@ -176,10 +192,16 @@ private ImmutableList<String> splitSpawnArgsIntoWorkerArgsAndFlagFiles(
176192
.filter(entry -> entry.getKey().equals(spawn.getMnemonic()))
177193
.forEach(entry -> mnemonicFlags.add(entry.getValue()));
178194

179-
return workerArgs
180-
.add("--persistent_worker")
181-
.addAll(MoreObjects.firstNonNull(mnemonicFlags.build(), ImmutableList.of()))
182-
.build();
195+
return workerArgs.add("--persistent_worker").addAll(mnemonicFlags.build()).build();
196+
}
197+
198+
private void throwFlagFileFailure(String reason, Spawn spawn) throws UserExecException {
199+
throw new UserExecException(
200+
FailureDetails.FailureDetail.newBuilder()
201+
.setMessage(String.format(ERROR_MESSAGE_PREFIX + reason, spawn.getMnemonic()))
202+
.setWorker(
203+
FailureDetails.Worker.newBuilder().setCode(FailureDetails.Worker.Code.NO_FLAGFILE))
204+
.build());
183205
}
184206

185207
/** A pair of the {@link WorkerKey} and the list of flag files. */

‎src/test/java/com/google/devtools/build/lib/worker/BUILD‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ java_library(
4242
"//src/main/java/com/google/devtools/build/lib/actions",
4343
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
4444
"//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
45+
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
46+
"//src/main/java/com/google/devtools/build/lib/cmdline",
4547
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
4648
"//src/main/java/com/google/devtools/build/lib/sandbox:sandbox_helpers",
4749
"//src/main/java/com/google/devtools/build/lib/shell",
@@ -50,6 +52,7 @@ java_library(
5052
"//src/main/java/com/google/devtools/build/lib/worker",
5153
"//src/main/java/com/google/devtools/build/lib/worker:worker_key",
5254
"//src/main/java/com/google/devtools/build/lib/worker:worker_spawn_runner",
55+
"//src/main/java/net/starlark/java/syntax",
5356
"//src/test/java/com/google/devtools/build/lib/actions/util",
5457
"//third_party:guava",
5558
],

‎src/test/java/com/google/devtools/build/lib/worker/TestUtils.java‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,16 @@
1919
import com.google.common.collect.ImmutableSet;
2020
import com.google.common.collect.ImmutableSortedMap;
2121
import com.google.common.hash.HashCode;
22+
import com.google.devtools.build.lib.actions.ActionOwner;
23+
import com.google.devtools.build.lib.actions.BuildConfigurationEvent;
2224
import com.google.devtools.build.lib.actions.ExecutionRequirements;
2325
import com.google.devtools.build.lib.actions.ExecutionRequirements.WorkerProtocolFormat;
2426
import com.google.devtools.build.lib.actions.ResourceSet;
2527
import com.google.devtools.build.lib.actions.SimpleSpawn;
2628
import com.google.devtools.build.lib.actions.Spawn;
2729
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
30+
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
31+
import com.google.devtools.build.lib.cmdline.Label;
2832
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
2933
import com.google.devtools.build.lib.collect.nestedset.Order;
3034
import com.google.devtools.build.lib.shell.Subprocess;
@@ -36,6 +40,7 @@
3640
import java.io.IOException;
3741
import java.io.InputStream;
3842
import java.io.OutputStream;
43+
import net.starlark.java.syntax.Location;
3944

4045
/** Utilities that come in handy when unit-testing the worker code. */
4146
class TestUtils {
@@ -44,16 +49,36 @@ private TestUtils() {}
4449

4550
/** A helper method to create a fake Spawn with the given execution info. */
4651
static Spawn createSpawn(ImmutableMap<String, String> executionInfo) {
52+
return createSpawn(ImmutableList.of(), executionInfo);
53+
}
54+
55+
static Spawn createSpawn(
56+
ImmutableList<String> arguments, ImmutableMap<String, String> executionInfo) {
4757
return new SimpleSpawn(
4858
new ActionsTestUtil.NullAction(),
49-
/* arguments= */ ImmutableList.of(),
59+
arguments,
5060
/* environment= */ ImmutableMap.of(),
5161
executionInfo,
5262
/* inputs= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
5363
/* outputs= */ ImmutableSet.of(),
5464
ResourceSet.ZERO);
5565
}
5666

67+
static ActionOwner createActionOwner(String mnemonic) {
68+
return ActionOwner.create(
69+
Label.parseAbsoluteUnchecked("//null/action:owner"),
70+
ImmutableList.of(),
71+
new Location("dummy-file", 0, 0),
72+
mnemonic,
73+
"dummy-kind",
74+
"dummy-configuration",
75+
new BuildConfigurationEvent(
76+
BuildEventStreamProtos.BuildEventId.getDefaultInstance(),
77+
BuildEventStreamProtos.BuildEvent.getDefaultInstance()),
78+
null,
79+
ImmutableMap.of(),
80+
null);
81+
}
5782
/** A helper method to create a WorkerKey through WorkerParser. */
5883
static WorkerKey createWorkerKey(
5984
WorkerProtocolFormat protocolFormat,
@@ -98,7 +123,7 @@ static WorkerKey createWorkerKey(
98123
protocolFormat,
99124
fs,
100125
/* mnemonic= */ "dummy",
101-
/* proxied= */ true,
126+
/* multiplex= */ true,
102127
/* sandboxed= */ true,
103128
dynamic,
104129
/* args...= */ "arg1",

‎src/test/java/com/google/devtools/build/lib/worker/WorkerParserTest.java‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,18 @@
1616

1717
import static com.google.common.truth.Truth.assertThat;
1818
import static com.google.devtools.build.lib.actions.ExecutionRequirements.SUPPORTS_MULTIPLEX_SANDBOXING;
19+
import static org.junit.Assert.assertThrows;
1920

21+
import com.google.common.collect.ImmutableList;
22+
import com.google.common.collect.ImmutableMap;
23+
import com.google.common.collect.Maps;
24+
import com.google.devtools.build.lib.actions.Spawn;
25+
import com.google.devtools.build.lib.actions.UserExecException;
2026
import com.google.devtools.build.lib.vfs.DigestHashFunction;
2127
import com.google.devtools.build.lib.vfs.FileSystem;
2228
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
29+
import java.util.ArrayList;
30+
import java.util.List;
2331
import org.junit.Test;
2432
import org.junit.runner.RunWith;
2533
import org.junit.runners.JUnit4;
@@ -120,4 +128,102 @@ public void createWorkerKey_understandsMultiplexSandboxing() {
120128
assertThat(keyDynamicMultiplexSandboxing.isSandboxed()).isTrue();
121129
assertThat(keyDynamicMultiplexSandboxing.getWorkerTypeName()).isEqualTo("multiplex-worker");
122130
}
131+
132+
@Test
133+
public void splitSpawnArgsIntoWorkerArgsAndFlagFiles_splitsArgsBasicCase()
134+
throws UserExecException {
135+
WorkerOptions options = new WorkerOptions();
136+
options.workerExtraFlags = ImmutableList.of();
137+
WorkerParser parser = new WorkerParser(null, options, null, null);
138+
139+
Spawn spawn = TestUtils.createSpawn(ImmutableList.of("--foo", "@bar"), ImmutableMap.of());
140+
List<String> flagFiles = new ArrayList<>();
141+
ImmutableList<String> args = parser.splitSpawnArgsIntoWorkerArgsAndFlagFiles(spawn, flagFiles);
142+
assertThat(args).containsExactly("--foo", "--persistent_worker").inOrder();
143+
assertThat(flagFiles).containsExactly("@bar");
144+
}
145+
146+
@Test
147+
public void splitSpawnArgsIntoWorkerArgsAndFlagFiles_addsExtras() throws UserExecException {
148+
WorkerOptions options = new WorkerOptions();
149+
options.workerExtraFlags =
150+
ImmutableList.of(
151+
Maps.immutableEntry("Null", "--qux"),
152+
Maps.immutableEntry("Other action", "--should_not_appear"),
153+
Maps.immutableEntry("Null", "--quxify"));
154+
WorkerParser parser = new WorkerParser(null, options, null, null);
155+
Spawn spawn = TestUtils.createSpawn(ImmutableList.of("--foo", "@bar"), ImmutableMap.of());
156+
157+
List<String> flagFiles = new ArrayList<>();
158+
ImmutableList<String> args = parser.splitSpawnArgsIntoWorkerArgsAndFlagFiles(spawn, flagFiles);
159+
160+
assertThat(args).containsExactly("--foo", "--persistent_worker", "--qux", "--quxify").inOrder();
161+
assertThat(flagFiles).containsExactly("@bar");
162+
}
163+
164+
@Test
165+
public void splitSpawnArgsIntoWorkerArgsAndFlagFiles_addsFlagFiles() throws UserExecException {
166+
WorkerOptions options = new WorkerOptions();
167+
options.workerExtraFlags = ImmutableList.of();
168+
options.strictFlagfiles = false;
169+
WorkerParser parser = new WorkerParser(null, options, null, null);
170+
Spawn spawn =
171+
TestUtils.createSpawn(
172+
ImmutableList.of("--foo", "--flagfile=bar", "@@escaped", "@bar", "@bartoo", "--final"),
173+
ImmutableMap.of());
174+
175+
List<String> flagFiles = new ArrayList<>();
176+
ImmutableList<String> args = parser.splitSpawnArgsIntoWorkerArgsAndFlagFiles(spawn, flagFiles);
177+
178+
assertThat(args)
179+
.containsExactly("--foo", "@@escaped", "--final", "--persistent_worker")
180+
.inOrder();
181+
assertThat(flagFiles).containsExactly("--flagfile=bar", "@bar", "@bartoo").inOrder();
182+
}
183+
184+
@Test
185+
public void splitSpawnArgsIntoWorkerArgsAndFlagFiles_addsFlagFilesStrict()
186+
throws UserExecException {
187+
WorkerOptions options = new WorkerOptions();
188+
options.workerExtraFlags = ImmutableList.of();
189+
options.strictFlagfiles = true;
190+
WorkerParser parser = new WorkerParser(null, options, null, null);
191+
Spawn spawn =
192+
TestUtils.createSpawn(
193+
ImmutableList.of("--foo", "@@escaped", "--final", "@bar"), ImmutableMap.of());
194+
195+
List<String> flagFiles = new ArrayList<>();
196+
ImmutableList<String> args = parser.splitSpawnArgsIntoWorkerArgsAndFlagFiles(spawn, flagFiles);
197+
198+
assertThat(args)
199+
.containsExactly("--foo", "@@escaped", "--final", "--persistent_worker")
200+
.inOrder();
201+
assertThat(flagFiles).containsExactly("@bar");
202+
}
203+
204+
@Test
205+
public void splitSpawnArgsIntoWorkerArgsAndFlagFiles_strictFlagFiles() throws UserExecException {
206+
assertIllegalFlags("Must have args");
207+
assertIllegalFlags("Must have a flagfile", "--foo", "--final");
208+
assertIllegalFlags("Flagfile must be at the end", "@earlyFile", "--final");
209+
assertIllegalFlags("Only one flagfile allowed", "@earlyFile", "--final", "@lateFile");
210+
assertIllegalFlags(
211+
"Only one flagfile allowed, regardless of syntax",
212+
"--flagfile=foo",
213+
"--final",
214+
"@lateFile");
215+
}
216+
217+
private void assertIllegalFlags(String message, String... args) {
218+
WorkerOptions options = new WorkerOptions();
219+
options.workerExtraFlags = ImmutableList.of();
220+
options.strictFlagfiles = true;
221+
WorkerParser parser = new WorkerParser(null, options, null, null);
222+
Spawn spawn = TestUtils.createSpawn(ImmutableList.copyOf(args), ImmutableMap.of());
223+
224+
assertThrows(
225+
message,
226+
UserExecException.class,
227+
() -> parser.splitSpawnArgsIntoWorkerArgsAndFlagFiles(spawn, new ArrayList<>()));
228+
}
123229
}

0 commit comments

Comments
 (0)