Skip to content

Commit 5593358

Browse files
katrecopybara-github
katre
authored and
copybara-github
committed
Update ConfiguredTargetFunction.computeUnloadedToolchainContexts to
directly require the correct execution platform. This fixes several issues around toolchains that depend on toolchains. Fixes bazelbuild#13243. Closes bazelbuild#13258. PiperOrigin-RevId: 364597996
1 parent 239a8e7 commit 5593358

File tree

8 files changed

+282
-72
lines changed

8 files changed

+282
-72
lines changed

‎src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -520,18 +520,31 @@ static ToolchainCollection<UnloadedToolchainContext> computeUnloadedToolchainCon
520520

521521
Map<String, ToolchainContextKey> toolchainContextKeys = new HashMap<>();
522522
String targetUnloadedToolchainContext = "target-unloaded-toolchain-context";
523-
ToolchainContextKey toolchainContextKey;
523+
524+
ToolchainContextKey.Builder toolchainContextKeyBuilder =
525+
ToolchainContextKey.key()
526+
.configurationKey(toolchainConfig)
527+
.requiredToolchainTypeLabels(requiredDefaultToolchains)
528+
.execConstraintLabels(defaultExecConstraintLabels)
529+
.shouldSanityCheckConfiguration(configuration.trimConfigurationsRetroactively());
530+
524531
if (parentToolchainContextKey != null) {
525-
toolchainContextKey = parentToolchainContextKey;
526-
} else {
527-
toolchainContextKey =
528-
ToolchainContextKey.key()
529-
.configurationKey(toolchainConfig)
530-
.requiredToolchainTypeLabels(requiredDefaultToolchains)
531-
.execConstraintLabels(defaultExecConstraintLabels)
532-
.shouldSanityCheckConfiguration(configuration.trimConfigurationsRetroactively())
533-
.build();
532+
// Find out what execution platform the parent used, and force that.
533+
// This key should always be present, but check just in case.
534+
ToolchainContext parentToolchainContext =
535+
(ToolchainContext)
536+
env.getValueOrThrow(parentToolchainContextKey, ToolchainException.class);
537+
if (env.valuesMissing()) {
538+
return null;
539+
}
540+
541+
Label execPlatform = parentToolchainContext.executionPlatform().label();
542+
if (execPlatform != null) {
543+
toolchainContextKeyBuilder.forceExecutionPlatform(execPlatform);
544+
}
534545
}
546+
547+
ToolchainContextKey toolchainContextKey = toolchainContextKeyBuilder.build();
535548
toolchainContextKeys.put(targetUnloadedToolchainContext, toolchainContextKey);
536549
for (Map.Entry<String, ExecGroup> group : execGroups.entrySet()) {
537550
ExecGroup execGroup = group.getValue();
@@ -558,14 +571,6 @@ static ToolchainCollection<UnloadedToolchainContext> computeUnloadedToolchainCon
558571
(UnloadedToolchainContext) values.get(unloadedToolchainContextKey.getValue()).get();
559572
if (!valuesMissing) {
560573
String execGroup = unloadedToolchainContextKey.getKey();
561-
if (parentToolchainContextKey != null) {
562-
// Since we inherited the toolchain context from the parent of the dependency, the current
563-
// target may also be in the resolved toolchains list. We need to clear it out.
564-
// TODO(configurability): When updating this for config_setting, only remove the current
565-
// target, not everything, because config_setting might want to check the toolchain
566-
// dependencies.
567-
unloadedToolchainContext = unloadedToolchainContext.withoutResolvedToolchains();
568-
}
569574
if (execGroup.equals(targetUnloadedToolchainContext)) {
570575
toolchainContexts.addDefaultContext(unloadedToolchainContext);
571576
} else {

‎src/main/java/com/google/devtools/build/lib/skyframe/ToolchainContextKey.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,12 @@
1414
package com.google.devtools.build.lib.skyframe;
1515

1616
import com.google.auto.value.AutoValue;
17+
import com.google.common.base.Preconditions;
1718
import com.google.common.collect.ImmutableSet;
1819
import com.google.devtools.build.lib.cmdline.Label;
1920
import com.google.devtools.build.skyframe.SkyFunctionName;
2021
import com.google.devtools.build.skyframe.SkyKey;
22+
import java.util.Optional;
2123

2224
/**
2325
* {@link SkyKey} implementation used for {@link ToolchainResolutionFunction} to produce {@link
@@ -31,7 +33,8 @@ public static Builder key() {
3133
return new AutoValue_ToolchainContextKey.Builder()
3234
.requiredToolchainTypeLabels(ImmutableSet.of())
3335
.execConstraintLabels(ImmutableSet.of())
34-
.shouldSanityCheckConfiguration(false);
36+
.shouldSanityCheckConfiguration(false)
37+
.forceExecutionPlatform(Optional.empty());
3538
}
3639

3740
@Override
@@ -47,6 +50,8 @@ public SkyFunctionName functionName() {
4750

4851
abstract boolean shouldSanityCheckConfiguration();
4952

53+
abstract Optional<Label> forceExecutionPlatform();
54+
5055
/** Builder for {@link ToolchainContextKey}. */
5156
@AutoValue.Builder
5257
public interface Builder {
@@ -63,5 +68,12 @@ public interface Builder {
6368
Builder shouldSanityCheckConfiguration(boolean shouldSanityCheckConfiguration);
6469

6570
ToolchainContextKey build();
71+
72+
Builder forceExecutionPlatform(Optional<Label> execPlatform);
73+
74+
default Builder forceExecutionPlatform(Label execPlatform) {
75+
Preconditions.checkNotNull(execPlatform);
76+
return forceExecutionPlatform(Optional.of(execPlatform));
77+
}
6678
}
6779
}

‎src/main/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunction.java‎

Lines changed: 58 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ public UnloadedToolchainContext compute(SkyKey skyKey, Environment env)
117117
env,
118118
key.configurationKey(),
119119
resolvedToolchainTypeLabels,
120+
key.forceExecutionPlatform().map(platformKeys::find),
120121
builder,
121122
platformKeys,
122123
key.shouldSanityCheckConfiguration());
@@ -182,6 +183,24 @@ abstract static class PlatformKeys {
182183

183184
abstract ImmutableList<ConfiguredTargetKey> executionPlatformKeys();
184185

186+
@Nullable
187+
public ConfiguredTargetKey find(Label platformLabel) {
188+
if (platformLabel.equals(hostPlatformKey().getLabel())) {
189+
return hostPlatformKey();
190+
}
191+
if (platformLabel.equals(targetPlatformKey().getLabel())) {
192+
return targetPlatformKey();
193+
}
194+
195+
for (ConfiguredTargetKey configuredTargetKey : executionPlatformKeys()) {
196+
if (platformLabel.equals(configuredTargetKey.getLabel())) {
197+
return configuredTargetKey;
198+
}
199+
}
200+
201+
return null;
202+
}
203+
185204
static PlatformKeys create(
186205
ConfiguredTargetKey hostPlatformKey,
187206
ConfiguredTargetKey targetPlatformKey,
@@ -350,6 +369,7 @@ private void determineToolchainImplementations(
350369
Environment environment,
351370
BuildConfigurationValue.Key configurationKey,
352371
ImmutableSet<Label> requiredToolchainTypeLabels,
372+
Optional<ConfiguredTargetKey> forcedExecutionPlatform,
353373
UnloadedToolchainContextImpl.Builder builder,
354374
PlatformKeys platformKeys,
355375
boolean shouldSanityCheckConfiguration)
@@ -415,19 +435,12 @@ private void determineToolchainImplementations(
415435
ImmutableSet<ToolchainTypeInfo> requiredToolchainTypes = requiredToolchainTypesBuilder.build();
416436

417437
// Find and return the first execution platform which has all required toolchains.
418-
Optional<ConfiguredTargetKey> selectedExecutionPlatformKey;
419-
if (!requiredToolchainTypeLabels.isEmpty()) {
420-
selectedExecutionPlatformKey =
421-
findExecutionPlatformForToolchains(
422-
requiredToolchainTypes,
423-
platformKeys.executionPlatformKeys(),
424-
resolvedToolchains);
425-
} else if (!platformKeys.executionPlatformKeys().isEmpty()) {
426-
// Just use the first execution platform.
427-
selectedExecutionPlatformKey = Optional.of(platformKeys.executionPlatformKeys().get(0));
428-
} else {
429-
selectedExecutionPlatformKey = Optional.empty();
430-
}
438+
Optional<ConfiguredTargetKey> selectedExecutionPlatformKey =
439+
findExecutionPlatformForToolchains(
440+
requiredToolchainTypes,
441+
forcedExecutionPlatform,
442+
platformKeys.executionPlatformKeys(),
443+
resolvedToolchains);
431444

432445
if (!selectedExecutionPlatformKey.isPresent()) {
433446
throw new NoMatchingPlatformException(
@@ -477,25 +490,47 @@ private static Table<ConfiguredTargetKey, ToolchainTypeInfo, Label> findPlatform
477490
*/
478491
private static Optional<ConfiguredTargetKey> findExecutionPlatformForToolchains(
479492
ImmutableSet<ToolchainTypeInfo> requiredToolchainTypes,
493+
Optional<ConfiguredTargetKey> forcedExecutionPlatform,
480494
ImmutableList<ConfiguredTargetKey> availableExecutionPlatformKeys,
481495
Table<ConfiguredTargetKey, ToolchainTypeInfo, Label> resolvedToolchains) {
482-
for (ConfiguredTargetKey executionPlatformKey : availableExecutionPlatformKeys) {
483-
if (!resolvedToolchains.containsRow(executionPlatformKey)) {
484-
continue;
485-
}
486496

487-
Map<ToolchainTypeInfo, Label> toolchains = resolvedToolchains.row(executionPlatformKey);
488-
if (!toolchains.keySet().containsAll(requiredToolchainTypes)) {
489-
// Not all toolchains are present, ignore this execution platform.
490-
continue;
497+
if (forcedExecutionPlatform.isPresent()) {
498+
// Is the forced platform suitable?
499+
if (isPlatformSuitable(
500+
forcedExecutionPlatform.get(), requiredToolchainTypes, resolvedToolchains)) {
501+
return forcedExecutionPlatform;
491502
}
503+
}
504+
505+
return availableExecutionPlatformKeys.stream()
506+
.filter(epk -> isPlatformSuitable(epk, requiredToolchainTypes, resolvedToolchains))
507+
.findFirst();
508+
}
509+
510+
private static boolean isPlatformSuitable(
511+
ConfiguredTargetKey executionPlatformKey,
512+
ImmutableSet<ToolchainTypeInfo> requiredToolchainTypes,
513+
Table<ConfiguredTargetKey, ToolchainTypeInfo, Label> resolvedToolchains) {
514+
if (requiredToolchainTypes.isEmpty()) {
515+
// Since there aren't any toolchains, we should be able to use any execution platform that
516+
// has made it this far.
517+
return true;
518+
}
492519

493-
return Optional.of(executionPlatformKey);
520+
if (!resolvedToolchains.containsRow(executionPlatformKey)) {
521+
return false;
494522
}
495523

496-
return Optional.empty();
524+
Map<ToolchainTypeInfo, Label> toolchains = resolvedToolchains.row(executionPlatformKey);
525+
if (!toolchains.keySet().containsAll(requiredToolchainTypes)) {
526+
// Not all toolchains are present, ignore this execution platform.
527+
return false;
528+
}
529+
530+
return true;
497531
}
498532

533+
499534
@Nullable
500535
@Override
501536
public String extractTag(SkyKey skyKey) {

‎src/main/java/com/google/devtools/build/lib/skyframe/UnloadedToolchainContext.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,4 @@ public interface UnloadedToolchainContext extends ToolchainContext, SkyValue {
4141

4242
@Override
4343
ImmutableSet<Label> resolvedToolchainLabels();
44-
45-
/** Returns a copy of this context, without the resolved toolchain data. */
46-
UnloadedToolchainContext withoutResolvedToolchains();
4744
}

‎src/main/java/com/google/devtools/build/lib/skyframe/UnloadedToolchainContextImpl.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,4 @@ public ImmutableSet<Label> resolvedToolchainLabels() {
7474
}
7575

7676
protected abstract Builder toBuilder();
77-
78-
@Override
79-
public UnloadedToolchainContext withoutResolvedToolchains() {
80-
return this.toBuilder().setToolchainTypeToResolved(ImmutableSetMultimap.of()).build();
81-
}
8277
}

‎src/test/java/com/google/devtools/build/lib/skyframe/ToolchainResolutionFunctionTest.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,4 +431,67 @@ public void resolve_noMatchingPlatform() throws Exception {
431431
.hasExceptionThat()
432432
.isInstanceOf(NoMatchingPlatformException.class);
433433
}
434+
435+
@Test
436+
public void resolve_forceExecutionPlatform() throws Exception {
437+
// This should select execution platform linux, toolchain extra_toolchain_linux, due to the
438+
// forced execution platform, even though execution platform mac is registered first.
439+
addToolchain(
440+
/* packageName= */ "extra",
441+
/* toolchainName= */ "extra_toolchain_linux",
442+
/* execConstraints= */ ImmutableList.of("//constraints:linux"),
443+
/* targetConstraints= */ ImmutableList.of("//constraints:linux"),
444+
/* data= */ "baz");
445+
addToolchain(
446+
/* packageName= */ "extra",
447+
/* toolchainName= */ "extra_toolchain_mac",
448+
/* execConstraints= */ ImmutableList.of("//constraints:mac"),
449+
/* targetConstraints= */ ImmutableList.of("//constraints:linux"),
450+
/* data= */ "baz");
451+
rewriteWorkspace(
452+
"register_toolchains('//extra:extra_toolchain_linux', '//extra:extra_toolchain_mac')",
453+
"register_execution_platforms('//platforms:mac', '//platforms:linux')");
454+
455+
useConfiguration("--platforms=//platforms:linux");
456+
ToolchainContextKey key =
457+
ToolchainContextKey.key()
458+
.configurationKey(targetConfigKey)
459+
.requiredToolchainTypeLabels(testToolchainTypeLabel)
460+
.forceExecutionPlatform(Label.parseAbsoluteUnchecked("//platforms:linux"))
461+
.build();
462+
463+
EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
464+
465+
assertThatEvaluationResult(result).hasNoError();
466+
UnloadedToolchainContext unloadedToolchainContext = result.get(key);
467+
assertThat(unloadedToolchainContext).isNotNull();
468+
469+
assertThat(unloadedToolchainContext).hasToolchainType(testToolchainTypeLabel);
470+
assertThat(unloadedToolchainContext).hasResolvedToolchain("//extra:extra_toolchain_linux_impl");
471+
assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:linux");
472+
assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
473+
}
474+
475+
@Test
476+
public void resolve_forceExecutionPlatform_noRequiredToolchains() throws Exception {
477+
// This should select execution platform linux, due to the forced execution platform, even
478+
// though execution platform mac is registered first.
479+
rewriteWorkspace("register_execution_platforms('//platforms:mac', '//platforms:linux')");
480+
481+
useConfiguration("--platforms=//platforms:linux");
482+
ToolchainContextKey key =
483+
ToolchainContextKey.key()
484+
.configurationKey(targetConfigKey)
485+
.forceExecutionPlatform(Label.parseAbsoluteUnchecked("//platforms:linux"))
486+
.build();
487+
488+
EvaluationResult<UnloadedToolchainContext> result = invokeToolchainResolution(key);
489+
490+
assertThatEvaluationResult(result).hasNoError();
491+
UnloadedToolchainContext unloadedToolchainContext = result.get(key);
492+
assertThat(unloadedToolchainContext).isNotNull();
493+
494+
assertThat(unloadedToolchainContext).hasExecutionPlatform("//platforms:linux");
495+
assertThat(unloadedToolchainContext).hasTargetPlatform("//platforms:linux");
496+
}
434497
}

‎src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java‎

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -451,43 +451,47 @@ public void execGroups_defaultAndNamed() throws Exception {
451451

452452
@Test
453453
public void keepParentToolchainContext() throws Exception {
454+
// Add some platforms and custom constraints.
454455
scratch.file(
455-
"extra/BUILD",
456-
"load('//toolchain:toolchain_def.bzl', 'test_toolchain')",
457-
"toolchain_type(name = 'extra_toolchain')",
458-
"toolchain(",
459-
" name = 'toolchain',",
460-
" toolchain_type = '//extra:extra_toolchain',",
461-
" exec_compatible_with = [],",
462-
" target_compatible_with = [],",
463-
" toolchain = ':toolchain_impl')",
464-
"test_toolchain(",
465-
" name='toolchain_impl',",
466-
" data = 'foo')");
456+
"platforms/BUILD",
457+
"constraint_setting(name = 'local_setting')",
458+
"constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')",
459+
"constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')",
460+
"platform(name = 'local_platform_a',",
461+
" constraint_values = [':local_value_a'],",
462+
")",
463+
"platform(name = 'local_platform_b',",
464+
" constraint_values = [':local_value_b'],",
465+
")");
466+
467+
// Test normal resolution, and with a per-target exec constraint.
467468
scratch.file("a/BUILD", "load('//toolchain:rule.bzl', 'my_rule')", "my_rule(name = 'a')");
468469

469-
useConfiguration("--extra_toolchains=//extra:toolchain");
470+
useConfiguration(
471+
"--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b");
472+
470473
ConfiguredTarget target = Iterables.getOnlyElement(update("//a").getTargetsToBuild());
474+
ToolchainContextKey parentKey =
475+
ToolchainContextKey.key()
476+
.configurationKey(target.getConfigurationKey())
477+
// Force the constraint label, to make the exec platform be local_platform_b.
478+
.execConstraintLabels(Label.parseAbsoluteUnchecked("//platforms:local_value_b"))
479+
.build();
471480
ToolchainCollection<UnloadedToolchainContext> toolchainCollection =
472481
getToolchainCollection(
473482
target,
474483
ConfiguredTargetKey.builder()
475484
.setLabel(target.getOriginalLabel())
476485
.setConfigurationKey(target.getConfigurationKey())
477-
.setToolchainContextKey(
478-
ToolchainContextKey.key()
479-
.configurationKey(target.getConfigurationKey())
480-
.requiredToolchainTypeLabels(
481-
Label.parseAbsoluteUnchecked("//extra:extra_toolchain"))
482-
.build())
486+
.setToolchainContextKey(parentKey)
483487
.build());
484488

485489
assertThat(toolchainCollection).isNotNull();
486490
assertThat(toolchainCollection).hasDefaultExecGroup();
491+
492+
// This should have the same exec platform as parentToolchainKey, which is local_platform_b.
487493
assertThat(toolchainCollection)
488494
.defaultToolchainContext()
489-
.hasToolchainType("//extra:extra_toolchain");
490-
// These should be cleared out.
491-
assertThat(toolchainCollection).defaultToolchainContext().resolvedToolchainLabels().isEmpty();
495+
.hasExecutionPlatform("//platforms:local_platform_b");
492496
}
493497
}

0 commit comments

Comments
 (0)