Introduce configuration transition fragment declarations.
See https://github.com/bazelbuild/bazel/issues/11258 for context.
This essentially converts ConfigurationTransition.apply(BuildOptions...)
to ConfigurationTransition.apply(RestrictedBuildOptions...). The new
BuildOptionsView wraps BuildOptions while only allowing access
to options fragments the transitions declare.
Because there are a *lot* of uses of PatchTransition and SplitTransition,
this change only adds a parallel interface to PatchTransition.patch and
SplitTransition.split. Implementers continue to work as-is with the
BuildOptions variant, while we can incrementally switch them over to
the RestrictedBuildOptions variant at whatever pace we desire.
This helps decouple risk of failing CI if/when we accidentally forget
to migrate a few implementers or get their set of declared fragments wrong.
This implements step 1 of https://github.com/bazelbuild/bazel/issues/11258.
PiperOrigin-RevId: 312311815
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index d22f09e..0fa4dad 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -1468,7 +1468,10 @@
java_library(
name = "config/build_options",
- srcs = ["config/BuildOptions.java"],
+ srcs = [
+ "config/BuildOptions.java",
+ "config/BuildOptionsView.java",
+ ],
deps = [
":config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/cmdline",
@@ -1625,11 +1628,13 @@
deps = [
":config/build_options",
":config/core_options",
+ ":config/fragment_options",
":config/transitions/patch_transition",
":config/transitions/transition_factory",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//third_party:auto_value",
+ "//third_party:guava",
],
)
@@ -1720,6 +1725,7 @@
srcs = ["config/transitions/ComposingTransition.java"],
deps = [
":config/build_options",
+ ":config/fragment_options",
":config/transitions/configuration_transition",
":config/transitions/no_transition",
":config/transitions/null_transition",
@@ -1745,10 +1751,15 @@
java_library(
name = "config/transitions/configuration_transition",
- srcs = ["config/transitions/ConfigurationTransition.java"],
+ srcs = [
+ "config/transitions/ConfigurationTransition.java",
+ "config/transitions/TransitionUtil.java",
+ ],
deps = [
":config/build_options",
+ ":config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/events",
+ "//third_party:guava",
],
)
@@ -2190,6 +2201,7 @@
srcs = ["test/TestTrimmingTransitionFactory.java"],
deps = [
":config/build_options",
+ ":config/fragment_options",
":config/transitions/no_transition",
":config/transitions/patch_transition",
":config/transitions/transition_factory",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptionsView.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptionsView.java
new file mode 100644
index 0000000..49551ad
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptionsView.java
@@ -0,0 +1,83 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.analysis.config;
+
+import com.google.common.base.Preconditions;
+import java.util.Set;
+
+/**
+ * Wrapper for {@link BuildOptions} that only permits {@link BuildOptions#get} calls to {@link
+ * FragmentOptions} from a pre-declared set.
+ *
+ * <p>This lets Blaze understand what fragments transitions require, which is helpful for
+ * understanding what flags are and are not important to a given build.
+ */
+public class BuildOptionsView implements Cloneable {
+ private final BuildOptions options;
+ private final Set<Class<? extends FragmentOptions>> allowedFragments;
+
+ /** Wraps a given {@link BuildOptions} with a "permitted" set of {@link FragmentOptions}. */
+ public BuildOptionsView(
+ BuildOptions options, Set<Class<? extends FragmentOptions>> allowedFragments) {
+ this.options = options;
+ this.allowedFragments = allowedFragments;
+ }
+
+ /**
+ * Wrapper for {@link BuildOptions#get} that throws an {@link IllegalArgumentException} if the
+ * given {@link FragmentOptions} isn't in the "permitted" set.
+ */
+ public <T extends FragmentOptions> T get(Class<T> optionsClass) {
+ return options.get(checkFragment(optionsClass));
+ }
+
+ /**
+ * Wrapper for {@link BuildOptions#contains} that throws an {@link IllegalArgumentException} if
+ * the given {@link FragmentOptions} isn't in the "permitted" set.
+ */
+ public boolean contains(Class<? extends FragmentOptions> optionsClass) {
+ return options.contains(checkFragment(optionsClass));
+ }
+
+ /**
+ * Returns a new {@link BuildOptionsView} instance bound to a clone of the original's {@link
+ * BuildOptions}.
+ */
+ @Override
+ public BuildOptionsView clone() {
+ return new BuildOptionsView(options.clone(), allowedFragments);
+ }
+
+ /**
+ * Returns the underlying {@link BuildOptions}.
+ *
+ * <p>Since this sheds all extra security from {@link BuildOptionsView}, this should only be used
+ * when a transition is returning its final result.
+ *
+ * <p>!!! No transition should call any {@link BuildOptions} accessor after this! !!!
+ */
+ public BuildOptions underlying() {
+ return options;
+ }
+
+ private <T extends FragmentOptions> Class<T> checkFragment(Class<T> optionsClass) {
+ Preconditions.checkArgument(
+ allowedFragments.contains(optionsClass),
+ "Can't access %s in allowed fragments %s",
+ optionsClass,
+ allowedFragments);
+ return optionsClass;
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index c141859..8fa0701 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -37,6 +37,7 @@
import com.google.devtools.build.lib.analysis.config.transitions.NullTransition;
import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
+import com.google.devtools.build.lib.analysis.config.transitions.TransitionUtil;
import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition;
import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition.TransitionException;
import com.google.devtools.build.lib.cmdline.Label;
@@ -602,7 +603,8 @@
// TODO(bazel-team): Add safety-check that this never mutates fromOptions.
StoredEventHandler handlerWithErrorStatus = new StoredEventHandler();
- Map<String, BuildOptions> result = transition.apply(fromOptions, handlerWithErrorStatus);
+ Map<String, BuildOptions> result =
+ transition.apply(TransitionUtil.restrict(transition, fromOptions), handlerWithErrorStatus);
if (doesStarlarkTransition) {
// We use a temporary StoredEventHandler instead of the caller's event handler because
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
index f1c159a..6bbb5eb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
@@ -16,11 +16,15 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
/**
* A configuration transition that composes two other transitions in an ordered sequence.
@@ -49,12 +53,23 @@
}
@Override
- public Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler) {
+ public Set<Class<? extends FragmentOptions>> requiresOptionFragments() {
+ return ImmutableSet.<Class<? extends FragmentOptions>>builder()
+ .addAll(transition1.requiresOptionFragments())
+ .addAll(transition2.requiresOptionFragments())
+ .build();
+ }
+
+ @Override
+ public Map<String, BuildOptions> apply(BuildOptionsView buildOptions, EventHandler eventHandler) {
ImmutableMap.Builder<String, BuildOptions> toOptions = ImmutableMap.builder();
- for (Map.Entry<String, BuildOptions> entry1 :
- transition1.apply(buildOptions, eventHandler).entrySet()) {
- for (Map.Entry<String, BuildOptions> entry2 :
- transition2.apply(entry1.getValue(), eventHandler).entrySet()) {
+ Map<String, BuildOptions> transition1Output =
+ transition1.apply(
+ TransitionUtil.restrict(transition1, buildOptions.underlying()), eventHandler);
+ for (Map.Entry<String, BuildOptions> entry1 : transition1Output.entrySet()) {
+ Map<String, BuildOptions> transition2Output =
+ transition2.apply(TransitionUtil.restrict(transition2, entry1.getValue()), eventHandler);
+ for (Map.Entry<String, BuildOptions> entry2 : transition2Output.entrySet()) {
toOptions.put(composeKeys(entry1.getKey(), entry2.getKey()), entry2.getValue());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
index fc3b961..b9783ba 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
@@ -14,9 +14,13 @@
package com.google.devtools.build.lib.analysis.config.transitions;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.events.EventHandler;
import java.util.Map;
+import java.util.Set;
/**
* A configuration transition.
@@ -29,13 +33,26 @@
String PATCH_TRANSITION_KEY = "";
/**
+ * Declares the {@link FragmentOptions} this transition may read.
+ *
+ * <p>Blaze throws an {@link IllegalArgumentException} if {@link #apply} is called on an options
+ * fragment that isn't declared here.
+ */
+ default Set<Class<? extends FragmentOptions>> requiresOptionFragments() {
+ return ImmutableSet.of();
+ }
+
+ /**
* Returns the map of {@code BuildOptions} after applying this transition. The returned map keys
* are only used for dealing with split transitions. Patch transitions, including internal, native
* Patch transitions, should return a single entry map with key {@code PATCH_TRANSITION_KEY}.
*
+ * <p>Blaze throws an {@link IllegalArgumentException} if this method reads any options fragment
+ * not declared in {@link #requiresOptionFragments}.
+ *
* <p>Returning an empty or null map triggers a {@link RuntimeException}.
*/
- Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler);
+ Map<String, BuildOptions> apply(BuildOptionsView buildOptions, EventHandler eventHandler);
/**
* We want to keep the number of transition interfaces no larger than what's necessary to maintain
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java
index 16f95a8..b72329a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/PatchTransition.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.analysis.config.transitions;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.events.EventHandler;
import java.util.Collections;
import java.util.Map;
@@ -25,13 +26,12 @@
* <p>Also see {@link SplitTransition}, which maps a single input {@link BuildOptions} to possibly
* multiple {@link BuildOptions}.
*
- * <p>The concept is simple: given the input configuration's build options, the
- * transition does whatever it wants to them and returns the modified result.
+ * <p>The concept is simple: given the input configuration's build options, the transition does
+ * whatever it wants to them and returns the modified result.
*
- * <p>Implementations must be stateless: the output must exclusively depend on the
- * input build options and any immutable member fields. Implementations must also override
- * {@link Object#equals} and {@link Object#hashCode} unless exclusively accessed as
- * singletons. For example:
+ * <p>Implementations must be stateless: the output must exclusively depend on the input build
+ * options and any immutable member fields. Implementations must also override {@link Object#equals}
+ * and {@link Object#hashCode} unless exclusively accessed as singletons. For example:
*
* <pre>
* public class MyTransition implements PatchTransition {
@@ -40,7 +40,7 @@
* private MyTransition() {}
*
* {@literal @}Override
- * public BuildOptions patch(BuildOptions options) {
+ * public BuildOptions patch(RestrictedBuildOptions options) {
* BuildOptions toOptions = options.clone();
* // Change some setting on toOptions
* return toOptions;
@@ -49,26 +49,48 @@
* </pre>
*
* <p>For performance reasons, the input options are passed as a <i>reference</i>, not a
- * <i>copy</i>. Implementations should <i>always</i> treat these as immutable, and call
- * {@link com.google.devtools.build.lib.analysis.config.BuildOptions#clone}
- * before making changes. Unfortunately,
- * {@link com.google.devtools.build.lib.analysis.config.BuildOptions} doesn't currently
- * enforce immutability. So care must be taken not to modify the wrong instance.
+ * <i>copy</i>. Implementations should <i>always</i> treat these as immutable, and call {@link
+ * com.google.devtools.build.lib.analysis.config.BuildOptions#clone} before making changes.
+ * Unfortunately, {@link com.google.devtools.build.lib.analysis.config.BuildOptions} doesn't
+ * currently enforce immutability. So care must be taken not to modify the wrong instance.
*/
-@FunctionalInterface
public interface PatchTransition extends ConfigurationTransition {
/**
* Applies the transition.
*
+ * <p>This method is being deprecated (https://github.com/bazelbuild/bazel/issues/11258). Please
+ * use {@link #patch(BuildOptionsView, EventHandler)} for new uses.
+ *
* @param options the options representing the input configuration to this transition. <b>DO NOT
* MODIFY THIS VARIABLE WITHOUT CLONING IT FIRST!</b>
* @param eventHandler
* @return the options representing the desired post-transition configuration
*/
- BuildOptions patch(BuildOptions options, EventHandler eventHandler);
+ default BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
+ throw new UnsupportedOperationException(
+ "Either this or patch(RestrictedBuildOptions) must be overriden");
+ }
+
+ /**
+ * Applies the transition.
+ *
+ * <p>Blaze throws an {@link IllegalArgumentException} if this method reads any options fragment
+ * not declard in {@link ConfigurationTransition#requiresOptionFragments}.
+ *
+ * @param options the options representing the input configuration to this transition. <b>DO NOT
+ * MODIFY THIS VARIABLE WITHOUT CLONING IT FIRST!</b>
+ * @param eventHandler
+ * @return the options representing the desired post-transition configuration
+ */
+ default BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
+ // Escape hatch for implementers of the BuildOptions method: provide uninhibited access. When
+ // all implementers use this variation we'll remove this default implementation.
+ return patch(options.underlying(), eventHandler);
+ }
@Override
- default Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler) {
+ default Map<String, BuildOptions> apply(
+ BuildOptionsView buildOptions, EventHandler eventHandler) {
return Collections.singletonMap(PATCH_TRANSITION_KEY, patch(buildOptions, eventHandler));
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
index 5e630d2..5ec519e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/SplitTransition.java
@@ -17,6 +17,7 @@
import com.google.common.base.Verify;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.events.EventHandler;
import java.util.Collection;
@@ -35,16 +36,38 @@
* way (e.g. for determining which CPU corresponds to which dep for a multi-arch split dependency).
*/
@ThreadSafety.Immutable
-@FunctionalInterface
public interface SplitTransition extends ConfigurationTransition {
/**
* Returns the map of {@code BuildOptions} after splitting, or the original options if this split
* is a noop. The key values are used as dict keys in ctx.split_attr, so human-readable strings
* are recommended.
*
+ * <p>This method is being deprecated (https://github.com/bazelbuild/bazel/issues/11258). Please
+ * use {@link #split(BuildOptionsView, EventHandler)} for new uses.
+ *
* <p>Returning an empty or null list triggers a {@link RuntimeException}.
*/
- Map<String, BuildOptions> split(BuildOptions buildOptions, EventHandler eventHandler);
+ default Map<String, BuildOptions> split(BuildOptions buildOptions, EventHandler eventHandler) {
+ throw new UnsupportedOperationException(
+ "Either this or patch(RestrictedBuildOptions) must be overriden");
+ }
+
+ /**
+ * Returns the map of {@code BuildOptions} after splitting, or the original options if this split
+ * is a noop. The key values are used as dict keys in ctx.split_attr, so human-readable strings
+ * are recommended.
+ *
+ * <p>Blaze throws an {@link IllegalArgumentException} if this method reads any options fragment
+ * not declard in {@link ConfigurationTransition#requiresOptionFragments}.
+ *
+ * <p>Returning an empty or null list triggers a {@link RuntimeException}.
+ */
+ default Map<String, BuildOptions> split(
+ BuildOptionsView buildOptions, EventHandler eventHandler) {
+ // Escape hatch for implementers of the BuildOptions method: provide uninhibited access. When
+ // all implementers use this variation we'll remove this default implementation.
+ return split(buildOptions.underlying(), eventHandler);
+ }
/**
* Returns true iff {@code option} and {@code splitOptions} are equal.
@@ -56,7 +79,8 @@
}
@Override
- default Map<String, BuildOptions> apply(BuildOptions buildOptions, EventHandler eventHandler) {
+ default Map<String, BuildOptions> apply(
+ BuildOptionsView buildOptions, EventHandler eventHandler) {
Map<String, BuildOptions> splitOptions = split(buildOptions, eventHandler);
Verify.verifyNotNull(splitOptions, "Split transition output may not be null");
Verify.verify(!splitOptions.isEmpty(), "Split transition output may not be empty");
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionUtil.java
new file mode 100644
index 0000000..60a20ea
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionUtil.java
@@ -0,0 +1,31 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.analysis.config.transitions;
+
+import com.google.devtools.build.lib.analysis.config.BuildOptions;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
+
+/** Utility methods for configuration transitions. */
+public class TransitionUtil {
+ private TransitionUtil() {}
+
+ /**
+ * Returns a {@link BuildOptionsView} bound to the fragment options a {@link
+ * ConfigurationTransition} reads.
+ */
+ public static BuildOptionsView restrict(
+ ConfigurationTransition transition, BuildOptions options) {
+ return new BuildOptionsView(options, transition.requiresOptionFragments());
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java
index 9991a1d..fa20d77 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/cquery/TransitionsOutputFormatterCallback.java
@@ -31,6 +31,7 @@
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NullTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
+import com.google.devtools.build.lib.analysis.config.transitions.TransitionUtil;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.causes.Cause;
import com.google.devtools.build.lib.cmdline.Label;
@@ -146,7 +147,9 @@
// Skyframe loading to get flag default values. See ConfigurationResolver.applyTransition
// for an example of the required logic.
Collection<BuildOptions> toOptions =
- dep.getTransition().apply(fromOptions, eventHandler).values();
+ dep.getTransition()
+ .apply(TransitionUtil.restrict(dep.getTransition(), fromOptions), eventHandler)
+ .values();
String hostConfigurationChecksum = hostConfiguration.checksum();
String dependencyName;
if (attributeAndDep.getKey() == DependencyKind.TOOLCHAIN_DEPENDENCY) {
