Allow starlark transitions to read starlark-defined options.

Calculate and supply defaults for all read starlark options before transitions are applied. Future work includes: - combining skyfunction calls for inputs and outputs of starlark transitions - enforcing that BuildOptions.starlarkOptions will never hold default values (opposite of native options which always hold default values) in order to make BuildConfigurationWithDefault == BuildConfigurationWithNoValue - Making sure BuildConfiguration properly reflects StarlarkOptions Work towards #5574, #5577, #5578 RELNOTES: None. PiperOrigin-RevId: 242681946
bazelbuild · Apr 9, 2019 · 1a70c27 · 1a70c27
1 parent 6778a05
commit 1a70c27
Show file tree

Hide file tree

Showing 9 changed files with 351 additions and 73 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -1090,13 +1090,14 @@ public ArtifactRoot getRoot(
   private final Supplier<BuildConfigurationEvent> buildEventSupplier;
 
   /**
-   * Returns true if this configuration is semantically equal to the other, with
-   * the possible exception that the other has fewer fragments.
+   * Returns true if this configuration is semantically equal to the other, with the possible
+   * exception that the other has fewer fragments.
    *
    * <p>This is useful for trimming: as the same configuration gets "trimmed" while going down a
    * dependency chain, it's still the same configuration but loses some of its fragments. So we need
    * a more nuanced concept of "equality" than simple reference equality.
    */
+  // TODO(b/121048710): make this reflect starlark options
   public boolean equalsOrIsSupersetOf(BuildConfiguration other) {
     return this.equals(other)
         || (other != null

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -496,6 +496,11 @@ public Builder addStarlarkOption(Label key, Object value) {
       return this;
     }
 
+    /** Returns whether the builder contains a particular Starlark option. */
+    boolean contains(Label key) {
+      return starlarkOptions.containsKey(key);
+    }
+
     /** Removes the value for the Starlark option with the given key. */
     public Builder removeStarlarkOption(Label key) {
       starlarkOptions.remove(key);

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
@@ -20,6 +20,7 @@
 import com.google.common.base.Verify;
 import com.google.common.base.VerifyException;
 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.common.collect.LinkedHashMultimap;
@@ -218,13 +219,21 @@ public static OrderedSetMultimap<DependencyKind, Dependency> resolveConfiguratio
       FragmentsAndTransition transitionKey = new FragmentsAndTransition(depFragments, transition);
       List<BuildOptions> toOptions = transitionsMap.get(transitionKey);
       if (toOptions == null) {
+        // Default values for all build settings read in {@code transition}
+        ImmutableMap<Label, Object> defaultBuildSettingValues;
+        try {
+          defaultBuildSettingValues = StarlarkTransition.getDefaultInputValues(env, transition);
+        } catch (TransitionException e) {
+          throw new ConfiguredTargetFunction.DependencyEvaluationException(e);
+        }
         toOptions =
             applyTransition(
                 currentConfiguration.getOptions(),
                 transition,
                 depFragments,
                 ruleClassProvider,
-                !sameFragments);
+                !sameFragments,
+                defaultBuildSettingValues);
         transitionsMap.put(transitionKey, toOptions);
       }
 
@@ -233,7 +242,7 @@ public static OrderedSetMultimap<DependencyKind, Dependency> resolveConfiguratio
       // configured target.
       try {
         ImmutableSet<SkyKey> buildSettingPackageKeys =
-            StarlarkTransition.getBuildSettingPackageKeys(transition);
+            StarlarkTransition.getBuildSettingPackageKeys(transition, "outputs");
         Map<SkyKey, SkyValue> buildSettingPackages = env.getValues(buildSettingPackageKeys);
         if (env.valuesMissing()) {
           return null;
@@ -458,18 +467,22 @@ private static Set<Class<? extends BuildConfiguration.Fragment>> getTransitiveFr
   /**
    * Applies a configuration transition over a set of build options.
    *
-   * @return the build options for the transitioned configuration. If trimResults is true,
-   *     only options needed by the required fragments are included. Else the same options as the
+   * @return the build options for the transitioned configuration. If trimResults is true, only
+   *     options needed by the required fragments are included. Else the same options as the
    *     original input are included (with different possible values, of course).
    */
   @VisibleForTesting
-  public static List<BuildOptions> applyTransition(BuildOptions fromOptions,
+  public static List<BuildOptions> applyTransition(
+      BuildOptions fromOptions,
       ConfigurationTransition transition,
       Iterable<Class<? extends BuildConfiguration.Fragment>> requiredFragments,
-      RuleClassProvider ruleClassProvider, boolean trimResults) {
+      RuleClassProvider ruleClassProvider,
+      boolean trimResults,
+      ImmutableMap<Label, Object> buildSettingDefaults) {
+    BuildOptions fromOptionsWithDefaults =
+        addDefaultStarlarkOptions(fromOptions, buildSettingDefaults);
     // TODO(bazel-team): safety-check that this never mutates fromOptions.
-    List<BuildOptions> result = transition.apply(fromOptions);
-
+    List<BuildOptions> result = transition.apply(fromOptionsWithDefaults);
     if (!trimResults) {
       return result;
     } else {
@@ -482,6 +495,18 @@ public static List<BuildOptions> applyTransition(BuildOptions fromOptions,
     }
   }
 
+  private static BuildOptions addDefaultStarlarkOptions(
+      BuildOptions fromOptions, ImmutableMap<Label, Object> buildSettingDefaults) {
+    BuildOptions.Builder optionsWithDefaults = fromOptions.toBuilder();
+    for (Map.Entry<Label, Object> buildSettingDefault : buildSettingDefaults.entrySet()) {
+      Label buildSetting = buildSettingDefault.getKey();
+      if (!optionsWithDefaults.contains(buildSetting)) {
+        optionsWithDefaults.addStarlarkOption(buildSetting, buildSettingDefault.getValue());
+      }
+    }
+    return optionsWithDefaults.build();
+  }
+
   /**
    * Checks the config fragments required by a dep against the fragments in its actual
    * configuration. If any are missing, triggers a descriptive "missing fragments" error.

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
@@ -160,6 +160,7 @@ static SkylarkDict<String, Object> buildSettings(
     try (Mutability mutability = Mutability.create("build_settings")) {
       SkylarkDict<String, Object> dict = SkylarkDict.withMutability(mutability);
 
+      // Add native options
       for (Map.Entry<String, OptionInfo> entry : optionInfoMap.entrySet()) {
         String optionName = entry.getKey();
         String optionKey = COMMAND_LINE_OPTION_PREFIX + optionName;
@@ -182,7 +183,14 @@ static SkylarkDict<String, Object> buildSettings(
         }
       }
 
-      // TODO(juliexxia): Allowing reading of starlark-defined build settings.
+      // Add Starlark options
+      for (Map.Entry<Label, Object> starlarkOption : buildOptions.getStarlarkOptions().entrySet()) {
+        if (!remainingInputs.remove(starlarkOption.getKey().toString())) {
+          continue;
+        }
+        dict.put(starlarkOption.getKey().toString(), starlarkOption.getValue(), null, mutability);
+      }
+
       if (!remainingInputs.isEmpty()) {
         throw new EvalException(
             starlarkTransition.getLocationForErrorReporting(),

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
@@ -14,7 +14,10 @@
 package com.google.devtools.build.lib.analysis.skylark;
 
 import static com.google.devtools.build.lib.analysis.skylark.FunctionTransitionUtil.COMMAND_LINE_OPTION_PREFIX;
+import static com.google.devtools.build.lib.packages.RuleClass.Builder.SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
 
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -29,6 +32,7 @@
 import com.google.devtools.build.lib.skyframe.PackageValue;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import com.google.devtools.build.skyframe.SkyFunction.Environment;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.List;
@@ -54,6 +58,10 @@ public boolean hasErrors() {
     return starlarkDefinedConfigTransition.getEventHandler().hasErrors();
   }
 
+  private List<String> getInputs() {
+    return starlarkDefinedConfigTransition.getInputs();
+  }
+
   private List<String> getOutputs() {
     return starlarkDefinedConfigTransition.getOutputs();
   }
@@ -78,30 +86,43 @@ public String getMessage() {
   }
 
   /**
-   * For a given transition, find all Starlark-defined build settings that were set by that
-   * transition. Then return all package keys for those flags.
+   * For a given transition, find all relevant Starlark-defined build settings. Then return all
+   * package keys for those flags.
    *
    * <p>Currently this method does not handle the possibility of aliased build settings. We may not
    * actually load the package that actually contains the build setting but we won't know until we
    * fetch the actual target.
+   *
+   * @param root transition to inspect
+   * @param inputsOrOutputs whether to return inputs or outputs
    */
   // TODO(juliexxia): handle the possibility of aliased build settings.
-  public static ImmutableSet<SkyKey> getBuildSettingPackageKeys(ConfigurationTransition root) {
+  public static ImmutableSet<SkyKey> getBuildSettingPackageKeys(
+      ConfigurationTransition root, String inputsOrOutputs) {
     ImmutableSet.Builder<SkyKey> keyBuilder = new ImmutableSet.Builder<>();
     try {
       root.visit(
           (StarlarkTransitionVisitor)
               transition -> {
-                for (Label setting : getChangedStarlarkSettings(transition)) {
-                  keyBuilder.add(PackageValue.key(setting.getPackageIdentifier()));
-                }
+                keyBuilder.addAll(
+                    getBuildSettingPackageKeys(
+                        getRelevantStarlarkSettingsFromTransition(transition, inputsOrOutputs)));
               });
     } catch (TransitionException e) {
       // Not actually thrown in the visitor, but declared.
     }
     return keyBuilder.build();
   }
 
+  private static ImmutableSet<SkyKey> getBuildSettingPackageKeys(
+      ImmutableSet<Label> buildSettings) {
+    ImmutableSet.Builder<SkyKey> keyBuilder = new ImmutableSet.Builder<>();
+    for (Label setting : buildSettings) {
+      keyBuilder.add(PackageValue.key(setting.getPackageIdentifier()));
+    }
+    return keyBuilder.build();
+  }
+
   /**
    * Method to be called after Starlark-transitions are applied. Handles events and checks outputs.
    *
@@ -138,27 +159,11 @@ public static void validate(
     root.visit(
         (StarlarkTransitionVisitor)
             transition -> {
-              List<Label> changedSettings = getChangedStarlarkSettings(transition);
+              ImmutableSet<Label> changedSettings =
+                  getRelevantStarlarkSettingsFromTransition(transition, "outputs");
               for (Label setting : changedSettings) {
-                Package buildSettingPackage =
-                    ((PackageValue)
-                            buildSettingPackages.get(
-                                PackageValue.key(setting.getPackageIdentifier())))
-                        .getPackage();
-                Target buildSettingTarget;
-                try {
-                  buildSettingTarget = buildSettingPackage.getTarget(setting.getName());
-                } catch (NoSuchTargetException e) {
-                  throw new TransitionException(e);
-                }
-                if (buildSettingTarget.getAssociatedRule() == null
-                    || buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting()
-                        == null) {
-                  throw new TransitionException(
-                      String.format(
-                          "attempting to transition on '%s' which is not a build setting",
-                          setting));
-                }
+                Target buildSettingTarget =
+                    getAndCheckBuildSettingTarget(buildSettingPackages, setting);
                 changedSettingToType.put(
                     setting,
                     buildSettingTarget
@@ -183,11 +188,79 @@ public static void validate(
     }
   }
 
-  private static List<Label> getChangedStarlarkSettings(StarlarkTransition transition) {
-    return transition.getOutputs().stream()
-        .filter(setting -> !setting.startsWith(COMMAND_LINE_OPTION_PREFIX))
-        .map(Label::parseAbsoluteUnchecked)
-        .collect(Collectors.toList());
+  /**
+   * For a given transition, find all Starlark build settings that are read while applying it, then
+   * return a map of their label to their default values.
+   */
+  public static ImmutableMap<Label, Object> getDefaultInputValues(
+      Environment env, ConfigurationTransition root)
+      throws TransitionException, InterruptedException {
+    ImmutableSet<SkyKey> buildSettingInputPackageKeys = getBuildSettingPackageKeys(root, "inputs");
+    Map<SkyKey, SkyValue> buildSettingPackages = env.getValues(buildSettingInputPackageKeys);
+    if (env.valuesMissing()) {
+      return null;
+    }
+    return getDefaultInputValues(buildSettingPackages, root);
+  }
+
+  /**
+   * For a given transition, find all Starlark build settings that are read while applying it, then
+   * return a map of their label to their default values.
+   */
+  public static ImmutableMap<Label, Object> getDefaultInputValues(
+      Map<SkyKey, SkyValue> buildSettingPackages, ConfigurationTransition root)
+      throws TransitionException {
+    ImmutableMap.Builder<Label, Object> defaultValues = new ImmutableMap.Builder<>();
+    root.visit(
+        (StarlarkTransitionVisitor)
+            transition -> {
+              ImmutableSet<Label> settings =
+                  getRelevantStarlarkSettingsFromTransition(transition, "inputs");
+              for (Label setting : settings) {
+                Target buildSettingTarget =
+                    getAndCheckBuildSettingTarget(buildSettingPackages, setting);
+                defaultValues.put(
+                    setting,
+                    buildSettingTarget
+                        .getAssociatedRule()
+                        .getAttributeContainer()
+                        .getAttr(SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME));
+              }
+            });
+    return defaultValues.build();
+  }
+
+  private static Target getAndCheckBuildSettingTarget(
+      Map<SkyKey, SkyValue> buildSettingPackages, Label setting) throws TransitionException {
+    Package buildSettingPackage =
+        ((PackageValue) buildSettingPackages.get(PackageValue.key(setting.getPackageIdentifier())))
+            .getPackage();
+    Preconditions.checkNotNull(
+        buildSettingPackage, "Reading build setting for which we don't have a package");
+    Target buildSettingTarget;
+    try {
+      buildSettingTarget = buildSettingPackage.getTarget(setting.getName());
+    } catch (NoSuchTargetException e) {
+      throw new TransitionException(e);
+    }
+    if (buildSettingTarget.getAssociatedRule() == null
+        || buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting() == null) {
+      throw new TransitionException(
+          String.format("attempting to transition on '%s' which is not a build setting", setting));
+    }
+    return buildSettingTarget;
+  }
+
+  // TODO(juliexxia): use an enum for "inputs"/"outputs" here and elsewhere in starlark transitions.
+  private static ImmutableSet<Label> getRelevantStarlarkSettingsFromTransition(
+      StarlarkTransition transition, String inputOrOutput) {
+    List<String> toGet =
+        inputOrOutput.equals("inputs") ? transition.getInputs() : transition.getOutputs();
+    return ImmutableSet.copyOf(
+        toGet.stream()
+            .filter(setting -> !setting.startsWith(COMMAND_LINE_OPTION_PREFIX))
+            .map(Label::parseAbsoluteUnchecked)
+            .collect(Collectors.toSet()));
   }
 
   /**