Convert remaining native uses of patch() / split().

In service of #11258. PiperOrigin-RevId: 315986864
bazelbuild · Jun 11, 2020 · a048f0f · a048f0f
1 parent ea5ec54
commit a048f0f
Show file tree

Hide file tree

Showing 22 changed files with 237 additions and 78 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/HostTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/HostTransition.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.analysis.config;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
 import com.google.devtools.build.lib.events.EventHandler;
@@ -32,12 +33,18 @@ public boolean isHostTransition() {
   }
 
   @Override
-  public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
+  public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
+    return ImmutableSet.of(CoreOptions.class);
+  }
+
+  @Override
+  public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
     if (options.get(CoreOptions.class).isHost) {
       // If the input already comes from the host configuration, just return the existing values.
       //
       // We don't do this just for convenience: if an
-      // {@link com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy}
+      // {@link
+      // com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy}
       // overrides option defaults, {@link FragmentOptions#getHost} won't honor that policy. That's
       // because it uses its own options parser that's not aware of the policy. This can create
       // problems for, e.g., {@link JavaOptions#getHost}, which promotes --host_foo flags to
@@ -48,9 +55,9 @@ public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
       // manually set host.hostFoo = original.hostFoo). But those raise larger questions about the
       // nature of host/target relationships, so for the time being this is a straightforward
       // and practical fix.
-      return options.clone();
+      return options.clone().underlying();
     } else {
-      return options.createHostOptions();
+      return options.underlying().createHostOptions();
     }
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTrimmingTransitionFactory.java
@@ -15,6 +15,8 @@
 
 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.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
@@ -41,17 +43,24 @@ public enum TestTrimmingTransition implements PatchTransition {
     INSTANCE;
 
     @Override
-    public BuildOptions patch(BuildOptions originalOptions, EventHandler eventHandler) {
+    public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
+      return ImmutableSet.of(TestOptions.class);
+    }
+
+    @Override
+    public BuildOptions patch(BuildOptionsView originalOptions, EventHandler eventHandler) {
       if (!originalOptions.contains(TestOptions.class)) {
         // nothing to do, already trimmed this fragment
-        return originalOptions;
+        return originalOptions.underlying();
       }
       TestOptions originalTestOptions = originalOptions.get(TestOptions.class);
       if (!originalTestOptions.trimTestConfiguration) {
         // nothing to do, trimming is disabled
-        return originalOptions;
+        return originalOptions.underlying();
       }
-      return originalOptions.toBuilder().removeFragmentOptions(TestOptions.class).build();
+      return originalOptions.underlying().toBuilder()
+          .removeFragmentOptions(TestOptions.class)
+          .build();
     }
   }
 

diff --git a/...gle/devtools/build/lib/rules/config/ConfigFeatureFlagTaggedTrimmingTransitionFactory.java b/...gle/devtools/build/lib/rules/config/ConfigFeatureFlagTaggedTrimmingTransitionFactory.java
@@ -17,10 +17,13 @@
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_KEYED_STRING_DICT;
 import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL_LIST;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Ordering;
 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.CoreOptions;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -49,14 +52,19 @@ public static final class ConfigFeatureFlagTaggedTrimmingTransition implements P
     }
 
     @Override
-    public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
+    public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
+      return ImmutableSet.of(ConfigFeatureFlagOptions.class, CoreOptions.class);
+    }
+
+    @Override
+    public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
       if (!(options.contains(ConfigFeatureFlagOptions.class)
           && options.get(ConfigFeatureFlagOptions.class)
               .enforceTransitiveConfigsForConfigFeatureFlag
           && options.get(CoreOptions.class).useDistinctHostConfiguration)) {
-        return options;
+        return options.underlying();
       }
-      return FeatureFlagValue.trimFlagValues(options, flags);
+      return FeatureFlagValue.trimFlagValues(options.underlying(), flags);
     }
 
     @Override

diff --git a/...n/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTransitionFactory.java b/...n/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTransitionFactory.java
@@ -16,8 +16,11 @@
 
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_KEYED_STRING_DICT;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 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.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
@@ -56,11 +59,16 @@ public ConfigFeatureFlagValuesTransition(Map<Label, String> flagValues) {
     }
 
     @Override
-    public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
+    public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
+      return ImmutableSet.of(ConfigFeatureFlagOptions.class);
+    }
+
+    @Override
+    public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
       if (!options.contains(ConfigFeatureFlagOptions.class)) {
-        return options;
+        return options.underlying();
       }
-      return FeatureFlagValue.replaceFlagValues(options, flagValues);
+      return FeatureFlagValue.replaceFlagValues(options.underlying(), flagValues);
     }
 
     @Override

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -245,6 +245,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/null_transition",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:configurations_collector",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_object_value",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -46,10 +46,12 @@
 import com.google.devtools.build.lib.analysis.ToolchainContext;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 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.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.ConfigurationResolver;
 import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
 import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition.TransitionException;
 import com.google.devtools.build.lib.causes.AnalysisFailedCause;
@@ -488,10 +490,14 @@ static ToolchainCollection<UnloadedToolchainContext> computeUnloadedToolchainCon
     //         ...
     //
     // None of this has any effect on rules that don't utilize manual trimming.
+    PatchTransition toolchainTaggedTrimmingTransition =
+        ((ConfiguredRuleClassProvider) ruleClassProvider).getToolchainTaggedTrimmingTransition();
     BuildOptions toolchainOptions =
-        ((ConfiguredRuleClassProvider) ruleClassProvider)
-            .getToolchainTaggedTrimmingTransition()
-            .patch(configuration.getOptions(), env.getListener());
+        toolchainTaggedTrimmingTransition.patch(
+            new BuildOptionsView(
+                configuration.getOptions(),
+                toolchainTaggedTrimmingTransition.requiresOptionFragments()),
+            env.getListener());
 
     BuildConfigurationValue.Key toolchainConfig =
         BuildConfigurationValue.keyWithoutPlatformMapping(

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareAnalysisPhaseFunction.java
@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 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.ConfigurationResolver;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.Fragment;
@@ -79,9 +80,11 @@ public PrepareAnalysisPhaseValue compute(SkyKey key, Environment env)
     PrepareAnalysisPhaseKey options = (PrepareAnalysisPhaseKey) key.argument();
 
     BuildOptions targetOptions = defaultBuildOptions.applyDiff(options.getOptionsDiff());
+    BuildOptionsView hostTransitionOptionsView =
+        new BuildOptionsView(targetOptions, HostTransition.INSTANCE.requiresOptionFragments());
     BuildOptions hostOptions =
         targetOptions.get(CoreOptions.class).useDistinctHostConfiguration
-            ? HostTransition.INSTANCE.patch(targetOptions, env.getListener())
+            ? HostTransition.INSTANCE.patch(hostTransitionOptionsView, env.getListener())
             : targetOptions;
 
     ImmutableSortedSet<Class<? extends Fragment>> allFragments =

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -82,6 +82,7 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiffForReconstruction;
+import com.google.devtools.build.lib.analysis.config.BuildOptionsView;
 import com.google.devtools.build.lib.analysis.config.ConfigurationResolver;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.Fragment;
@@ -1511,9 +1512,12 @@ public BuildConfigurationCollection createConfigurations(
     BuildConfiguration firstTargetConfig = topLevelTargetConfigs.get(0);
 
     BuildOptions targetOptions = firstTargetConfig.getOptions();
+    BuildOptionsView hostTransitionOptionsView =
+        new BuildOptionsView(
+            firstTargetConfig.getOptions(), HostTransition.INSTANCE.requiresOptionFragments());
     BuildOptions hostOptions =
         targetOptions.get(CoreOptions.class).useDistinctHostConfiguration
-            ? HostTransition.INSTANCE.patch(targetOptions, eventHandler)
+            ? HostTransition.INSTANCE.patch(hostTransitionOptionsView, eventHandler)
             : targetOptions;
     BuildConfiguration hostConfig = getConfiguration(eventHandler, hostOptions, keepGoing);
 

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
@@ -23,6 +23,7 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Action;
 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.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.Fragment;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
@@ -547,11 +548,16 @@ public static final class DiffResetOptions extends FragmentOptions {
     public static final PatchTransition CLEAR_IRRELEVANT =
         new PatchTransition() {
           @Override
-          public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
-            BuildOptions cloned = options.clone();
+          public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
+            return ImmutableSet.of(DiffResetOptions.class);
+          }
+
+          @Override
+          public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
+            BuildOptionsView cloned = options.clone();
             cloned.get(DiffResetOptions.class).probablyIrrelevantOption = "(cleared)";
             cloned.get(DiffResetOptions.class).alsoIrrelevantOption = "(cleared)";
-            return cloned;
+            return cloned.underlying();
           }
         };
 

diff --git a/...st/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java b/...st/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java
@@ -19,8 +19,11 @@
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 
 import com.google.common.collect.ImmutableMap;
+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.TransitionFactories;
 import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
 import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
@@ -51,10 +54,15 @@ public class ConfigurationsForLateBoundTargetsTest extends AnalysisTestCase {
   private static final PatchTransition CHANGE_FOO_FLAG_TRANSITION =
       new PatchTransition() {
         @Override
-        public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
-          BuildOptions toOptions = options.clone();
+        public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
+          return ImmutableSet.of(LateBoundSplitUtil.TestOptions.class);
+        }
+
+        @Override
+        public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
+          BuildOptionsView toOptions = options.clone();
           toOptions.get(LateBoundSplitUtil.TestOptions.class).fooFlag = "PATCHED!";
-          return toOptions;
+          return toOptions.underlying();
         }
       };
 

diff --git a/...google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java b/...google/devtools/build/lib/analysis/config/transitions/ComposingTransitionFactoryTest.java
@@ -18,8 +18,10 @@
 import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 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.HostTransition;
 import com.google.devtools.build.lib.analysis.config.TransitionFactories;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -217,8 +219,8 @@ private static final class StubPatch implements PatchTransition {
     }
 
     @Override
-    public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
-      return updateOptions(options, flagLabel, flagValue);
+    public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
+      return updateOptions(options.underlying(), flagLabel, flagValue);
     }
   }
 
@@ -232,13 +234,14 @@ private static final class StubSplit implements SplitTransition {
     }
 
     @Override
-    public Map<String, BuildOptions> split(BuildOptions options, EventHandler eventHandler) {
+    public ImmutableMap<String, BuildOptions> split(
+        BuildOptionsView options, EventHandler eventHandler) {
       return IntStream.range(0, flagValues.size())
           .boxed()
           .collect(
               toImmutableMap(
                   i -> "stub_split" + i,
-                  i -> updateOptions(options, flagLabel, flagValues.get(i))));
+                  i -> updateOptions(options.underlying(), flagLabel, flagValues.get(i))));
     }
   }
 }
diff --git a/...va/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java b/...va/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java
@@ -18,6 +18,7 @@
 import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -183,8 +184,8 @@ private static final class StubPatch implements PatchTransition {
     }
 
     @Override
-    public BuildOptions patch(BuildOptions options, EventHandler eventHandler) {
-      return updateOptions(options, flagLabel, flagValue);
+    public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) {
+      return updateOptions(options.underlying(), flagLabel, flagValue);
     }
   }
 
@@ -198,13 +199,14 @@ private static final class StubSplit implements SplitTransition {
     }
 
     @Override
-    public Map<String, BuildOptions> split(BuildOptions options, EventHandler eventHandler) {
+    public ImmutableMap<String, BuildOptions> split(
+        BuildOptionsView options, EventHandler eventHandler) {
       return IntStream.range(0, flagValues.size())
           .boxed()
           .collect(
               toImmutableMap(
                   i -> "stub_split" + i,
-                  i -> updateOptions(options, flagLabel, flagValues.get(i))));
+                  i -> updateOptions(options.underlying(), flagLabel, flagValues.get(i))));
     }
   }
 

diff --git a/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java b/src/test/java/com/google/devtools/build/lib/packages/AttributeTest.java
@@ -25,6 +25,7 @@
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableMap;
 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.HostTransition;
 import com.google.devtools.build.lib.analysis.config.TransitionFactories;
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
@@ -287,8 +288,10 @@ public void testHostTransition() throws Exception {
 
   private static class TestSplitTransition implements SplitTransition {
     @Override
-    public Map<String, BuildOptions> split(BuildOptions buildOptions, EventHandler eventHandler) {
-      return ImmutableMap.of("test0", buildOptions.clone(), "test1", buildOptions.clone());
+    public Map<String, BuildOptions> split(
+        BuildOptionsView buildOptions, EventHandler eventHandler) {
+      return ImmutableMap.of(
+          "test0", buildOptions.clone().underlying(), "test1", buildOptions.clone().underlying());
     }
   }
 

diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD b/src/test/java/com/google/devtools/build/lib/query2/cquery/BUILD
@@ -89,6 +89,7 @@ java_library(
         ":configured_target_query_helper",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/split_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",