-
Notifications
You must be signed in to change notification settings - Fork 0
Filter spawn strategies by execution platform #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
b166287
9c1e9ff
4da7fbb
b5f6348
3d17888
d02829e
0c32764
c7ca9b1
fca4828
88395ce
01357a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
import com.google.common.collect.ImmutableCollection; | ||
import com.google.common.collect.ImmutableList; | ||
import com.google.common.collect.ImmutableListMultimap; | ||
import com.google.common.collect.ImmutableMap; | ||
import com.google.common.collect.ImmutableMultimap; | ||
import com.google.common.collect.LinkedListMultimap; | ||
import com.google.common.collect.ListMultimap; | ||
|
@@ -34,6 +35,7 @@ | |
import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy; | ||
import com.google.devtools.build.lib.actions.Spawn; | ||
import com.google.devtools.build.lib.actions.SpawnStrategy; | ||
import com.google.devtools.build.lib.cmdline.Label; | ||
import com.google.devtools.build.lib.events.Event; | ||
import com.google.devtools.build.lib.events.EventHandler; | ||
import com.google.devtools.build.lib.events.Reporter; | ||
|
@@ -69,6 +71,7 @@ public final class SpawnStrategyRegistry | |
|
||
private final ImmutableListMultimap<String, SpawnStrategy> mnemonicToStrategies; | ||
private final StrategyRegexFilter strategyRegexFilter; | ||
private final StrategyPlatformFilter strategyPlatformFilter; | ||
private final ImmutableList<? extends SpawnStrategy> defaultStrategies; | ||
private final ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToRemoteDynamicStrategies; | ||
private final ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToLocalDynamicStrategies; | ||
|
@@ -77,12 +80,14 @@ public final class SpawnStrategyRegistry | |
private SpawnStrategyRegistry( | ||
ImmutableListMultimap<String, SpawnStrategy> mnemonicToStrategies, | ||
StrategyRegexFilter strategyRegexFilter, | ||
StrategyPlatformFilter strategyPlatformFilter, | ||
ImmutableList<? extends SpawnStrategy> defaultStrategies, | ||
ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToRemoteDynamicStrategies, | ||
ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToLocalDynamicStrategies, | ||
@Nullable AbstractSpawnStrategy remoteLocalFallbackStrategy) { | ||
this.mnemonicToStrategies = mnemonicToStrategies; | ||
this.strategyRegexFilter = strategyRegexFilter; | ||
this.strategyPlatformFilter = strategyPlatformFilter; | ||
this.defaultStrategies = defaultStrategies; | ||
this.mnemonicToRemoteDynamicStrategies = mnemonicToRemoteDynamicStrategies; | ||
this.mnemonicToLocalDynamicStrategies = mnemonicToLocalDynamicStrategies; | ||
|
@@ -105,7 +110,8 @@ private SpawnStrategyRegistry( | |
* using the given {@link Reporter}. | ||
*/ | ||
public List<? extends SpawnStrategy> getStrategies(Spawn spawn, @Nullable EventHandler reporter) { | ||
return getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter); | ||
return strategyPlatformFilter.getStrategies( | ||
spawn, getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter)); | ||
} | ||
|
||
Comment on lines
+113
to
116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Platform filtering is applied, but null execution-platforms will crash builds
Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); Many host-only actions (coverage report, build‐info extraction, etc.) still run with a Please make the filter a no-op when - var platformLabel = spawn.getExecutionPlatformLabel();
- Preconditions.checkNotNull(platformLabel,
- "Attempting to spawn action without an execution platform.");
+ @Nullable var platformLabel = spawn.getExecutionPlatformLabel();
+ if (platformLabel == null) {
+ // Legacy or host actions – no platform-specific restrictions.
+ return candidateStrategies;
+ } |
||
/** | ||
|
@@ -116,6 +122,8 @@ public List<? extends SpawnStrategy> getStrategies(Spawn spawn, @Nullable EventH | |
* | ||
* <p>If the reason for selecting the context is worth mentioning to the user, logs a message | ||
* using the given {@link Reporter}. | ||
* | ||
* NOTE: This method is public for Blaze, getStrategies(Spawn, EventHandler) must be used in Bazel. | ||
*/ | ||
public List<? extends SpawnStrategy> getStrategies( | ||
ActionExecutionMetadata resourceOwner, String mnemonic, @Nullable EventHandler reporter) { | ||
|
@@ -154,7 +162,7 @@ public ImmutableCollection<SandboxedSpawnStrategy> getDynamicSpawnActionContexts | |
? mnemonicToRemoteDynamicStrategies | ||
: mnemonicToLocalDynamicStrategies; | ||
if (mnemonicToDynamicStrategies.containsKey(spawn.getMnemonic())) { | ||
return mnemonicToDynamicStrategies.get(spawn.getMnemonic()); | ||
return strategyPlatformFilter.getStrategies(spawn, mnemonicToDynamicStrategies.get(spawn.getMnemonic())); | ||
} | ||
if (mnemonicToDynamicStrategies.containsKey("")) { | ||
return mnemonicToDynamicStrategies.get(""); | ||
|
@@ -164,8 +172,9 @@ public ImmutableCollection<SandboxedSpawnStrategy> getDynamicSpawnActionContexts | |
|
||
@Nullable | ||
@Override | ||
public AbstractSpawnStrategy getRemoteLocalFallbackStrategy() { | ||
return remoteLocalFallbackStrategy; | ||
public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | ||
return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | ||
.getFirst(); | ||
} | ||
|
||
/** | ||
|
@@ -203,9 +212,16 @@ void logSpawnStrategies() { | |
strategyRegexFilter.getFilterToStrategies().asMap().entrySet()) { | ||
Collection<SpawnStrategy> value = entry.getValue(); | ||
logger.atInfo().log( | ||
"FilterToStrategyImplementations: \"%s\" = [%s]", | ||
"FilterDescriptionToStrategyImplementations: \"%s\" = [%s]", | ||
entry.getKey(), toImplementationNames(value)); | ||
} | ||
for (Map.Entry<Label, ImmutableList<SpawnStrategy>> entry : | ||
strategyPlatformFilter.getFilterToStrategies().entrySet()) { | ||
Collection<SpawnStrategy> value = entry.getValue(); | ||
logger.atInfo().log( | ||
"FilterPlatformToStrategyImplementations: \"%s\" = [%s]", | ||
entry.getKey().getCanonicalName(), toImplementationNames(value)); | ||
Comment on lines
+218
to
+223
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
logger.atInfo().log( | ||
"DefaultStrategyImplementations: [%s]", toImplementationNames(defaultStrategies)); | ||
|
@@ -287,6 +303,7 @@ public static final class Builder { | |
private final HashMap<String, List<String>> mnemonicToRemoteDynamicIdentifiers = | ||
new HashMap<>(); | ||
private final HashMap<String, List<String>> mnemonicToLocalDynamicIdentifiers = new HashMap<>(); | ||
private final HashMap<Label, List<String>> execPlatformFilters = new HashMap<>(); | ||
|
||
@Nullable private String remoteLocalFallbackStrategyIdentifier; | ||
|
||
|
@@ -314,6 +331,12 @@ public Builder addDescriptionFilter(RegexFilter filter, List<String> identifiers | |
return this; | ||
} | ||
|
||
@CanIgnoreReturnValue | ||
public Builder addExecPlatformFilter(Label execPlatform, List<String> identifiers) { | ||
this.execPlatformFilters.put(execPlatform, identifiers); | ||
return this; | ||
} | ||
|
||
/** | ||
* Adds a filter limiting any spawn whose {@linkplain Spawn#getMnemonic() mnemonic} | ||
* (case-sensitively) matches the given mnemonic to only use strategies with the given | ||
|
@@ -444,6 +467,14 @@ public SpawnStrategyRegistry build() throws AbruptExitException { | |
} | ||
} | ||
|
||
ImmutableMap.Builder<Label, ImmutableList<SpawnStrategy>> platformToStrategies = ImmutableMap.builder(); | ||
for (Map.Entry<Label, List<String>> entry : execPlatformFilters.entrySet()) { | ||
Label platform = entry.getKey(); | ||
platformToStrategies.put( | ||
platform, | ||
strategyMapper.toStrategies(entry.getValue(), "platform " + platform.getCanonicalName())); | ||
} | ||
|
||
ImmutableListMultimap.Builder<String, SpawnStrategy> mnemonicToStrategies = | ||
new ImmutableListMultimap.Builder<>(); | ||
for (Map.Entry<String, List<String>> entry : mnemonicToIdentifiers.entrySet()) { | ||
|
@@ -514,6 +545,7 @@ public SpawnStrategyRegistry build() throws AbruptExitException { | |
mnemonicToStrategies.build(), | ||
new StrategyRegexFilter( | ||
strategyMapper, strategyPolicy, filterToIdentifiers, filterToStrategies), | ||
new StrategyPlatformFilter(strategyMapper, platformToStrategies.build()), | ||
defaultStrategies, | ||
mnemonicToRemoteStrategies.build(), | ||
mnemonicToLocalStrategies.build(), | ||
|
@@ -595,6 +627,46 @@ public String toString() { | |
} | ||
} | ||
|
||
private static class StrategyPlatformFilter { | ||
private final StrategyMapper strategyMapper; | ||
private final ImmutableMap<Label, ImmutableList<SpawnStrategy>> platformToStrategies; | ||
|
||
private StrategyPlatformFilter( | ||
StrategyMapper strategyMapper, | ||
ImmutableMap<Label, ImmutableList<SpawnStrategy>> platformToStrategies) { | ||
this.strategyMapper = strategyMapper; | ||
this.platformToStrategies = platformToStrategies; | ||
} | ||
|
||
public <T extends SpawnStrategy> List<T> getStrategies( | ||
Spawn spawn, List<T> candidateStrategies) { | ||
var platformLabel = spawn.getExecutionPlatformLabel(); | ||
Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); | ||
Comment on lines
+643
to
+644
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This if (platformLabel == null) {
// Log a warning or throw a more informative exception here
return candidateStrategies;
}
Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); |
||
|
||
var allowedStrategies = platformToStrategies.get(platformLabel); | ||
if (allowedStrategies != null) { | ||
List<T> filteredStrategies = new ArrayList<>(); | ||
for (var strategy : candidateStrategies) { | ||
if (allowedStrategies.contains(strategy)) { | ||
filteredStrategies.add(strategy); | ||
} | ||
} | ||
return filteredStrategies; | ||
} | ||
|
||
return candidateStrategies; | ||
} | ||
|
||
public <T extends SpawnStrategy> ImmutableCollection<T> getStrategies( | ||
Spawn spawn, ImmutableCollection<T> candidateStrategies) { | ||
return ImmutableList.copyOf(getStrategies(spawn, Lists.newCopyOnWriteArrayList(candidateStrategies))); | ||
} | ||
|
||
ImmutableMap<Label, ImmutableList<SpawnStrategy>> getFilterToStrategies() { | ||
return platformToStrategies; | ||
} | ||
} | ||
|
||
/* Maps the strategy identifier (e.g. "local", "worker"..) to the real strategy. */ | ||
private static class StrategyMapper { | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Implementation looks good but lacks error handling for malformed labels.
The new loop properly iterates over the platform-specific strategy mappings and adds them to the registry. This implements the core functionality needed for the platform-scoped spawn strategies.
However,
parseCanonicalUnchecked
could throw a runtime exception if a malformed label is provided. Consider whether adding error handling for malformed labels would improve user experience:🏁 Script executed:
Length of output: 2053
Add error handling for malformed platform labels
While the loop correctly registers each
<exec_platform, strategies>
entry, a malformed platform label will surface as an uncaughtIllegalArgumentException
at runtime. To improve usability, wrap theLabel.parseCanonicalUnchecked
call in atry
/catch
and emit a clear, user-facing error (for example, anOptionsParsingException
) indicating which key was invalid:• File:
src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java
Lines ~95–97
Suggested adjustment:
This ensures malformed labels are caught early and reported with a descriptive message.