Add dynamic config support for top-level configuration hooks.

PiperOrigin-RevId: 155223580

7 files changed, 305 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 5a45dc6bca..4c22da39fb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -40,6 +40,7 @@ import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAsp
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 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.ComposingSplitTransition;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -840,12 +841,19 @@ public class BuildView {
     for (BuildConfiguration config : configurations.getTargetConfigurations()) {
       for (Target target : targets) {
         nodes.add(new TargetAndConfiguration(target,
-            BuildConfigurationCollection.configureTopLevelTarget(config, target)));
+            config.useDynamicConfigurations()
+                // Dynamic configurations apply top-level transitions through a different code path:
+                // BuildConfiguration#topLevelConfigurationHook. That path has the advantages of a)
+                // not requiring a global transitions table and b) making its choices outside core
+                // Bazel code.
+                ? (target.isConfigurable() ? config : null)
+                : BuildConfigurationCollection.configureTopLevelTarget(config, target)));
       }
     }
-    return configurations.useDynamicConfigurations()
-        ? getDynamicConfigurations(nodes, eventHandler)
-        : ImmutableList.copyOf(nodes);
+    return ImmutableList.copyOf(
+        configurations.useDynamicConfigurations()
+            ? getDynamicConfigurations(nodes, eventHandler)
+            : nodes);
   }
 
   /**
@@ -855,7 +863,8 @@ public class BuildView {
    *
    * <p>Else returns configurations that unconditionally include all fragments.
    *
-   * <p>Preserves the original input order. Uses original (untrimmed) configurations for targets
+   * <p>Preserves the original input order (but merges duplicate nodes that might occur due to
+   * top-level configuration transitions) . Uses original (untrimmed) configurations for targets
    * that can't be evaluated (e.g. due to loading phase errors).
    *
    * <p>This is suitable for feeding {@link ConfiguredTargetValue} keys: as general principle {@link
@@ -864,7 +873,7 @@ public class BuildView {
    */
   // TODO(bazel-team): error out early for targets that fail - untrimmed configurations should
   // never make it through analysis (and especially not seed ConfiguredTargetValues)
-  private List<TargetAndConfiguration> getDynamicConfigurations(
+  private LinkedHashSet<TargetAndConfiguration> getDynamicConfigurations(
       Iterable<TargetAndConfiguration> inputs, ExtendedEventHandler eventHandler)
       throws InterruptedException {
     Map<Label, Target> labelsToTargets = new LinkedHashMap<>();
@@ -876,23 +885,10 @@ public class BuildView {
 
     for (TargetAndConfiguration targetAndConfig : inputs) {
       labelsToTargets.put(targetAndConfig.getLabel(), targetAndConfig.getTarget());
-
-      Attribute.Transition ruleclassTransition = null;
-      if (targetAndConfig.getTarget().getAssociatedRule() != null) {
-        Rule associatedRule = targetAndConfig.getTarget().getAssociatedRule();
-        RuleTransitionFactory transitionFactory =
-            associatedRule.getRuleClassObject().getTransitionFactory();
-        if (transitionFactory != null) {
-          ruleclassTransition = transitionFactory.buildTransitionFor(associatedRule);
-        }
-      }
       if (targetAndConfig.getConfiguration() != null) {
         asDeps.put(targetAndConfig.getConfiguration(),
             Dependency.withTransitionAndAspects(
-                targetAndConfig.getLabel(),
-                ruleclassTransition == null
-                  ? Attribute.ConfigurationTransition.NONE
-                  : ruleclassTransition,
+                targetAndConfig.getLabel(), getTopLevelTransition(targetAndConfig),
                 // TODO(bazel-team): support top-level aspects
                 AspectCollection.EMPTY));
       }
@@ -916,8 +912,7 @@ public class BuildView {
       }
     }
 
-    ImmutableList.Builder<TargetAndConfiguration> result =
-        ImmutableList.<TargetAndConfiguration>builder();
+    LinkedHashSet<TargetAndConfiguration> result = new LinkedHashSet<>();
     for (TargetAndConfiguration originalInput : inputs) {
       if (successfullyEvaluatedTargets.containsKey(originalInput)) {
         // The configuration was successfully trimmed.
@@ -927,9 +922,46 @@ public class BuildView {
         result.add(originalInput);
       }
     }
-    return result.build();
+    return result;
+  }
+
+  /**
+   * Returns the transition to apply to the top-level configuration before applying it to this
+   * target. This enables support for rule-triggered top-level configuration hooks.
+   */
+  private static Attribute.Transition getTopLevelTransition(
+      TargetAndConfiguration targetAndConfig) {
+    Target target = targetAndConfig.getTarget();
+    BuildConfiguration fromConfig = targetAndConfig.getConfiguration();
+    Preconditions.checkArgument(fromConfig.useDynamicConfigurations());
+
+    // Top-level transitions (chosen by configuration fragments):
+    Transition topLevelTransition = fromConfig.topLevelConfigurationHook(target);
+    if (topLevelTransition == null) {
+      topLevelTransition = ConfigurationTransition.NONE;
+    }
+
+    // Rule class transitions (chosen by rule class definitions):
+    if (target.getAssociatedRule() == null) {
+      return topLevelTransition;
+    }
+    Rule associatedRule = target.getAssociatedRule();
+    RuleTransitionFactory transitionFactory =
+        associatedRule.getRuleClassObject().getTransitionFactory();
+    if (transitionFactory == null) {
+      return topLevelTransition;
+    }
+    Attribute.Transition ruleClassTransition = transitionFactory.buildTransitionFor(associatedRule);
+    if (ruleClassTransition == null) {
+      return topLevelTransition;
+    } else if (topLevelTransition == ConfigurationTransition.NONE) {
+      return ruleClassTransition;
+    } else {
+      return new ComposingSplitTransition(topLevelTransition, ruleClassTransition);
+    }
   }
 
+
   /**
    * Gets a dynamic configuration for the given target.
    *
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
index e942556160..8514cdd7cd 100644
--- 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
@@ -212,6 +212,20 @@ public final class BuildConfiguration {
     public PatchTransition getArtifactOwnerTransition() {
       return null;
     }
+
+    /**
+     * Returns an extra transition that should apply to top-level targets in this
+     * configuration. Returns null if no transition is needed.
+     *
+     * <p>Overriders should not change {@link FragmentOptions} not associated with their fragment.
+     *
+     * <p>If multiple fragments specify a transition, they're composed together in a
+     * deterministic but undocumented order (so don't write code expecting a specific order).
+     */
+    @Nullable
+    public PatchTransition topLevelConfigurationHook(Target toTarget) {
+      return null;
+    }
   }
 
   private static final Label convertLabel(String input) throws OptionsParsingException {
@@ -2679,4 +2693,24 @@ public final class BuildConfiguration {
   public ImmutableCollection<String> getSkylarkFragmentNames() {
     return skylarkVisibleFragments.keySet();
   }
+
+  /**
+   * Returns an extra transition that should apply to top-level targets in this
+   * configuration. Returns null if no transition is needed.
+   */
+  @Nullable
+  public PatchTransition topLevelConfigurationHook(Target toTarget) {
+    PatchTransition currentTransition = null;
+    for (Fragment fragment : fragments.values()) {
+      PatchTransition fragmentTransition = fragment.topLevelConfigurationHook(toTarget);
+      if (fragmentTransition == null) {
+        continue;
+      } else if (currentTransition == null) {
+        currentTransition = fragmentTransition;
+      } else {
+        currentTransition = new ComposingPatchTransition(currentTransition, fragmentTransition);
+      }
+    }
+    return currentTransition;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingPatchTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingPatchTransition.java
new file mode 100644
index 0000000000..fb4c7e5d6c
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ComposingPatchTransition.java
@@ -0,0 +1,42 @@
+// 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;
+
+import com.google.common.collect.Iterables;
+
+/**
+ * A {@link ComposingSplitTransition} that only supports {@link PatchTransition}s
+ *
+ * <p>Calling code that doesn't want to have to handle splits should prefer this version.
+ */
+public class ComposingPatchTransition implements PatchTransition {
+  private final ComposingSplitTransition delegate;
+
+  public ComposingPatchTransition(PatchTransition transition1, PatchTransition transition2) {
+    this.delegate = new ComposingSplitTransition(transition1, transition2);
+  }
+
+  @Override
+  public boolean defaultsToSelf() {
+    throw new UnsupportedOperationException(
+        "dynamic configurations don't use global transition tables");
+  }
+
+  @Override
+  public BuildOptions apply(BuildOptions options) {
+    return Iterables.getOnlyElement(delegate.split(options));
+  }
+}
+
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index b911026c94..4388571311 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -40,10 +40,13 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
+import com.google.devtools.build.lib.packages.OutputFile;
+import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigurationParameters;
 import com.google.devtools.build.lib.rules.cpp.CppLinkActionConfigs.CppLinkPlatform;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.transitions.ContextCollectorOwnerTransition;
+import com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
@@ -2253,4 +2256,19 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
   public PatchTransition getArtifactOwnerTransition() {
     return isLipoContextCollector() ? ContextCollectorOwnerTransition.INSTANCE : null;
   }
+
+  @Nullable
+  @Override
+  public PatchTransition topLevelConfigurationHook(Target toTarget) {
+    // Top-level output files that aren't outputs of the LIPO context should be built in
+    // the data config. This is so their output path prefix doesn't have "-lipo" in it, which
+    // is a confusing and unnecessary deviation from how they would normally look.
+    if (toTarget instanceof OutputFile
+        && isLipoOptimization()
+        && !toTarget.getAssociatedRule().getLabel().equals(getLipoContextLabel())) {
+      return DisableLipoTransition.INSTANCE;
+    } else {
+      return null;
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 164e3a510e..21988605e5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -330,6 +330,32 @@ public class BuildViewTest extends BuildViewTestBase {
   }
 
   @Test
+  public void topLevelConfigurationHook() throws Exception {
+    setConfigFragmentsAvailableInTests(TestConfigFragments.FragmentWithTopLevelConfigHook1Factory);
+    scratch.file(
+        "package/BUILD",
+        "sh_binary(name = 'binary', srcs = ['binary.sh'])");
+    ConfiguredTarget ct = Iterables.getOnlyElement(update("//package:binary").getTargetsToBuild());
+    BuildConfiguration.Options options =
+        ct.getConfiguration().getOptions().get(BuildConfiguration.Options.class);
+    assertThat(options.testArguments).containsExactly("CONFIG HOOK 1");
+  }
+
+  @Test
+  public void topLevelComposedConfigurationHooks() throws Exception {
+    setConfigFragmentsAvailableInTests(
+        TestConfigFragments.FragmentWithTopLevelConfigHook1Factory,
+        TestConfigFragments.FragmentWithTopLevelConfigHook2Factory);
+    scratch.file(
+        "package/BUILD",
+        "sh_binary(name = 'binary', srcs = ['binary.sh'])");
+    ConfiguredTarget ct = Iterables.getOnlyElement(update("//package:binary").getTargetsToBuild());
+    BuildConfiguration.Options options =
+        ct.getConfiguration().getOptions().get(BuildConfiguration.Options.class);
+    assertThat(options.testArguments).containsExactly("CONFIG HOOK 1", "CONFIG HOOK 2");
+  }
+
+  @Test
   public void testGetDirectPrerequisites() throws Exception {
     scratch.file(
         "package/BUILD",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/TestConfigFragments.java b/src/test/java/com/google/devtools/build/lib/analysis/TestConfigFragments.java
new file mode 100644
index 0000000000..e0b862c851
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/analysis/TestConfigFragments.java
@@ -0,0 +1,113 @@
+// 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;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+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.ConfigurationEnvironment;
+import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
+import com.google.devtools.build.lib.analysis.config.PatchTransition;
+import com.google.devtools.build.lib.packages.Target;
+
+/**
+ * Grab bag file for test configuration fragments and fragment factories.
+ */
+public class TestConfigFragments {
+  /**
+   * A {@link PatchTransition} that appends a given value to
+   * {@link BuildConfiguration.Options#testArguments}.
+   */
+  private static class TestArgTransition implements PatchTransition {
+
+    private final String patchMessage;
+
+    TestArgTransition(String patchMessage) {
+      this.patchMessage = patchMessage;
+    }
+
+    @Override
+    public boolean defaultsToSelf() {
+      throw new UnsupportedOperationException(
+          "dynamic configurations don't use global transition tables");
+    }
+
+    @Override
+    public BuildOptions apply(BuildOptions options) {
+      BuildOptions toOptions = options.clone();
+      BuildConfiguration.Options coreOptions =
+          toOptions.get(BuildConfiguration.Options.class);
+      coreOptions.testArguments = new ImmutableList.Builder<String>()
+          .addAll(coreOptions.testArguments).add(patchMessage).build();
+      return toOptions;
+    }
+  }
+
+  /**
+   * A {@link ConfigurationFragmentFactory} that trivially returns a given fragment.
+   */
+  private static class SimpleFragmentFactory implements ConfigurationFragmentFactory {
+    private final BuildConfiguration.Fragment fragment;
+
+    public SimpleFragmentFactory(BuildConfiguration.Fragment fragment) {
+      this.fragment = fragment;
+    }
+
+    @Override
+    public BuildConfiguration.Fragment create(ConfigurationEnvironment env,
+        BuildOptions buildOptions) throws InvalidConfigurationException {
+      return fragment;
+    }
+
+    @Override
+    public Class<? extends BuildConfiguration.Fragment> creates() {
+      return fragment.getClass();
+    }
+
+    @Override
+    public ImmutableSet<Class<? extends FragmentOptions>> requiredOptions() {
+      return ImmutableSet.<Class<? extends FragmentOptions>>of();
+    }
+  }
+
+  /**
+   * Factory for a test fragment with a top-level configuration hook.
+   */
+  public static SimpleFragmentFactory FragmentWithTopLevelConfigHook1Factory =
+      new SimpleFragmentFactory(new BuildConfiguration.Fragment() {
+        @Override
+        public PatchTransition topLevelConfigurationHook(Target toTarget) {
+          return new TestArgTransition("CONFIG HOOK 1");
+        }
+      });
+
+  /**
+   * Factory for a test fragment with a top-level configuration hook.
+   */
+  public static SimpleFragmentFactory FragmentWithTopLevelConfigHook2Factory =
+      // The anonymous class definition for the BuildConfiguration.Fragment needs to be different
+      // than the one of its peer above. This is because Bazel indexes configuration fragments
+      // by class name. So we need to make sure all fragment definitions retain distinct class
+      // names (i.e. "TestConfigFragments$1", "TestConfigFragments$2", etc).
+      new SimpleFragmentFactory(new BuildConfiguration.Fragment() {
+        @Override
+        public PatchTransition topLevelConfigurationHook(Target toTarget) {
+          return new TestArgTransition("CONFIG HOOK 2");
+        }
+      });
+}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index a7ddfa9228..5b49663020 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 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.ConfigurationFactory;
+import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.buildtool.BuildRequest.BuildRequestOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -164,6 +165,10 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
   protected void useRuleClassProvider(ConfiguredRuleClassProvider ruleClassProvider)
       throws Exception {
     this.ruleClassProvider = ruleClassProvider;
+    useConfigurationFactory(
+        new ConfigurationFactory(
+            ruleClassProvider.getConfigurationCollectionFactory(),
+            ruleClassProvider.getConfigurationFragments()));
     PackageFactory pkgFactory =
         analysisMock
             .getPackageFactoryForTesting()
@@ -504,4 +509,16 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
     update();
   }
 
+  /**
+   * Makes custom configuration fragments available in tests.
+   */
+  protected final void setConfigFragmentsAvailableInTests(
+      ConfigurationFragmentFactory... factories) throws Exception {
+    ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
+    TestRuleClassProvider.addStandardRules(builder);
+    for (ConfigurationFragmentFactory factory : factories) {
+      builder.addConfigurationFragment(factory);
+    }
+    useRuleClassProvider(builder.build());
+  }
 }