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) {