Skip to content

Commit b32349f

Browse files
larsrc-googlecopybara-github
larsrc-google
authored and
copybara-github
committed
Set a fallback dynamic local strategy even when the dynamic_local_strategy flag is passed.
RELNOTES: n/a PiperOrigin-RevId: 344274807
1 parent 3c9ad2d commit b32349f

File tree

5 files changed

+178
-53
lines changed

5 files changed

+178
-53
lines changed

‎src/main/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModule.java‎

Lines changed: 42 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@
1515

1616
import com.google.common.annotations.VisibleForTesting;
1717
import com.google.common.collect.ImmutableList;
18+
import com.google.common.collect.ImmutableMap;
1819
import com.google.common.collect.ImmutableSet;
1920
import com.google.common.collect.Lists;
20-
import com.google.common.collect.Maps;
2121
import com.google.common.collect.Sets;
2222
import com.google.common.util.concurrent.ThreadFactoryBuilder;
2323
import com.google.devtools.build.lib.actions.Spawn;
@@ -35,6 +35,7 @@
3535
import com.google.devtools.build.lib.util.AbruptExitException;
3636
import com.google.devtools.build.lib.util.DetailedExitCode;
3737
import com.google.devtools.common.options.OptionsBase;
38+
import java.util.HashMap;
3839
import java.util.List;
3940
import java.util.Map;
4041
import java.util.concurrent.ExecutorService;
@@ -69,40 +70,53 @@ public void beforeCommand(CommandEnvironment env) {
6970
env.getEventBus().register(this);
7071
}
7172

72-
private List<Map.Entry<String, List<String>>> getLocalStrategies(
73-
DynamicExecutionOptions options) {
73+
@VisibleForTesting
74+
ImmutableMap<String, List<String>> getLocalStrategies(DynamicExecutionOptions options)
75+
throws AbruptExitException {
7476
// Options that set "allowMultiple" to true ignore the default value, so we replicate that
7577
// functionality here. Additionally, since we are still supporting --dynamic_worker_strategy,
7678
// but will deprecate it soon, we add its functionality to --dynamic_local_strategy. This allows
7779
// users to set --dynamic_local_strategy and not --dynamic_worker_strategy to stop defaulting to
78-
// worker strategy.
80+
// worker strategy. For simplicity, we add the default strategy first, it may be overridden
81+
// later.
82+
// ImmutableMap.Builder fails on duplicates, so we use a regular map first to remove dups.
83+
Map<String, List<String>> localAndWorkerStrategies = new HashMap<>();
7984
// TODO(steinman): Deprecate --dynamic_worker_strategy and clean this up.
80-
if (options.dynamicLocalStrategy == null || options.dynamicLocalStrategy.isEmpty()) {
81-
String workerStrategy =
82-
options.dynamicWorkerStrategy.isEmpty() ? "worker" : options.dynamicWorkerStrategy;
83-
return ImmutableList.of(
84-
Maps.immutableEntry("", ImmutableList.of(workerStrategy, "sandboxed")));
85-
}
86-
87-
ImmutableList.Builder<Map.Entry<String, List<String>>> localAndWorkerStrategies =
88-
ImmutableList.builder();
89-
for (Map.Entry<String, List<String>> entry : options.dynamicLocalStrategy) {
90-
if ("".equals(entry.getKey())) {
91-
List<String> newValue = Lists.newArrayList(options.dynamicWorkerStrategy);
92-
newValue.addAll(entry.getValue());
93-
localAndWorkerStrategies.add(Maps.immutableEntry("", newValue));
94-
} else {
95-
localAndWorkerStrategies.add(entry);
85+
List<String> defaultValue = Lists.newArrayList();
86+
String workerStrategy =
87+
options.dynamicWorkerStrategy.isEmpty() ? "worker" : options.dynamicWorkerStrategy;
88+
defaultValue.addAll(ImmutableList.of(workerStrategy, "sandboxed"));
89+
throwIfContainsDynamic(defaultValue, "--dynamic_local_strategy");
90+
localAndWorkerStrategies.put("", defaultValue);
91+
92+
if (!options.dynamicLocalStrategy.isEmpty()) {
93+
for (Map.Entry<String, List<String>> entry : options.dynamicLocalStrategy) {
94+
if ("".equals(entry.getKey())) {
95+
List<String> newValue = Lists.newArrayList();
96+
if (!options.dynamicWorkerStrategy.isEmpty()) {
97+
newValue.add(options.dynamicWorkerStrategy);
98+
}
99+
newValue.addAll(entry.getValue());
100+
localAndWorkerStrategies.put("", newValue);
101+
} else {
102+
localAndWorkerStrategies.put(entry.getKey(), entry.getValue());
103+
}
104+
throwIfContainsDynamic(entry.getValue(), "--dynamic_local_strategy");
96105
}
97106
}
98-
return localAndWorkerStrategies.build();
107+
return ImmutableMap.copyOf(localAndWorkerStrategies);
99108
}
100109

101-
private List<Map.Entry<String, List<String>>> getRemoteStrategies(
102-
DynamicExecutionOptions options) {
103-
return (options.dynamicRemoteStrategy == null || options.dynamicRemoteStrategy.isEmpty())
104-
? ImmutableList.of(Maps.immutableEntry("", ImmutableList.of("remote")))
105-
: options.dynamicRemoteStrategy;
110+
private ImmutableMap<String, List<String>> getRemoteStrategies(DynamicExecutionOptions options)
111+
throws AbruptExitException {
112+
Map<String, List<String>> strategies = new HashMap<>(); // Needed to dedup
113+
for (Map.Entry<String, List<String>> e : options.dynamicRemoteStrategy) {
114+
throwIfContainsDynamic(e.getValue(), "--dynamic_remote_strategy");
115+
strategies.put(e.getKey(), e.getValue());
116+
}
117+
return options.dynamicRemoteStrategy.isEmpty()
118+
? ImmutableMap.of("", ImmutableList.of("remote"))
119+
: ImmutableMap.copyOf(strategies);
106120
}
107121

108122
@Override
@@ -130,16 +144,8 @@ final void registerSpawnStrategies(
130144
}
131145
registryBuilder.registerStrategy(strategy, "dynamic", "dynamic_worker");
132146

133-
for (Map.Entry<String, List<String>> mnemonicToStrategies : getLocalStrategies(options)) {
134-
throwIfContainsDynamic(mnemonicToStrategies.getValue(), "--dynamic_local_strategy");
135-
registryBuilder.addDynamicLocalStrategiesByMnemonic(
136-
mnemonicToStrategies.getKey(), mnemonicToStrategies.getValue());
137-
}
138-
for (Map.Entry<String, List<String>> mnemonicToStrategies : getRemoteStrategies(options)) {
139-
throwIfContainsDynamic(mnemonicToStrategies.getValue(), "--dynamic_remote_strategy");
140-
registryBuilder.addDynamicRemoteStrategiesByMnemonic(
141-
mnemonicToStrategies.getKey(), mnemonicToStrategies.getValue());
142-
}
147+
registryBuilder.addDynamicLocalStrategies(getLocalStrategies(options));
148+
registryBuilder.addDynamicRemoteStrategies(getRemoteStrategies(options));
143149
}
144150

145151
private void throwIfContainsDynamic(List<String> strategies, String flagName)

‎src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -334,28 +334,28 @@ public Builder resetDefaultStrategies() {
334334
}
335335

336336
/**
337-
* Sets the strategy names to use in the remote branch of dynamic execution for a given action
338-
* mnemonic.
337+
* Sets the strategy names to use in the remote branch of dynamic execution for a set of action
338+
* mnemonics.
339339
*
340340
* <p>During execution, each strategy is {@linkplain SpawnStrategy#canExec(Spawn,
341341
* ActionContextRegistry) asked} whether it can execute a given Spawn. The first strategy in the
342342
* list that says so will get the job.
343343
*/
344-
public Builder addDynamicRemoteStrategiesByMnemonic(String mnemonic, List<String> strategies) {
345-
mnemonicToRemoteIdentifiers.put(mnemonic, strategies);
344+
public Builder addDynamicRemoteStrategies(Map<String, List<String>> strategies) {
345+
mnemonicToRemoteIdentifiers.putAll(strategies);
346346
return this;
347347
}
348348

349349
/**
350-
* Sets the strategy names to use in the local branch of dynamic execution for a given action
351-
* mnemonic.
350+
* Sets the strategy names to use in the local branch of dynamic execution for a number of
351+
* action mnemonics.
352352
*
353353
* <p>During execution, each strategy is {@linkplain SpawnStrategy#canExec(Spawn,
354354
* ActionContextRegistry) asked} whether it can execute a given Spawn. The first strategy in the
355355
* list that says so will get the job.
356356
*/
357-
public Builder addDynamicLocalStrategiesByMnemonic(String mnemonic, List<String> strategies) {
358-
mnemonicToLocalIdentifiers.put(mnemonic, strategies);
357+
public Builder addDynamicLocalStrategies(Map<String, List<String>> strategies) {
358+
mnemonicToLocalIdentifiers.putAll(strategies);
359359
return this;
360360
}
361361

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,20 @@ java_test(
4343
],
4444
)
4545

46+
java_test(
47+
name = "DynamicExecutionModuleTest",
48+
size = "small",
49+
srcs = ["DynamicExecutionModuleTest.java"],
50+
deps = [
51+
"//src/main/java/com/google/devtools/build/lib/dynamic",
52+
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
53+
"//src/main/java/com/google/devtools/common/options",
54+
"//third_party:guava",
55+
"//third_party:junit4",
56+
"//third_party:truth",
57+
],
58+
)
59+
4660
test_suite(
4761
name = "windows_tests",
4862
tags = [

‎src/test/java/com/google/devtools/build/lib/dynamic/DynamicExecutionModuleTest.java‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
// Copyright 2020 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.dynamic;
15+
16+
import static com.google.common.truth.Truth.assertThat;
17+
18+
import com.google.common.collect.ImmutableMap;
19+
import com.google.common.collect.Lists;
20+
import com.google.devtools.build.lib.util.AbruptExitException;
21+
import com.google.devtools.common.options.Converters;
22+
import com.google.devtools.common.options.OptionsParsingException;
23+
import java.util.Collections;
24+
import java.util.LinkedHashMap;
25+
import java.util.List;
26+
import java.util.Map;
27+
import org.junit.Before;
28+
import org.junit.Test;
29+
import org.junit.runner.RunWith;
30+
import org.junit.runners.JUnit4;
31+
32+
/** Tests for {@link com.google.devtools.build.lib.dynamic.DynamicExecutionModule}. */
33+
@RunWith(JUnit4.class)
34+
public class DynamicExecutionModuleTest {
35+
private DynamicExecutionModule module;
36+
private DynamicExecutionOptions options;
37+
38+
@Before
39+
public void setUp() {
40+
module = new DynamicExecutionModule();
41+
options = new DynamicExecutionOptions();
42+
options.dynamicWorkerStrategy = ""; // default
43+
options.dynamicLocalStrategy = Collections.emptyList(); // default
44+
options.dynamicRemoteStrategy = Collections.emptyList(); // default
45+
}
46+
47+
@Test
48+
public void testGetLocalStrategies_getsDefaultWithNoOptions()
49+
throws AbruptExitException, OptionsParsingException {
50+
assertThat(module.getLocalStrategies(options)).isEqualTo(parseStrategies("worker,sandboxed"));
51+
}
52+
53+
@Test
54+
public void testGetLocalStrategies_dynamicWorkerStrategyTakesSingleValue()
55+
throws AbruptExitException, OptionsParsingException {
56+
options.dynamicWorkerStrategy = "local,worker";
57+
// This looks weird, but it's expected behaviour that dynamic_worker_strategy
58+
// doesn't get parsed.
59+
Map<String, List<String>> expected = parseStrategies("sandboxed");
60+
expected.get("").add(0, "local,worker");
61+
assertThat(module.getLocalStrategies(options))
62+
.isEqualTo(ImmutableMap.copyOf(expected.entrySet()));
63+
}
64+
65+
@Test
66+
public void testGetLocalStrategies_genericOptionOverridesFallbacks()
67+
throws AbruptExitException, OptionsParsingException {
68+
options.dynamicLocalStrategy = parseStrategiesToOptions("local,worker");
69+
assertThat(module.getLocalStrategies(options)).isEqualTo(parseStrategies("local,worker"));
70+
}
71+
72+
@Test
73+
public void testGetLocalStrategies_specificOptionKeepsFallbacks()
74+
throws AbruptExitException, OptionsParsingException {
75+
options.dynamicLocalStrategy = parseStrategiesToOptions("Foo=local,worker");
76+
assertThat(module.getLocalStrategies(options))
77+
.isEqualTo(parseStrategies("Foo=local,worker", "worker,sandboxed"));
78+
}
79+
80+
@Test
81+
public void testGetLocalStrategies_canMixSpecificsAndGenericOptions()
82+
throws AbruptExitException, OptionsParsingException {
83+
options.dynamicLocalStrategy = parseStrategiesToOptions("Foo=local,worker", "worker");
84+
assertThat(module.getLocalStrategies(options))
85+
.isEqualTo(parseStrategies("Foo=local,worker", "worker"));
86+
}
87+
88+
private static List<Map.Entry<String, List<String>>> parseStrategiesToOptions(
89+
String... strategies) throws OptionsParsingException {
90+
Map<String, List<String>> result = parseStrategies(strategies);
91+
return Lists.newArrayList(result.entrySet());
92+
}
93+
94+
private static Map<String, List<String>> parseStrategies(String... strategies)
95+
throws OptionsParsingException {
96+
Map<String, List<String>> result = new LinkedHashMap<>();
97+
Converters.StringToStringListConverter converter = new Converters.StringToStringListConverter();
98+
for (String s : strategies) {
99+
Map.Entry<String, List<String>> converted = converter.convert(s);
100+
// Have to avoid using Immutable* to allow overwriting elements.
101+
result.put(converted.getKey(), Lists.newArrayList(converted.getValue()));
102+
}
103+
return result;
104+
}
105+
}

‎src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -314,8 +314,8 @@ public void testDynamicStrategies() throws Exception {
314314
SpawnStrategyRegistry.builder()
315315
.registerStrategy(strategy1, "foo")
316316
.registerStrategy(strategy2, "bar")
317-
.addDynamicLocalStrategiesByMnemonic("mnem", ImmutableList.of("bar"))
318-
.addDynamicRemoteStrategiesByMnemonic("mnem", ImmutableList.of("foo"))
317+
.addDynamicLocalStrategies(ImmutableMap.of("mnem", ImmutableList.of("bar")))
318+
.addDynamicRemoteStrategies(ImmutableMap.of("mnem", ImmutableList.of("foo")))
319319
.build();
320320

321321
assertThat(
@@ -337,7 +337,7 @@ public void testDynamicStrategyNotPresent() {
337337
() ->
338338
SpawnStrategyRegistry.builder()
339339
.registerStrategy(strategy1, "foo")
340-
.addDynamicLocalStrategiesByMnemonic("mnem", ImmutableList.of("bar"))
340+
.addDynamicLocalStrategies(ImmutableMap.of("mnem", ImmutableList.of("bar")))
341341
.build());
342342

343343
assertThat(exception).hasMessageThat().containsMatch("bar.*Valid.*foo");
@@ -352,7 +352,7 @@ public void testDynamicStrategyNotSandboxed() {
352352
() ->
353353
SpawnStrategyRegistry.builder()
354354
.registerStrategy(strategy1, "foo")
355-
.addDynamicLocalStrategiesByMnemonic("mnem", ImmutableList.of("foo"))
355+
.addDynamicLocalStrategies(ImmutableMap.of("mnem", ImmutableList.of("foo")))
356356
.build());
357357

358358
assertThat(exception).hasMessageThat().containsMatch("sandboxed strategy");
@@ -423,8 +423,8 @@ public void testNotifyUsed() throws Exception {
423423
.setDefaultStrategies(ImmutableList.of("9"))
424424
.setDefaultStrategies(ImmutableList.of("3"))
425425
.setRemoteLocalFallbackStrategyIdentifier("4")
426-
.addDynamicLocalStrategiesByMnemonic("oy", ImmutableList.of("5"))
427-
.addDynamicRemoteStrategiesByMnemonic("oy", ImmutableList.of("6"))
426+
.addDynamicLocalStrategies(ImmutableMap.of("oy", ImmutableList.of("5")))
427+
.addDynamicRemoteStrategies(ImmutableMap.of("oy", ImmutableList.of("6")))
428428
.build();
429429

430430
strategyRegistry.notifyUsed(null);
@@ -463,9 +463,9 @@ public void testNotifyUsedDynamic() throws Exception {
463463
.addDescriptionFilter(ELLO_MATCHER, ImmutableList.of("2"))
464464
.setDefaultStrategies(ImmutableList.of("3"))
465465
.setRemoteLocalFallbackStrategyIdentifier("4")
466-
.addDynamicLocalStrategiesByMnemonic("oy", ImmutableList.of("7"))
467-
.addDynamicLocalStrategiesByMnemonic("oy", ImmutableList.of("5"))
468-
.addDynamicRemoteStrategiesByMnemonic("oy", ImmutableList.of("6"))
466+
.addDynamicLocalStrategies(ImmutableMap.of("oy", ImmutableList.of("7")))
467+
.addDynamicLocalStrategies(ImmutableMap.of("oy", ImmutableList.of("5")))
468+
.addDynamicRemoteStrategies(ImmutableMap.of("oy", ImmutableList.of("6")))
469469
.build();
470470

471471
strategyRegistry.notifyUsedDynamic(null);

0 commit comments

Comments
 (0)