Don't let latebound split attributes leave the host configuration.

This fixes a bug that put them out of sync from how all other
attributes manage the host configuration.

PiperOrigin-RevId: 154992583

4 files changed, 213 insertions, 34 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index 440e7b56cd..6eea61a62f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -353,15 +353,17 @@ public abstract class DependencyResolver {
 
       Collection<BuildOptions> splitOptions =
           getSplitOptions(depResolver.rule, attribute, ruleConfig);
-      if (!splitOptions.isEmpty()) {
+      if (!splitOptions.isEmpty() && !ruleConfig.isHostConfiguration()) {
         // Late-bound attribute with a split transition:
         // Since we want to get the same results as BuildConfiguration.evaluateTransition (but
         // skip it since we've already applied the split), we want to make sure this logic
         // doesn't do anything differently. evaluateTransition has additional logic
-        // for host configs and attributes with configurators. So we check here that neither of
-        // of those apply, in the name of keeping the fork as simple as possible.
+        // for host configs and attributes with configurators. To keep the fork as simple as
+        // possible, we don't support the configurator case. And when we're in the host
+        // configuration, we fall back to the non-split branch, which calls
+        // BuildConfiguration.evaluateTransition, which returns its "host mode" result without
+        // ever looking at the split.
         Verify.verify(attribute.getConfigurator() == null);
-        Verify.verify(!lateBoundDefault.useHostConfiguration());
 
         Iterable<BuildConfiguration> splitConfigs;
         if (!ruleConfig.useDynamicConfigurations()) {
@@ -374,8 +376,8 @@ public abstract class DependencyResolver {
           }
         }
         for (BuildConfiguration splitConfig : splitConfigs) {
-          for (Label dep : resolveLateBoundAttribute(
-              depResolver.rule, attribute, splitConfig, attributeMap)) {
+          for (Label dep : resolveLateBoundAttribute(depResolver.rule, attribute,
+              lateBoundDefault.useHostConfiguration() ? hostConfig : splitConfig, attributeMap)) {
             // Skip the normal config transition pipeline and directly feed the split config. This
             // is because the split already had to be applied to determine the attribute's value.
             // This makes the split logic in the normal pipeline redundant and potentially
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 106ccd119c..164e3a510e 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
@@ -39,7 +39,6 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.FailAction;
 import com.google.devtools.build.lib.analysis.BuildView.AnalysisResult;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
-import com.google.devtools.build.lib.analysis.config.ConfigurationFactory;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
 import com.google.devtools.build.lib.analysis.util.AnalysisMock;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestBase;
@@ -49,7 +48,6 @@ import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
-import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
 import com.google.devtools.build.lib.testutil.Suite;
 import com.google.devtools.build.lib.testutil.TestSpec;
 import com.google.devtools.build.lib.testutil.TestUtils;
@@ -1180,6 +1178,7 @@ public class BuildViewTest extends BuildViewTestBase {
       update("//okay");
       fail();
     } catch (ViewCreationFailedException e) {
+      // Expected.
     }
     assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     assertContainsEventWithFrequency(
@@ -1239,31 +1238,6 @@ public class BuildViewTest extends BuildViewTestBase {
         + "required config fragments: Jvm");
   }
 
-  @Test
-  public void lateBoundSplitAttributeConfigs() throws Exception {
-    useRuleClassProvider(LateBoundSplitUtil.getRuleClassProvider());
-    // Register the latebound split fragment with the config creation environment.
-    useConfigurationFactory(new ConfigurationFactory(
-        ruleClassProvider.getConfigurationCollectionFactory(),
-        ruleClassProvider.getConfigurationFragments()));
-
-    scratch.file("foo/BUILD",
-        "rule_with_latebound_split(",
-        "    name = 'foo')",
-        "rule_with_test_fragment(",
-        "    name = 'latebound_dep')");
-    update("//foo:foo");
-    assertNotNull(getConfiguredTarget("//foo:foo"));
-    Iterable<ConfiguredTarget> deps = SkyframeExecutorTestUtils.getExistingConfiguredTargets(
-        skyframeExecutor, Label.parseAbsolute("//foo:latebound_dep"));
-    assertThat(deps).hasSize(2);
-    assertThat(
-        ImmutableList.of(
-            LateBoundSplitUtil.getOptions(Iterables.get(deps, 0).getConfiguration()).fooFlag,
-            LateBoundSplitUtil.getOptions(Iterables.get(deps, 1).getConfiguration()).fooFlag))
-        .containsExactly("one", "two");
-  }
-
   /**
    * Here, injecting_rule injects an aspect which acts on a action_rule() and registers an action.
    * The action_rule() registers another action of its own.
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
new file mode 100644
index 0000000000..9cf51a0895
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurationsForLateBoundTargetsTest.java
@@ -0,0 +1,203 @@
+// 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 static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.packages.Attribute.attr;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import static org.junit.Assert.assertNotNull;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
+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.ConfigurationFactory;
+import com.google.devtools.build.lib.analysis.config.PatchTransition;
+import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.AttributeMap;
+import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
+import com.google.devtools.build.lib.testutil.Suite;
+import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.build.lib.testutil.TestSpec;
+import com.google.devtools.build.lib.testutil.UnknownRuleConfiguredTarget;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests <target, sourceConfig> -> <dep, depConfig> relationships over latebound attributes.
+ *
+ * <p>Ideally these tests would be in
+ * {@link com.google.devtools.build.lib.skyframe.ConfigurationsForTargetsTest}. But that's a
+ * Skyframe test (ConfiguredTargetFunction is a Skyframe function). And the Skyframe library doesn't
+ * know anything about latebound attributes. So we need to place these properly under the analysis
+ * package.
+ */
+@TestSpec(size = Suite.SMALL_TESTS)
+@RunWith(JUnit4.class)
+public class ConfigurationsForLateBoundTargetsTest extends AnalysisTestCase {
+  private static final PatchTransition CHANGE_FOO_FLAG_TRANSITION = new PatchTransition() {
+    @Override
+    public BuildOptions apply(BuildOptions options) {
+      BuildOptions toOptions = options.clone();
+      toOptions.get(LateBoundSplitUtil.TestOptions.class).fooFlag = "PATCHED!";
+      return toOptions;
+    }
+
+    @Override
+    public boolean defaultsToSelf() {
+      return false;
+    }
+  };
+
+  /**
+   * Rule definition with a latebound dependency.
+   */
+  private static class LateBoundDepRule implements RuleDefinition {
+    private static final Attribute.LateBoundLabel<BuildConfiguration> LATEBOUND_VALUE_RESOLVER =
+        new Attribute.LateBoundLabel<BuildConfiguration>() {
+          @Override
+          public Label resolve(Rule rule, AttributeMap attributes, BuildConfiguration config) {
+            return Label.parseAbsoluteUnchecked("//foo:latebound_dep");
+          }
+        };
+
+    @Override
+    public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment environment) {
+      return builder
+          .add(
+              attr(":latebound_attr", LABEL)
+                  .value(LATEBOUND_VALUE_RESOLVER)
+                  .cfg(CHANGE_FOO_FLAG_TRANSITION))
+          .requiresConfigurationFragments(LateBoundSplitUtil.TestFragment.class)
+          .build();
+    }
+
+    @Override
+    public Metadata getMetadata() {
+      return RuleDefinition.Metadata.builder()
+          .name("rule_with_latebound_attr")
+          .ancestors(BaseRuleClasses.RuleBase.class)
+          .factoryClass(UnknownRuleConfiguredTarget.class)
+          .build();
+    }
+  }
+
+  @Before
+  public void setupCustomLateBoundRules() throws Exception {
+    ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
+    TestRuleClassProvider.addStandardRules(builder);
+    builder.addRuleDefinition(new LateBoundSplitUtil.RuleWithLateBoundSplitAttribute());
+    builder.addRuleDefinition(new LateBoundSplitUtil.RuleWithTestFragment());
+    builder.addConfigurationFragment(new LateBoundSplitUtil.FragmentLoader());
+    builder.addConfigurationOptions(LateBoundSplitUtil.TestOptions.class);
+    builder.addRuleDefinition(new LateBoundDepRule());
+    useRuleClassProvider(builder.build());
+
+    // Register the latebound split fragment with the config creation environment.
+    useConfigurationFactory(new ConfigurationFactory(
+        ruleClassProvider.getConfigurationCollectionFactory(),
+        ruleClassProvider.getConfigurationFragments()));
+  }
+
+  @Test
+  public void lateBoundAttributeInTargetConfiguration() throws Exception {
+    scratch.file("foo/BUILD",
+        "rule_with_latebound_attr(",
+        "    name = 'foo')",
+        "rule_with_test_fragment(",
+        "    name = 'latebound_dep')");
+    update("//foo:foo");
+    assertNotNull(getConfiguredTarget("//foo:foo", getTargetConfiguration()));
+    ConfiguredTarget dep = Iterables.getOnlyElement(
+        SkyframeExecutorTestUtils.getExistingConfiguredTargets(
+            skyframeExecutor, Label.parseAbsolute("//foo:latebound_dep")));
+    assertThat(dep.getConfiguration()).isNotEqualTo(getTargetConfiguration());
+    assertThat(LateBoundSplitUtil.getOptions(dep.getConfiguration()).fooFlag).isEqualTo("PATCHED!");
+  }
+
+  @Test
+  public void lateBoundAttributeInHostConfiguration() throws Exception {
+    scratch.file("foo/BUILD",
+        "genrule(",
+        "    name = 'gen',",
+        "    srcs = [],",
+        "    outs = ['gen.out'],",
+        "    cmd = 'echo hi > $@',",
+        "    tools = [':foo'])",
+        "rule_with_latebound_attr(",
+        "    name = 'foo')",
+        "rule_with_test_fragment(",
+        "    name = 'latebound_dep')");
+    update("//foo:gen");
+    assertNotNull(getConfiguredTarget("//foo:foo", getHostConfiguration()));
+    ConfiguredTarget dep = Iterables.getOnlyElement(
+        SkyframeExecutorTestUtils.getExistingConfiguredTargets(
+            skyframeExecutor, Label.parseAbsolute("//foo:latebound_dep")));
+    assertThat(dep.getConfiguration()).isEqualTo(getHostConfiguration());
+    // This is technically redundant, but slightly stronger in sanity checking that the host
+    // configuration doesn't happen to match what the patch would have done.
+    assertThat(LateBoundSplitUtil.getOptions(dep.getConfiguration()).fooFlag).isEmpty();
+  }
+
+  @Test
+  public void lateBoundSplitAttributeInTargetConfiguration() throws Exception {
+    scratch.file("foo/BUILD",
+        "rule_with_latebound_split(",
+        "    name = 'foo')",
+        "rule_with_test_fragment(",
+        "    name = 'latebound_dep')");
+    update("//foo:foo");
+    assertNotNull(getConfiguredTarget("//foo:foo"));
+    Iterable<ConfiguredTarget> deps = SkyframeExecutorTestUtils.getExistingConfiguredTargets(
+        skyframeExecutor, Label.parseAbsolute("//foo:latebound_dep"));
+    assertThat(deps).hasSize(2);
+    assertThat(
+        ImmutableList.of(
+            LateBoundSplitUtil.getOptions(Iterables.get(deps, 0).getConfiguration()).fooFlag,
+            LateBoundSplitUtil.getOptions(Iterables.get(deps, 1).getConfiguration()).fooFlag))
+        .containsExactly("one", "two");
+  }
+
+  @Test
+  public void lateBoundSplitAttributeInHostConfiguration() throws Exception {
+    scratch.file("foo/BUILD",
+        "genrule(",
+        "    name = 'gen',",
+        "    srcs = [],",
+        "    outs = ['gen.out'],",
+        "    cmd = 'echo hi > $@',",
+        "    tools = [':foo'])",
+        "rule_with_latebound_split(",
+        "    name = 'foo')",
+        "rule_with_test_fragment(",
+        "    name = 'latebound_dep')");
+    update("//foo:gen");
+    assertNotNull(getConfiguredTarget("//foo:foo", getHostConfiguration()));
+    ConfiguredTarget dep = Iterables.getOnlyElement(
+        SkyframeExecutorTestUtils.getExistingConfiguredTargets(
+            skyframeExecutor, Label.parseAbsolute("//foo:latebound_dep")));
+    assertThat(dep.getConfiguration()).isEqualTo(getHostConfiguration());
+    // This is technically redundant, but slightly stronger in sanity checking that the host
+    // configuration doesn't happen to match what the split would have done.
+    assertThat(LateBoundSplitUtil.getOptions(dep.getConfiguration()).fooFlag).isEmpty();
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java
index 2e29666d5d..4fa89fc3f1 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java
@@ -81,7 +81,7 @@ public class LateBoundSplitUtil {
   /**
    * The {@link BuildConfiguration.Fragment} that contains the options.
    */
-  private static class TestFragment extends BuildConfiguration.Fragment {
+  static class TestFragment extends BuildConfiguration.Fragment {
   }
 
   /**