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/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
index 045500e..8e4f081 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -30,6 +30,7 @@
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.util.AnalysisCachingTestBase;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.rules.java.JavaInfo;
import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
import com.google.devtools.build.lib.syntax.StarlarkValue;
@@ -544,11 +545,14 @@
public static final OptionDefinition ALSO_IRRELEVANT_OPTION =
OptionsParser.getOptionDefinitionByName(DiffResetOptions.class, "also_irrelevant");
public static final PatchTransition CLEAR_IRRELEVANT =
- (options, eventHandler) -> {
- BuildOptions cloned = options.clone();
- cloned.get(DiffResetOptions.class).probablyIrrelevantOption = "(cleared)";
- cloned.get(DiffResetOptions.class).alsoIrrelevantOption = "(cleared)";
- return cloned;
+ new PatchTransition() {
+ @Override
+ public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
+ BuildOptions cloned = options.clone();
+ cloned.get(DiffResetOptions.class).probablyIrrelevantOption = "(cleared)";
+ cloned.get(DiffResetOptions.class).alsoIrrelevantOption = "(cleared)";
+ return cloned;
+ }
};
@Option(
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java b/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
index 77fdbda..b49af3d 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
@@ -37,6 +37,7 @@
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -266,15 +267,19 @@
new TransitionFactory<AttributeTransitionData>() {
@Override
public SplitTransition create(AttributeTransitionData data) {
- return (BuildOptions options, EventHandler eventHandler) -> {
- String define = data.attributes().get("define", STRING);
- BuildOptions newOptions = options.clone();
- CoreOptions optionsFragment = newOptions.get(CoreOptions.class);
- optionsFragment.commandLineBuildVariables =
- optionsFragment.commandLineBuildVariables.stream()
- .filter((pair) -> !pair.getKey().equals(define))
- .collect(toImmutableList());
- return ImmutableMap.of("define_cleaner", newOptions);
+ return new SplitTransition() {
+ @Override
+ public Map<String, BuildOptions> split(
+ BuildOptions options, EventHandler eventHandler) {
+ String define = data.attributes().get("define", STRING);
+ BuildOptions newOptions = options.clone();
+ CoreOptions optionsFragment = newOptions.get(CoreOptions.class);
+ optionsFragment.commandLineBuildVariables =
+ optionsFragment.commandLineBuildVariables.stream()
+ .filter((pair) -> !pair.getKey().equals(define))
+ .collect(toImmutableList());
+ return ImmutableMap.of("define_cleaner", newOptions);
+ }
};
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java
index e2f3cb6..150bdae 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
import com.google.devtools.build.lib.analysis.util.MockRule;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
import com.google.devtools.build.lib.testutil.Suite;
@@ -48,10 +49,13 @@
@RunWith(JUnit4.class)
public class ConfigurationsForLateBoundTargetsTest extends AnalysisTestCase {
private static final PatchTransition CHANGE_FOO_FLAG_TRANSITION =
- (options, eventHandler) -> {
- BuildOptions toOptions = options.clone();
- toOptions.get(LateBoundSplitUtil.TestOptions.class).fooFlag = "PATCHED!";
- return toOptions;
+ new PatchTransition() {
+ @Override
+ public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
+ BuildOptions toOptions = options.clone();
+ toOptions.get(LateBoundSplitUtil.TestOptions.class).fooFlag = "PATCHED!";
+ return toOptions;
+ }
};
/** Rule definition with a latebound dependency. */
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsViewTest.java
new file mode 100644
index 0000000..6f9a723
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsViewTest.java
@@ -0,0 +1,86 @@
+// 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 static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.rules.cpp.CppOptions;
+import com.google.devtools.common.options.OptionsParser;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link BuildOptionsView}. */
+@RunWith(JUnit4.class)
+public final class BuildOptionsViewTest {
+
+ private static final ImmutableList<Class<? extends FragmentOptions>> BUILD_CONFIG_OPTIONS =
+ ImmutableList.of(CoreOptions.class, CppOptions.class);
+
+ private BuildOptions options;
+
+ @Before
+ public void constructBuildOptions() {
+ options =
+ BuildOptions.of(
+ BUILD_CONFIG_OPTIONS,
+ OptionsParser.builder().optionsClasses(BUILD_CONFIG_OPTIONS).build());
+ }
+
+ @Test
+ public void allowedGet() throws Exception {
+ BuildOptionsView restrictedOptions =
+ new BuildOptionsView(options, ImmutableSet.of(CoreOptions.class));
+ assertThat(restrictedOptions.get(CoreOptions.class))
+ .isSameInstanceAs(options.get(CoreOptions.class));
+ ;
+ }
+
+ @Test
+ public void prohibitedGet() throws Exception {
+ BuildOptionsView restrictedOptions =
+ new BuildOptionsView(options, ImmutableSet.of(CoreOptions.class));
+ assertThrows(IllegalArgumentException.class, () -> restrictedOptions.get(CppOptions.class));
+ }
+
+ @Test
+ public void allowedContains() throws Exception {
+ BuildOptionsView restrictedOptions =
+ new BuildOptionsView(options, ImmutableSet.of(CoreOptions.class));
+ assertThat(restrictedOptions.contains(CoreOptions.class)).isTrue();
+ }
+
+ @Test
+ public void prohibitedContains() throws Exception {
+ BuildOptionsView restrictedOptions =
+ new BuildOptionsView(options, ImmutableSet.of(CoreOptions.class));
+ assertThrows(
+ IllegalArgumentException.class, () -> restrictedOptions.contains(CppOptions.class));
+ }
+
+ @Test
+ public void cloneTest() throws Exception {
+ BuildOptionsView restrictedOptions =
+ new BuildOptionsView(options, ImmutableSet.of(CoreOptions.class));
+ BuildOptionsView clone = restrictedOptions.clone();
+ assertThat(clone).isNotSameInstanceAs(restrictedOptions);
+ assertThat(restrictedOptions.underlying()).isSameInstanceAs(options);
+ assertThat(clone.underlying()).isNotSameInstanceAs(options);
+ assertThat(clone.underlying()).isEqualTo(options);
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java
index aee0d37..67dcf08 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/TransitionFactoriesTest.java
@@ -20,6 +20,8 @@
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.events.EventHandler;
+import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -59,8 +61,13 @@
public void splitTransition() {
TransitionFactory<Object> factory =
TransitionFactories.of(
- (SplitTransition)
- (buildOptions, eventHandler) -> ImmutableMap.of("test0", buildOptions.clone()));
+ new SplitTransition() {
+ @Override
+ public Map<String, BuildOptions> split(
+ BuildOptions buildOptions, EventHandler eventHandler) {
+ return ImmutableMap.of("test0", buildOptions.clone());
+ }
+ });
assertThat(factory).isNotNull();
assertThat(factory.isHost()).isFalse();
assertThat(factory.isSplit()).isTrue();
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java
index 85855b5..fb31d78 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java
@@ -58,7 +58,10 @@
assertThat(composed.isSplit()).isFalse();
ConfigurationTransition transition = composed.create(new StubData());
Collection<BuildOptions> results =
- transition.apply(BuildOptions.builder().build(), eventHandler).values();
+ transition
+ .apply(
+ TransitionUtil.restrict(transition, BuildOptions.builder().build()), eventHandler)
+ .values();
assertThat(results).isNotNull();
assertThat(results).hasSize(1);
BuildOptions result = Iterables.getOnlyElement(results);
@@ -78,7 +81,8 @@
assertThat(composed.isSplit()).isTrue();
ConfigurationTransition transition = composed.create(new StubData());
Map<String, BuildOptions> results =
- transition.apply(BuildOptions.builder().build(), eventHandler);
+ transition.apply(
+ TransitionUtil.restrict(transition, BuildOptions.builder().build()), eventHandler);
assertThat(results).isNotNull();
assertThat(results).hasSize(2);
@@ -105,7 +109,8 @@
assertThat(composed.isSplit()).isTrue();
ConfigurationTransition transition = composed.create(new StubData());
Map<String, BuildOptions> results =
- transition.apply(BuildOptions.builder().build(), eventHandler);
+ transition.apply(
+ TransitionUtil.restrict(transition, BuildOptions.builder().build()), eventHandler);
assertThat(results).isNotNull();
assertThat(results).hasSize(2);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java
index acc758e..a3375b3 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java
@@ -18,13 +18,19 @@
import static org.junit.Assert.assertThrows;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
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.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.HostTransition;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.StoredEventHandler;
+import com.google.devtools.build.lib.rules.cpp.CppOptions;
+import com.google.devtools.build.lib.rules.java.JavaOptions;
import java.util.Map;
+import java.util.Set;
import java.util.stream.IntStream;
import org.junit.Before;
import org.junit.Test;
@@ -52,7 +58,8 @@
assertThat(composed).isNotNull();
Map<String, BuildOptions> results =
- composed.apply(BuildOptions.builder().build(), eventHandler);
+ composed.apply(
+ TransitionUtil.restrict(composed, BuildOptions.builder().build()), eventHandler);
assertThat(results).isNotNull();
assertThat(results).hasSize(1);
BuildOptions result = Iterables.getOnlyElement(results.values());
@@ -69,7 +76,8 @@
assertThat(composed).isNotNull();
Map<String, BuildOptions> results =
- composed.apply(BuildOptions.builder().build(), eventHandler);
+ composed.apply(
+ TransitionUtil.restrict(composed, BuildOptions.builder().build()), eventHandler);
assertThat(results).isNotNull();
assertThat(results).hasSize(2);
@@ -93,7 +101,8 @@
assertThat(composed).isNotNull();
Map<String, BuildOptions> results =
- composed.apply(BuildOptions.builder().build(), eventHandler);
+ composed.apply(
+ TransitionUtil.restrict(composed, BuildOptions.builder().build()), eventHandler);
assertThat(results).isNotNull();
assertThat(results).hasSize(2);
@@ -119,7 +128,9 @@
assertThat(composed).isNotNull();
assertThrows(
IllegalStateException.class,
- () -> composed.apply(BuildOptions.builder().build(), eventHandler));
+ () ->
+ composed.apply(
+ TransitionUtil.restrict(composed, BuildOptions.builder().build()), eventHandler));
}
@Test
@@ -197,4 +208,32 @@
i -> updateOptions(options, flagLabel, flagValues.get(i))));
}
}
+
+ private static final class TransitionWithCustomFragments implements PatchTransition {
+ private final Set<Class<? extends FragmentOptions>> fragments;
+
+ TransitionWithCustomFragments(Set<Class<? extends FragmentOptions>> fragments) {
+ this.fragments = fragments;
+ }
+
+ @Override
+ public Set<Class<? extends FragmentOptions>> requiresOptionFragments() {
+ return fragments;
+ }
+
+ @Override
+ public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
+ return options.underlying();
+ }
+ }
+
+ @Test
+ public void composed_required_fragments() throws Exception {
+ ConfigurationTransition composed =
+ ComposingTransition.of(
+ new TransitionWithCustomFragments(ImmutableSet.of(CppOptions.class)),
+ new TransitionWithCustomFragments(ImmutableSet.of(JavaOptions.class)));
+ assertThat(composed.requiresOptionFragments())
+ .containsExactly(CppOptions.class, JavaOptions.class);
+ }
}
