Make print() work from starlark transition implementation functions.

Give each starlark transition a stored event handler. After starlark transitions are applied (after top level transitions and inside configuration resolver), replay and then clear events. Declare a new abstract class for starlark transitions to consolidate fields/behavior. Add behavior to ComposingTransitions to allow decomposition. This is helpful here but will also be helpful in future work where we'll need to post-process starlark transitions after they've been applied (e.g. output type verification). bazelbuild#5574 RELNOTES: None. PiperOrigin-RevId: 229417148
weixiao-huang · Jan 31, 2019 · 4b7680b · 4b7680b
1 parent d1f482d
commit 4b7680b
Show file tree

Hide file tree

Showing 12 changed files with 294 additions and 22 deletions.
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
@@ -27,8 +27,10 @@
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.Dependency;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
+import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
+import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.events.Event;
@@ -217,6 +219,8 @@ public static OrderedSetMultimap<Attribute, Dependency> resolveConfigurations(
         transitionsMap.put(transitionKey, toOptions);
       }
 
+      postProcessStarlarkTransitions(env, transition);
+
       // If the transition doesn't change the configuration, trivially re-use the original
       // configuration.
       if (sameFragments && toOptions.size() == 1
@@ -296,6 +300,16 @@ public static OrderedSetMultimap<Attribute, Dependency> resolveConfigurations(
     return sortResolvedDeps(originalDeps, resolvedDeps, attributesAndLabels);
   }
 
+  private static void postProcessStarlarkTransitions(
+      SkyFunction.Environment env, ConfigurationTransition transition) {
+    ImmutableList<ConfigurationTransition> transitions =
+        ComposingTransition.decomposeTransition(transition);
+    ExtendedEventHandler eventHandler = env.getListener();
+    transitions.stream()
+        .filter(t -> t instanceof StarlarkTransition)
+        .forEach(t -> ((StarlarkTransition) t).replayOn(eventHandler));
+  }
+
   /**
    * Encapsulates a set of config fragments and a config transition. This can be used to determine
    * the exact build options needed to set a configuration.
@@ -322,6 +336,9 @@ public boolean equals(Object o) {
       } else if (o == null) {
         return false;
       } else {
+        if (!(o instanceof FragmentsAndTransition)) {
+          return false;
+        }
         FragmentsAndTransition other = (FragmentsAndTransition) o;
         return other.transition.equals(transition) && other.fragments.equals(fragments);
       }

diff --git a/...n/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java b/...n/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java
@@ -16,8 +16,8 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.skylarkbuildapi.config.ConfigurationTransitionApi;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.syntax.SkylarkSemantics;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 /**
  * Implementation of {@link ConfigurationTransitionApi}.
@@ -41,12 +42,14 @@ public abstract class StarlarkDefinedConfigTransition implements ConfigurationTr
   private final List<String> inputs;
   private final List<String> outputs;
   private final Location location;
+  private final StoredEventHandler eventHandler;
 
   private StarlarkDefinedConfigTransition(
       List<String> inputs, List<String> outputs, Location location) {
     this.inputs = inputs;
     this.outputs = outputs;
     this.location = location;
+    this.eventHandler = new StoredEventHandler();
   }
 
   /**
@@ -56,17 +59,16 @@ private StarlarkDefinedConfigTransition(
   public abstract Boolean isForAnalysisTesting();
 
   /**
-   * Returns the input option keys for this transition. Only option keys contained in this
-   * list will be provided in the 'settings' argument given to the transition implementation
-   * function.
+   * Returns the input option keys for this transition. Only option keys contained in this list will
+   * be provided in the 'settings' argument given to the transition implementation function.
    */
   public List<String> getInputs() {
     return inputs;
   }
 
   /**
-   * Returns the output option keys for this transition. The transition implementation function
-   * must return a dictionary where the option keys exactly match the elements of this list.
+   * Returns the output option keys for this transition. The transition implementation function must
+   * return a dictionary where the option keys exactly match the elements of this list.
    */
   public List<String> getOutputs() {
     return outputs;
@@ -80,6 +82,10 @@ public Location getLocationForErrorReporting() {
     return location;
   }
 
+  public StoredEventHandler getEventHandler() {
+    return eventHandler;
+  }
+
   /**
    * Given a map of a subset of the "previous" build settings, returns the changed build settings as
    * a result of applying this transition.
@@ -101,9 +107,8 @@ public static StarlarkDefinedConfigTransition newRegularTransition(
       List<String> inputs,
       List<String> outputs,
       SkylarkSemantics semantics,
-      EventHandler eventHandler,
       StarlarkContext context) {
-    return new RegularTransition(impl, inputs, outputs, semantics, eventHandler, context);
+    return new RegularTransition(impl, inputs, outputs, semantics, context);
   }
 
   public static StarlarkDefinedConfigTransition newAnalysisTestTransition(
@@ -134,25 +139,41 @@ public ImmutableList<Map<String, Object>> getChangedSettings(
     public void repr(SkylarkPrinter printer) {
       printer.append("<analysis_test_transition object>");
     }
+
+    @Override
+    public boolean equals(Object object) {
+      if (object == this) {
+        return true;
+      }
+      if (object instanceof AnalysisTestTransition) {
+        AnalysisTestTransition otherTransition = (AnalysisTestTransition) object;
+        return Objects.equals(otherTransition.getInputs(), this.getInputs())
+            && Objects.equals(otherTransition.getOutputs(), this.getOutputs())
+            && Objects.equals(otherTransition.changedSettings, this.changedSettings);
+      }
+      return false;
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(this.getInputs(), this.getOutputs(), this.changedSettings);
+    }
   }
 
   private static class RegularTransition extends StarlarkDefinedConfigTransition {
     private final BaseFunction impl;
     private final SkylarkSemantics semantics;
-    private final EventHandler eventHandler;
     private final StarlarkContext starlarkContext;
 
     public RegularTransition(
         BaseFunction impl,
         List<String> inputs,
         List<String> outputs,
         SkylarkSemantics semantics,
-        EventHandler eventHandler,
         StarlarkContext context) {
       super(inputs, outputs, impl.getLocation());
       this.impl = impl;
       this.semantics = semantics;
-      this.eventHandler = eventHandler;
       this.starlarkContext = context;
     }
 
@@ -233,12 +254,31 @@ private Object evalFunction(BaseFunction function, ImmutableList<Object> args)
         Environment env =
             Environment.builder(mutability)
                 .setSemantics(semantics)
-                .setEventHandler(eventHandler)
+                .setEventHandler(getEventHandler())
                 .setStarlarkContext(starlarkContext)
                 .build();
 
         return function.call(args, ImmutableMap.of(), null, env);
       }
     }
+
+    @Override
+    public boolean equals(Object object) {
+      if (object == this) {
+        return true;
+      }
+      if (object instanceof RegularTransition) {
+        RegularTransition otherTransition = (RegularTransition) object;
+        return Objects.equals(otherTransition.getInputs(), this.getInputs())
+            && Objects.equals(otherTransition.getOutputs(), this.getOutputs())
+            && Objects.equals(otherTransition.impl, this.impl);
+      }
+      return false;
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(this.getInputs(), this.getOutputs(), this.impl);
+    }
   }
 }
diff --git a/...n/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java b/...n/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
@@ -17,6 +17,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
 
@@ -82,4 +83,29 @@ public boolean equals(Object other) {
         && ((ComposingTransition) other).transition1.equals(this.transition1)
         && ((ComposingTransition) other).transition2.equals(this.transition2);
   }
+
+  /**
+   * Recursively decompose a composing transition into all the {@link ConfigurationTransition}
+   * instances that it holds.
+   *
+   * @param root {@link ComposingTransition} to decompose
+   */
+  public static ImmutableList<ConfigurationTransition> decomposeTransition(
+      ConfigurationTransition root) {
+    ArrayList<ConfigurationTransition> toBeInspected = new ArrayList<>();
+    ImmutableList.Builder<ConfigurationTransition> transitions = new ImmutableList.Builder<>();
+    toBeInspected.add(root);
+    ConfigurationTransition current;
+    while (!toBeInspected.isEmpty()) {
+      current = toBeInspected.remove(0);
+      if (current instanceof ComposingTransition) {
+        ComposingTransition composingCurrent = (ComposingTransition) current;
+        toBeInspected.addAll(
+            ImmutableList.of(composingCurrent.transition1, composingCurrent.transition2));
+      } else {
+        transitions.add(current);
+      }
+    }
+    return transitions.build();
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java
@@ -55,7 +55,7 @@ public class FunctionTransitionUtil {
    * incoming {@link BuildOptions}. For native options, this involves a preprocess step of
    * converting options to their "command line form".
    *
-   * Also Validate that transitions output sensical results.
+   * <p>Also validate that transitions output sensical results.
    *
    * @param buildOptions the pre-transition build options
    * @param starlarkTransition the transition to apply

diff --git a/...a/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java b/...a/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java
@@ -17,6 +17,7 @@
 import static com.google.devtools.build.lib.analysis.skylark.FunctionTransitionUtil.applyAndValidate;
 import static com.google.devtools.build.lib.analysis.skylark.SkylarkAttributesCollection.ERROR_MESSAGE_FOR_NO_ATTR;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
@@ -52,21 +53,25 @@ public class StarlarkAttributeTransitionProvider implements SplitTransitionProvi
     this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
   }
 
+  @VisibleForTesting
+  public StarlarkDefinedConfigTransition getStarlarkDefinedConfigTransitionForTesting() {
+    return starlarkDefinedConfigTransition;
+  }
+
   @Override
   public SplitTransition apply(AttributeMap attributeMap) {
     Preconditions.checkArgument(attributeMap instanceof ConfiguredAttributeMapper);
     return new FunctionSplitTransition(
         starlarkDefinedConfigTransition, (ConfiguredAttributeMapper) attributeMap);
   }
 
-  private static class FunctionSplitTransition implements SplitTransition {
-    private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;
+  class FunctionSplitTransition extends StarlarkTransition implements SplitTransition {
     private final StructImpl attrObject;
 
     FunctionSplitTransition(
         StarlarkDefinedConfigTransition starlarkDefinedConfigTransition,
         ConfiguredAttributeMapper attributeMap) {
-      this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
+      super(starlarkDefinedConfigTransition);
 
       LinkedHashMap<String, Object> attributes = new LinkedHashMap<>();
       for (String attribute : attributeMap.getAttributeNames()) {

diff --git a/...n/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java b/...n/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java
@@ -16,6 +16,7 @@
 
 import static com.google.devtools.build.lib.analysis.skylark.FunctionTransitionUtil.applyAndValidate;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
@@ -53,19 +54,22 @@ public class StarlarkRuleTransitionProvider implements RuleTransitionFactory {
     this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
   }
 
+  @VisibleForTesting
+  public StarlarkDefinedConfigTransition getStarlarkDefinedConfigTransitionForTesting() {
+    return starlarkDefinedConfigTransition;
+  }
+
   @Override
   public PatchTransition buildTransitionFor(Rule rule) {
     return new FunctionPatchTransition(starlarkDefinedConfigTransition, rule);
   }
 
-  private static class FunctionPatchTransition implements PatchTransition {
-
-    private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;
+  class FunctionPatchTransition extends StarlarkTransition implements PatchTransition {
     private final StructImpl attrObject;
 
     FunctionPatchTransition(
         StarlarkDefinedConfigTransition starlarkDefinedConfigTransition, Rule rule) {
-      this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
+      super(starlarkDefinedConfigTransition);
 
       LinkedHashMap<String, Object> attributes = new LinkedHashMap<>();
       RawAttributeMapper attributeMapper = RawAttributeMapper.of(rule);

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkTransition.java
@@ -0,0 +1,52 @@
+// Copyright 2019 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.skylark;
+
+import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
+import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import java.util.Objects;
+
+/** A marker class for configuration transitions that are defined in Starlark. */
+public abstract class StarlarkTransition implements ConfigurationTransition {
+
+  private final StarlarkDefinedConfigTransition starlarkDefinedConfigTransition;
+
+  StarlarkTransition(StarlarkDefinedConfigTransition starlarkDefinedConfigTransition) {
+    this.starlarkDefinedConfigTransition = starlarkDefinedConfigTransition;
+  }
+
+  public void replayOn(ExtendedEventHandler eventHandler) {
+    starlarkDefinedConfigTransition.getEventHandler().replayOn(eventHandler);
+    starlarkDefinedConfigTransition.getEventHandler().clear();
+  }
+
+  @Override
+  public boolean equals(Object object) {
+    if (object == this) {
+      return true;
+    }
+    if (object instanceof StarlarkTransition) {
+      StarlarkDefinedConfigTransition starlarkDefinedConfigTransition =
+          ((StarlarkTransition) object).starlarkDefinedConfigTransition;
+      return Objects.equals(starlarkDefinedConfigTransition, this.starlarkDefinedConfigTransition);
+    }
+    return false;
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hashCode(starlarkDefinedConfigTransition);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -2150,6 +2150,11 @@ public SplitTransition getSplitTransition(AttributeMap attributeMapper) {
     return splitTransitionProvider.apply(attributeMapper);
   }
 
+  @VisibleForTesting
+  public SplitTransitionProvider getSplitTransitionProviderForTesting() {
+    return splitTransitionProvider;
+  }
+
   /**
    * Returns true if this attribute transitions on a split transition.
    * See {@link SplitTransition}.

diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java
@@ -59,7 +59,7 @@ public ConfigurationTransitionApi transition(
     validateBuildSettingKeys(inputs, "input", location);
     validateBuildSettingKeys(outputs, "output", location);
     return StarlarkDefinedConfigTransition.newRegularTransition(
-        implementation, inputs, outputs, semantics, env.getEventHandler(), context);
+        implementation, inputs, outputs, semantics, context);
   }
 
   @Override