From b1662874f7bee7bcc914cb7c9e8af5ac7ebb453a Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 11 Nov 2024 03:22:33 +0000 Subject: [PATCH 1/9] Filter spawn strategies by execution platform --- .../lib/bazel/rules/BazelStrategyModule.java | 5 ++ .../com/google/devtools/build/lib/exec/BUILD | 2 + .../build/lib/exec/ExecutionOptions.java | 17 ++++ .../lib/exec/RemoteLocalFallbackRegistry.java | 3 +- .../build/lib/exec/SpawnStrategyRegistry.java | 82 +++++++++++++++++-- .../build/lib/remote/RemoteSpawnRunner.java | 2 +- .../lib/exec/SpawnStrategyRegistryTest.java | 25 +++++- .../build/lib/exec/util/FakeOwner.java | 8 +- 8 files changed, 132 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java index f503180cada615..3a88de5bfeda19 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelStrategyModule.java @@ -18,6 +18,7 @@ import com.google.devtools.build.lib.analysis.actions.FileWriteActionContext; import com.google.devtools.build.lib.analysis.actions.TemplateExpansionContext; import com.google.devtools.build.lib.buildtool.BuildRequest; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.exec.ExecutionOptions; import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; import com.google.devtools.build.lib.exec.SpawnCache; @@ -90,5 +91,9 @@ public void registerSpawnStrategies( for (Map.Entry> entry : options.strategyByRegexp) { registryBuilder.addDescriptionFilter(entry.getKey(), entry.getValue()); } + + for (Map.Entry> strategy : options.allowedStrategiesByExecPlatform) { + registryBuilder.addExecPlatformFilter(Label.parseCanonicalUnchecked(strategy.getKey()), strategy.getValue()); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 058a96f8db9520..f6ae1c433e7e11 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -107,6 +107,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/util:resource_converter", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/common/options", + "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//third_party:guava", ], ) @@ -363,6 +364,7 @@ java_library( ":remote_local_fallback_registry", ":spawn_strategy_policy", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception", diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java index 811567aea62c74..8b9ee99bd83d0e 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java @@ -122,6 +122,23 @@ public class ExecutionOptions extends OptionsBase { + "the 'local' strategy, but reversing the order would run it with 'sandboxed'. ") public List>> strategyByRegexp; + @Option( + name = "allowed_strategies_by_exec_platform", + allowMultiple = true, + converter = Converters.StringToStringListConverter.class, + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = + """ + Filters spawn strategies by the execution platform. + Example: `--allowed_strategies_by_exec_platform=@platforms//host:host=local,sandboxed,worker` + to prevent actions configured for the host platform from being spawned remotely. + Example: `--allowed_strategies_by_exec_platform=//:linux_amd64=remote` to prevent actions + configured for a platform `//:linux_amd64` from being spawned locally. + """) + public List>> allowedStrategiesByExecPlatform; + @Option( name = "materialize_param_files", defaultValue = "false", diff --git a/src/main/java/com/google/devtools/build/lib/exec/RemoteLocalFallbackRegistry.java b/src/main/java/com/google/devtools/build/lib/exec/RemoteLocalFallbackRegistry.java index bfce3797d3cb36..14a020afa56cbf 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/RemoteLocalFallbackRegistry.java +++ b/src/main/java/com/google/devtools/build/lib/exec/RemoteLocalFallbackRegistry.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.exec; import com.google.devtools.build.lib.actions.ActionContext; +import com.google.devtools.build.lib.actions.Spawn; import javax.annotation.Nullable; /** @@ -29,5 +30,5 @@ public interface RemoteLocalFallbackRegistry extends ActionContext { * @return remote fallback strategy or {@code null} if none was registered */ @Nullable - AbstractSpawnStrategy getRemoteLocalFallbackStrategy(); + AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn); } diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java index 05281426ebd6d0..7f1c85b81c719f 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistry.java @@ -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 mnemonicToStrategies; private final StrategyRegexFilter strategyRegexFilter; + private final StrategyPlatformFilter strategyPlatformFilter; private final ImmutableList defaultStrategies; private final ImmutableMultimap mnemonicToRemoteDynamicStrategies; private final ImmutableMultimap mnemonicToLocalDynamicStrategies; @@ -77,12 +80,14 @@ public final class SpawnStrategyRegistry private SpawnStrategyRegistry( ImmutableListMultimap mnemonicToStrategies, StrategyRegexFilter strategyRegexFilter, + StrategyPlatformFilter strategyPlatformFilter, ImmutableList defaultStrategies, ImmutableMultimap mnemonicToRemoteDynamicStrategies, ImmutableMultimap 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 getStrategies(Spawn spawn, @Nullable EventHandler reporter) { - return getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter); + return strategyPlatformFilter.getStrategies( + spawn, getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter)); } /** @@ -116,6 +122,8 @@ public List getStrategies(Spawn spawn, @Nullable EventH * *

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 getStrategies( ActionExecutionMetadata resourceOwner, String mnemonic, @Nullable EventHandler reporter) { @@ -154,7 +162,7 @@ public ImmutableCollection 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 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 value = entry.getValue(); logger.atInfo().log( - "FilterToStrategyImplementations: \"%s\" = [%s]", + "FilterDescriptionToStrategyImplementations: \"%s\" = [%s]", entry.getKey(), toImplementationNames(value)); } + for (Map.Entry> entry : + strategyPlatformFilter.getFilterToStrategies().entrySet()) { + Collection value = entry.getValue(); + logger.atInfo().log( + "FilterPlatformToStrategyImplementations: \"%s\" = [%s]", + entry.getKey().getCanonicalName(), toImplementationNames(value)); + } logger.atInfo().log( "DefaultStrategyImplementations: [%s]", toImplementationNames(defaultStrategies)); @@ -287,6 +303,7 @@ public static final class Builder { private final HashMap> mnemonicToRemoteDynamicIdentifiers = new HashMap<>(); private final HashMap> mnemonicToLocalDynamicIdentifiers = new HashMap<>(); + private final HashMap> execPlatformFilters = new HashMap<>(); @Nullable private String remoteLocalFallbackStrategyIdentifier; @@ -316,6 +333,12 @@ public Builder addDescriptionFilter(RegexFilter filter, List identifiers return this; } + @CanIgnoreReturnValue + public Builder addExecPlatformFilter(Label execPlatform, List 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 @@ -446,6 +469,14 @@ public SpawnStrategyRegistry build() throws AbruptExitException { } } + ImmutableMap.Builder> platformToStrategies = ImmutableMap.builder(); + for (Map.Entry> entry : execPlatformFilters.entrySet()) { + Label platform = entry.getKey(); + platformToStrategies.put( + platform, + strategyMapper.toStrategies(entry.getValue(), "platform " + platform.getCanonicalName())); + } + ImmutableListMultimap.Builder mnemonicToStrategies = new ImmutableListMultimap.Builder<>(); for (Map.Entry> entry : mnemonicToIdentifiers.entrySet()) { @@ -516,6 +547,7 @@ public SpawnStrategyRegistry build() throws AbruptExitException { mnemonicToStrategies.build(), new StrategyRegexFilter( strategyMapper, strategyPolicy, filterToIdentifiers, filterToStrategies), + new StrategyPlatformFilter(strategyMapper, platformToStrategies.build()), defaultStrategies, mnemonicToRemoteStrategies.build(), mnemonicToLocalStrategies.build(), @@ -597,6 +629,46 @@ public String toString() { } } + private static class StrategyPlatformFilter { + private final StrategyMapper strategyMapper; + private final ImmutableMap> platformToStrategies; + + private StrategyPlatformFilter( + StrategyMapper strategyMapper, + ImmutableMap> platformToStrategies) { + this.strategyMapper = strategyMapper; + this.platformToStrategies = platformToStrategies; + } + + public List getStrategies( + Spawn spawn, List candidateStrategies) { + var platformLabel = spawn.getExecutionPlatformLabel(); + Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); + + var allowedStrategies = platformToStrategies.get(platformLabel); + if (allowedStrategies != null) { + List filteredStrategies = new ArrayList<>(); + for (var strategy : candidateStrategies) { + if (allowedStrategies.contains(strategy)) { + filteredStrategies.add(strategy); + } + } + return filteredStrategies; + } + + return candidateStrategies; + } + + public ImmutableCollection getStrategies( + Spawn spawn, ImmutableCollection candidateStrategies) { + return ImmutableList.copyOf(getStrategies(spawn, Lists.newCopyOnWriteArrayList(candidateStrategies))); + } + + ImmutableMap> getFilterToStrategies() { + return platformToStrategies; + } + } + /* Maps the strategy identifier (e.g. "local", "worker"..) to the real strategy. */ private static class StrategyMapper { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index fb4c8b86db1cb0..89b92192703ead 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -555,7 +555,7 @@ private SpawnResult execLocally(Spawn spawn, SpawnExecutionContext context) context.getContext(RemoteLocalFallbackRegistry.class); checkNotNull(localFallbackRegistry, "Expected a RemoteLocalFallbackRegistry to be registered"); AbstractSpawnStrategy remoteLocalFallbackStrategy = - localFallbackRegistry.getRemoteLocalFallbackStrategy(); + localFallbackRegistry.getRemoteLocalFallbackStrategy(spawn); checkNotNull( remoteLocalFallbackStrategy, "A remote local fallback strategy must be set if using remote fallback."); diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java index c6a4b2a79a39fc..c72a83f58dc163 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnStrategy; +import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.Event; @@ -300,6 +301,24 @@ public void testDuplicatedDescriptionFilter() throws Exception { .containsExactly(strategy2); } + @Test + public void testPlatformFilter() throws Exception { + NoopStrategy strategy1 = new NoopStrategy("1"); + NoopStrategy strategy2 = new NoopStrategy("2"); + SpawnStrategyRegistry strategyRegistry = + SpawnStrategyRegistry.builder() + .registerStrategy(strategy1, "foo") + .registerStrategy(strategy2, "bar") + .addExecPlatformFilter(Label.parseCanonical("//:dummy_platform"), ImmutableList.of("foo")) + .build(); + + assertThat( + strategyRegistry.getStrategies( + createSpawnWithMnemonicAndDescription("", ""), + SpawnStrategyRegistryTest::noopEventHandler)) + .containsExactly(strategy1); + } + @Test public void testMultipleDefaultStrategies() throws Exception { NoopStrategy strategy1 = new NoopStrategy("1"); @@ -537,7 +556,7 @@ public void testRemoteLocalFallback() throws Exception { .setRemoteLocalFallbackStrategyIdentifier("bar") .build(); - assertThat(strategyRegistry.getRemoteLocalFallbackStrategy()).isEqualTo(strategy2); + assertThat(strategyRegistry.getRemoteLocalFallbackStrategy(createSpawnWithMnemonicAndDescription("", ""))).isEqualTo(strategy2); } @Test @@ -561,7 +580,7 @@ public void testRemoteLocalFallbackNotRegistered() throws Exception { SpawnStrategyRegistry strategyRegistry = SpawnStrategyRegistry.builder().registerStrategy(strategy1, "foo").build(); - assertThat(strategyRegistry.getRemoteLocalFallbackStrategy()).isNull(); + assertThat(strategyRegistry.getRemoteLocalFallbackStrategy(createSpawnWithMnemonicAndDescription("", ""))).isNull(); } @Test @@ -649,7 +668,7 @@ public void testNotifyUsedDynamic() throws Exception { assertThat(strategy7.usedCalled).isEqualTo(0); } - private Spawn createSpawnWithMnemonicAndDescription(String mnemonic, String description) { + private Spawn createSpawnWithMnemonicAndDescription(String mnemonic, String description) throws Exception { return new SimpleSpawn( new FakeOwner(mnemonic, description, "//dummy:label"), ImmutableList.of(), diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java index 2f52090552d434..0fb20c51c7ac8b 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java @@ -80,8 +80,12 @@ private FakeOwner( /* isBuiltForToolConfiguration= */ false); } - public FakeOwner(String mnemonic, String progressMessage, String ownerLabel) { - this(mnemonic, progressMessage, checkNotNull(ownerLabel), null); + public FakeOwner(String mnemonic, String progressMessage, String ownerLabel) throws Exception { + this( + mnemonic, + progressMessage, + checkNotNull(ownerLabel), + PlatformInfo.builder().setLabel(Label.parseCanonical("//:dummy_platform")).build()); } @Override From 9c1e9ffb71284cfbd507eb6865191587f6e83bf3 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 11 Nov 2024 03:46:27 +0000 Subject: [PATCH 2/9] Update "spawn cannot be executed" error advice --- .../devtools/build/lib/exec/SpawnStrategyResolver.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java index 8a8182c2272e2d..830c8ce07feab0 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnStrategyResolver.java @@ -86,10 +86,12 @@ public List resolve( if (fallbackStrategies.isEmpty()) { String message = String.format( - "%s spawn cannot be executed with any of the available strategies: %s. Your" - + " --spawn_strategy, --genrule_strategy and/or --strategy flags are probably" - + " too strict. Visit https://github.com/bazelbuild/bazel/issues/7480 for" - + " advice", + """ + %s spawn cannot be executed with any of the available strategies: %s. Your \ + --spawn_strategy, --genrule_strategy, strategy and/or \ + --allowed_strategies_by_exec_platform flags are probably too strict. \ + Visit https://github.com/bazelbuild/bazel/issues/7480 for advice. + """, spawn.getMnemonic(), strategies); throw new UserExecException( FailureDetail.newBuilder() From 4da7fbb9952d15229d0b68c6783061e77c2295e9 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 11 Nov 2024 04:12:26 +0000 Subject: [PATCH 3/9] Conceal exception for tests --- .../build/lib/exec/SpawnStrategyRegistryTest.java | 2 +- .../devtools/build/lib/exec/util/FakeOwner.java | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java index c72a83f58dc163..1694a103ff0abe 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java @@ -668,7 +668,7 @@ public void testNotifyUsedDynamic() throws Exception { assertThat(strategy7.usedCalled).isEqualTo(0); } - private Spawn createSpawnWithMnemonicAndDescription(String mnemonic, String description) throws Exception { + private Spawn createSpawnWithMnemonicAndDescription(String mnemonic, String description) { return new SimpleSpawn( new FakeOwner(mnemonic, description, "//dummy:label"), ImmutableList.of(), diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java index 0fb20c51c7ac8b..cf7166460046a0 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java @@ -80,12 +80,20 @@ private FakeOwner( /* isBuiltForToolConfiguration= */ false); } - public FakeOwner(String mnemonic, String progressMessage, String ownerLabel) throws Exception { + public FakeOwner(String mnemonic, String progressMessage, String ownerLabel) { this( mnemonic, progressMessage, checkNotNull(ownerLabel), - PlatformInfo.builder().setLabel(Label.parseCanonical("//:dummy_platform")).build()); + createPlatformInfo()); + } + + private static PlatformInfo createPlatformInfo() { + try { + return PlatformInfo.builder().setLabel(Label.parseCanonical("//:dummy_platform")).build(); + } catch (Exception ex) { + throw new RuntimeException("Fake PlatformInfo construction failed.", ex); + } } @Override From b5f63486fc9dd2d0643787f7bdc1c34f0728b61d Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 11 Nov 2024 04:13:17 +0000 Subject: [PATCH 4/9] Fix missed interface change consequence --- .../google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java index fc319076995472..2f1355695dad04 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java @@ -265,7 +265,7 @@ private FakeSpawnExecutionContext getSpawnContext(Spawn spawn) { AbstractSpawnStrategy fakeLocalStrategy = new AbstractSpawnStrategy(execRoot, localRunner, new ExecutionOptions()) {}; ClassToInstanceMap actionContextRegistry = - ImmutableClassToInstanceMap.of(RemoteLocalFallbackRegistry.class, () -> fakeLocalStrategy); + ImmutableClassToInstanceMap.of(RemoteLocalFallbackRegistry.class, (_spawn) -> fakeLocalStrategy); var actionInputFetcher = new RemoteActionInputFetcher( From d02829e8f991843c1e7bb0dd156c5b804a1acab8 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 10 Feb 2025 08:03:34 +0000 Subject: [PATCH 5/9] Fix digest mismatch due to platform presence --- .../google/devtools/build/lib/exec/util/FakeOwner.java | 10 +--------- .../java/com/google/devtools/build/lib/remote/BUILD | 1 + .../RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java | 2 ++ 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java index 5ff8a14dae9724..f01b0a8d19d2cd 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java +++ b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java @@ -84,15 +84,7 @@ public FakeOwner(String mnemonic, String progressMessage, String ownerLabel) { mnemonic, progressMessage, checkNotNull(ownerLabel), - createPlatformInfo()); - } - - private static PlatformInfo createPlatformInfo() { - try { - return PlatformInfo.builder().setLabel(Label.parseCanonical("//:dummy_platform")).build(); - } catch (Exception ex) { - throw new RuntimeException("Fake PlatformInfo construction failed.", ex); - } + PlatformInfo.EMPTY_PLATFORM_INFO); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index 0afa13bb3aca7a..eb4e3627025180 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -91,6 +91,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:symlink_entry", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/analysis/platform", + "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils", "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index c171c3bb6c79ba..30637a4649c278 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -76,6 +76,7 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.StaticInputMetadataProvider; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; +import com.google.devtools.build.lib.analysis.platform.PlatformUtils; import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; import com.google.devtools.build.lib.clock.JavaClock; @@ -378,6 +379,7 @@ public int maxConcurrency() { .setValue("value") .build()) .addAllOutputPaths(ImmutableList.of("bar", "foo")) + .setPlatform(PlatformUtils.getPlatformProto(simpleSpawn, null)) .build(); cmdDigest = DIGEST_UTIL.compute(command); channel.release(); From 0c32764d0b3958340331cb796a93c9a0a962d2f2 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 10 Feb 2025 08:47:38 +0000 Subject: [PATCH 6/9] Suppress `platform` proto generation when no properties --- .../devtools/build/lib/analysis/platform/PlatformUtils.java | 5 +++++ src/test/java/com/google/devtools/build/lib/remote/BUILD | 1 - .../remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java | 2 -- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java index 3366b6b3ab39fe..cb0b54d692a2d9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java @@ -76,6 +76,7 @@ public static Platform getPlatformProto( ? remoteOptions.getRemoteDefaultExecProperties() : ImmutableSortedMap.of(); + // TODO Rewrite `spawn.getExecutionPlatform() == null`, should always be set if (spawn.getExecutionPlatform() == null && spawn.getCombinedExecProperties().isEmpty() && defaultExecProperties.isEmpty() @@ -133,6 +134,10 @@ public static Platform getPlatformProto( } } + if (properties.size() == 0) { + return null; + } + Platform.Builder platformBuilder = Platform.newBuilder(); for (Map.Entry entry : properties.entrySet()) { platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue()); diff --git a/src/test/java/com/google/devtools/build/lib/remote/BUILD b/src/test/java/com/google/devtools/build/lib/remote/BUILD index eb4e3627025180..0afa13bb3aca7a 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/BUILD @@ -91,7 +91,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:symlink_entry", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/analysis/platform", - "//src/main/java/com/google/devtools/build/lib/analysis/platform:platform_utils", "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper", "//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module", diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java index 30637a4649c278..c171c3bb6c79ba 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java @@ -76,7 +76,6 @@ import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.StaticInputMetadataProvider; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; -import com.google.devtools.build.lib.analysis.platform.PlatformUtils; import com.google.devtools.build.lib.authandtls.CallCredentialsProvider; import com.google.devtools.build.lib.authandtls.GoogleAuthUtils; import com.google.devtools.build.lib.clock.JavaClock; @@ -379,7 +378,6 @@ public int maxConcurrency() { .setValue("value") .build()) .addAllOutputPaths(ImmutableList.of("bar", "foo")) - .setPlatform(PlatformUtils.getPlatformProto(simpleSpawn, null)) .build(); cmdDigest = DIGEST_UTIL.compute(command); channel.release(); From fca48281fe1a6d7dfc6df533dbe6d438d74f28e0 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 10 Feb 2025 08:53:08 +0000 Subject: [PATCH 7/9] Earlier exit --- .../build/lib/analysis/platform/PlatformUtils.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java index cb0b54d692a2d9..3db8d6b862975e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java @@ -76,8 +76,7 @@ public static Platform getPlatformProto( ? remoteOptions.getRemoteDefaultExecProperties() : ImmutableSortedMap.of(); - // TODO Rewrite `spawn.getExecutionPlatform() == null`, should always be set - if (spawn.getExecutionPlatform() == null + if ((spawn.getExecutionPlatform() == null || spawn.getExecutionPlatform().execProperties().isEmpty()) && spawn.getCombinedExecProperties().isEmpty() && defaultExecProperties.isEmpty() && additionalProperties.isEmpty()) { @@ -134,10 +133,6 @@ public static Platform getPlatformProto( } } - if (properties.size() == 0) { - return null; - } - Platform.Builder platformBuilder = Platform.newBuilder(); for (Map.Entry entry : properties.entrySet()) { platformBuilder.addPropertiesBuilder().setName(entry.getKey()).setValue(entry.getValue()); From 88395ce76f1904411bb9e7228c44681dcf69637a Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 10 Feb 2025 10:33:56 +0000 Subject: [PATCH 8/9] Add missing check for remote exec properties --- .../devtools/build/lib/analysis/platform/PlatformUtils.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java index 3db8d6b862975e..6f9315a91b6895 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java @@ -76,7 +76,11 @@ public static Platform getPlatformProto( ? remoteOptions.getRemoteDefaultExecProperties() : ImmutableSortedMap.of(); - if ((spawn.getExecutionPlatform() == null || spawn.getExecutionPlatform().execProperties().isEmpty()) + if ((spawn.getExecutionPlatform() == null + || ( + spawn.getExecutionPlatform().execProperties().isEmpty() + && spawn.getExecutionPlatform().remoteExecutionProperties().isEmpty() + )) && spawn.getCombinedExecProperties().isEmpty() && defaultExecProperties.isEmpty() && additionalProperties.isEmpty()) { From 01357a04289b7687350f4ffe32f64192d2284cd5 Mon Sep 17 00:00:00 2001 From: Jordan Mele Date: Mon, 10 Feb 2025 12:09:35 +0000 Subject: [PATCH 9/9] Fix `testPlatformFilter` --- src/test/java/com/google/devtools/build/lib/exec/BUILD | 1 + .../devtools/build/lib/exec/SpawnStrategyRegistryTest.java | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/google/devtools/build/lib/exec/BUILD b/src/test/java/com/google/devtools/build/lib/exec/BUILD index bd7978c336e65c..bb4233da23c1e6 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/BUILD @@ -45,6 +45,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", + "//src/main/java/com/google/devtools/build/lib/analysis/platform", "//src/main/java/com/google/devtools/build/lib/bazel/rules/python", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", "//src/main/java/com/google/devtools/build/lib/clock", diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java index 1694a103ff0abe..84e32d76ab5bf7 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnStrategyRegistryTest.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.actions.Spawn; import com.google.devtools.build.lib.actions.SpawnResult; import com.google.devtools.build.lib.actions.SpawnStrategy; +import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -309,7 +310,7 @@ public void testPlatformFilter() throws Exception { SpawnStrategyRegistry.builder() .registerStrategy(strategy1, "foo") .registerStrategy(strategy2, "bar") - .addExecPlatformFilter(Label.parseCanonical("//:dummy_platform"), ImmutableList.of("foo")) + .addExecPlatformFilter(PlatformInfo.EMPTY_PLATFORM_INFO.label(), ImmutableList.of("foo")) .build(); assertThat(