Add validation that all targets used as platforms actually advertise
they provide PlatformInfo.
Fixes #10282.
Closes #10307.
PiperOrigin-RevId: 282766536
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
index 6c96440..c60fb08 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
@@ -16,18 +16,28 @@
import static com.google.common.base.Predicates.equalTo;
import static com.google.common.base.Predicates.not;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.devtools.build.lib.skyframe.RegisteredExecutionPlatformsFunction.hasPlatformInfo;
import static java.util.stream.Collectors.joining;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.PlatformConfiguration;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
+import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction.ConfiguredValueCreationException;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.ValueOrException;
import com.google.devtools.build.skyframe.ValueOrException3;
import java.util.HashMap;
import java.util.Map;
@@ -38,9 +48,16 @@
@Nullable
public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo(
- Iterable<ConfiguredTargetKey> platformKeys, Environment env, boolean sanityCheckConfiguration)
+ ImmutableList<ConfiguredTargetKey> platformKeys,
+ Environment env,
+ boolean sanityCheckConfiguration)
throws InterruptedException, InvalidPlatformException {
+ validatePlatformKeys(platformKeys, env);
+ if (env.valuesMissing()) {
+ return null;
+ }
+
Map<
SkyKey,
ValueOrException3<
@@ -66,6 +83,53 @@
return platforms;
}
+ /** Validate that all keys are for actual platform targets. */
+ private static void validatePlatformKeys(
+ ImmutableList<ConfiguredTargetKey> platformKeys, Environment env)
+ throws InterruptedException, InvalidPlatformException {
+ // Load the packages. This should already be in Skyframe and thus not require a restart.
+ ImmutableSet<PackageValue.Key> packageKeys =
+ platformKeys.stream()
+ .map(ConfiguredTargetKey::getLabel)
+ .map(Label::getPackageIdentifier)
+ .distinct()
+ .map(PackageValue::key)
+ .collect(toImmutableSet());
+
+ Map<SkyKey, ValueOrException<NoSuchPackageException>> values =
+ env.getValuesOrThrow(packageKeys, NoSuchPackageException.class);
+ boolean valuesMissing = env.valuesMissing();
+ Map<PackageIdentifier, Package> packages = valuesMissing ? null : new HashMap<>();
+ for (Map.Entry<SkyKey, ValueOrException<NoSuchPackageException>> value : values.entrySet()) {
+ try {
+ PackageValue packageValue = (PackageValue) value.getValue().get();
+ if (!valuesMissing && packageValue != null) {
+ packages.put(packageValue.getPackage().getPackageIdentifier(), packageValue.getPackage());
+ }
+ } catch (NoSuchPackageException e) {
+ throw new InvalidPlatformException(e);
+ }
+ }
+ if (valuesMissing) {
+ return;
+ }
+
+ // Now check each platform.
+ for (ConfiguredTargetKey platformKey : platformKeys) {
+ try {
+ Label platformLabel = platformKey.getLabel();
+ Target target =
+ packages.get(platformLabel.getPackageIdentifier()).getTarget(platformLabel.getName());
+ if (!hasPlatformInfo(target)) {
+ // validation failure
+ throw new InvalidPlatformException(platformLabel);
+ }
+ } catch (NoSuchTargetException e) {
+ throw new InvalidPlatformException(e);
+ }
+ }
+ }
+
/**
* Returns the {@link PlatformInfo} provider from the {@link ConfiguredTarget} in the {@link
* ValueOrException3}, or {@code null} if the {@link ConfiguredTarget} is not present. If the
@@ -120,7 +184,7 @@
} catch (ConfiguredValueCreationException e) {
throw new InvalidPlatformException(key.getLabel(), e);
} catch (NoSuchThingException e) {
- throw new InvalidPlatformException(key.getLabel(), e);
+ throw new InvalidPlatformException(e);
} catch (ActionConflictException e) {
throw new InvalidPlatformException(key.getLabel(), e);
}
@@ -138,7 +202,7 @@
super(formatError(label, DEFAULT_ERROR), e);
}
- public InvalidPlatformException(Label label, NoSuchThingException e) {
+ public InvalidPlatformException(NoSuchThingException e) {
// Just propagate the inner exception, because it's directly actionable.
super(e);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
index 26a1dd4..e154ee6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
@@ -225,6 +225,18 @@
}
}
+ static boolean hasPlatformInfo(Target target) {
+ // If the rule uses toolchain resolution, it can't be used as a target or exec platform.
+ RuleClass ruleClass = target.getAssociatedRule().getRuleClassObject();
+ if (ruleClass == null || ruleClass.useToolchainResolution()) {
+ return false;
+ }
+
+ return ruleClass.getAdvertisedProviders().advertises(PlatformInfo.class);
+ }
+
+ // This class uses AutoValue solely to get default equals/hashCode behavior, which is needed to
+ // make skyframe serialization work properly.
@AutoValue
@AutoCodec
abstract static class HasPlatformInfo extends FilteringPolicy {
@@ -234,14 +246,7 @@
if (explicit) {
return true;
}
-
- // If the rule requires platforms or toolchain resolution, it can't be used as a platform.
- RuleClass ruleClass = target.getAssociatedRule().getRuleClassObject();
- if (ruleClass == null || ruleClass.useToolchainResolution()) {
- return false;
- }
-
- return ruleClass.getAdvertisedProviders().advertises(PlatformInfo.class);
+ return hasPlatformInfo(target);
}
@AutoCodec.Instantiator
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
index 1ed9020..3e638be 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
@@ -138,9 +138,9 @@
return GET_PLATFORM_INFO_FUNCTION;
}
- abstract Iterable<ConfiguredTargetKey> platformKeys();
+ abstract ImmutableList<ConfiguredTargetKey> platformKeys();
- public static GetPlatformInfoKey create(Iterable<ConfiguredTargetKey> platformKeys) {
+ public static GetPlatformInfoKey create(ImmutableList<ConfiguredTargetKey> platformKeys) {
return new AutoValue_PlatformLookupUtilTest_GetPlatformInfoKey(platformKeys);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunctionTest.java
index ee4b18e..f2a5b62 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunctionTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.skyframe.EvaluationResultSubjectFactory.assertThatEvaluationResult;
+import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
@@ -40,6 +41,7 @@
import com.google.devtools.build.skyframe.EvaluationResult;
import com.google.devtools.build.skyframe.SkyKey;
import javax.annotation.Nullable;
+import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -56,11 +58,12 @@
@AutoCodec @AutoCodec.VisibleForSerialization
static final ConfiguredTargetKey MAC_CTKEY = Mockito.mock(ConfiguredTargetKey.class);
- static {
- Mockito.when(LINUX_CTKEY.functionName())
- .thenReturn(InjectedActionLookupKey.INJECTED_ACTION_LOOKUP);
- Mockito.when(MAC_CTKEY.functionName())
- .thenReturn(InjectedActionLookupKey.INJECTED_ACTION_LOOKUP);
+ @Before
+ public void setUpKeys() {
+ when(LINUX_CTKEY.functionName()).thenReturn(InjectedActionLookupKey.INJECTED_ACTION_LOOKUP);
+ when(LINUX_CTKEY.getLabel()).thenReturn(Label.parseAbsoluteUnchecked("//platforms:linux"));
+ when(MAC_CTKEY.functionName()).thenReturn(InjectedActionLookupKey.INJECTED_ACTION_LOOKUP);
+ when(MAC_CTKEY.getLabel()).thenReturn(Label.parseAbsoluteUnchecked("//platforms:mac"));
}
private static ConfiguredTargetValue createConfiguredTargetValue(
diff --git a/src/test/shell/bazel/toolchain_test.sh b/src/test/shell/bazel/toolchain_test.sh
index e29a454..4df2123 100755
--- a/src/test/shell/bazel/toolchain_test.sh
+++ b/src/test/shell/bazel/toolchain_test.sh
@@ -1375,6 +1375,36 @@
expect_log "Misconfigured toolchains: //:upper_toolchain is declared as a toolchain but has inappropriate dependencies"
}
+
+# Catch the error when a target platform requires a configuration which contains the same target platform.
+# This can only happen when the target platform is not actually a platform.
+function test_target_platform_cycle() {
+ cat >> hello.sh <<EOF
+ #!/bin/sh
+ echo "Hello world"
+EOF
+ cat >> target.sh <<EOF
+ #!/bin/sh
+ echo "Hello target"
+EOF
+ chmod +x hello.sh
+ chmod +x target.sh
+ cat >> BUILD <<EOF
+sh_binary(
+ name = "hello",
+ srcs = ["hello.sh"],
+)
+sh_binary(
+ name = "target",
+ srcs = ["target.sh"],
+)
+EOF
+
+ bazel build --platforms=//:hello //:target &> $TEST_log && fail "Build succeeded unexpectedly"
+ expect_log "While resolving toolchains for target //:target: Target //:hello was referenced as a platform, but does not provide PlatformInfo"
+}
+
+
function test_platform_duplicate_constraint_error() {
# Write a platform with duplicate constraint values for the same setting.
mkdir -p platform
