Refactor CppLinkAction to construct its command line using the crosstool instead of a hardcoded switch. Add action configs to CppConfiguration for each link type.

--
MOS_MIGRATED_REVID=128470872

2 files changed, 242 insertions, 12 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
index c4ea2be93f..7531632e8a 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
@@ -17,8 +17,8 @@ package com.google.devtools.build.lib.rules.cpp;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
-import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
@@ -34,18 +34,17 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
+import com.google.devtools.build.lib.rules.cpp.CppLinkActionConfigs.CppLinkPlatform;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
 import com.google.devtools.build.lib.vfs.PathFragment;
-
+import java.io.IOException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/**
- * Tests for {@link CppLinkAction}.
- */
+/** Tests for {@link CppLinkAction}. */
 @RunWith(JUnit4.class)
 public class CppLinkActionTest extends BuildViewTestCase {
   private RuleContext createDummyRuleContext() throws Exception {
@@ -66,6 +65,18 @@ public class CppLinkActionTest extends BuildViewTestCase {
           }
         }, masterConfig);
   }
+  
+  private final FeatureConfiguration getMockFeatureConfiguration() throws Exception {
+    return CcToolchainFeaturesTest.buildFeatures(
+            CppLinkActionConfigs.getCppLinkActionConfigs(CppLinkPlatform.LINUX))
+        .getFeatureConfiguration(
+            Link.LinkTargetType.EXECUTABLE.getActionName(),
+            Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.PIC_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY.getActionName(),
+            Link.LinkTargetType.ALWAYS_LINK_PIC_STATIC_LIBRARY.getActionName());
+  }
 
   @Test
   public void testToolchainFeatureFlags() throws Exception {
@@ -93,9 +104,9 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 "out",
                 ImmutableList.<Artifact>of(),
                 ImmutableList.<LibraryToLink>of(),
-                featureConfiguration)
+                featureConfiguration,
+                false)
             .build();
-    assertThat(Joiner.on(" ").join(linkAction.getArgv())).contains("mock_tool");
     assertThat(linkAction.getArgv()).contains("some_flag");
   }
 
@@ -118,7 +129,8 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 "out",
                 ImmutableList.<Artifact>of(),
                 ImmutableList.<LibraryToLink>of(),
-                featureConfiguration)
+                featureConfiguration,
+                false)
             .build();
     assertThat(linkAction.getEnvironment()).containsEntry("foo", "bar");
   }
@@ -135,6 +147,8 @@ public class CppLinkActionTest extends BuildViewTestCase {
     final Artifact oFile = getSourceArtifact("cc/a.o");
     final Artifact oFile2 = getSourceArtifact("cc/a2.o");
     final Artifact interfaceSoBuilder = getBinArtifactWithNoOwner("foo/build_interface_so");
+    final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration();
+
     ActionTester.runTest(
         128,
         new ActionCombinationFactory() {
@@ -158,7 +172,7 @@ public class CppLinkActionTest extends BuildViewTestCase {
             builder.setWholeArchive((i & 16) == 0);
             builder.setFake((i & 32) == 0);
             builder.setRuntimeSolibDir((i & 64) == 0 ? null : new PathFragment("so1"));
-            builder.setFeatureConfiguration(new FeatureConfiguration());
+            builder.setFeatureConfiguration(featureConfiguration);
 
             return builder.build();
           }
@@ -177,6 +191,8 @@ public class CppLinkActionTest extends BuildViewTestCase {
     final Artifact oFile = getSourceArtifact("cc/a.o");
     final Artifact oFile2 = getSourceArtifact("cc/a2.o");
     final Artifact interfaceSoBuilder = getBinArtifactWithNoOwner("foo/build_interface_so");
+    final FeatureConfiguration featureConfiguration = getMockFeatureConfiguration();
+
     ActionTester.runTest(
         4,
         new ActionCombinationFactory() {
@@ -194,6 +210,7 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 (i & 1) == 0 ? ImmutableList.of(oFile) : ImmutableList.of(oFile2));
             builder.setLinkType(
                 (i & 2) == 0 ? LinkTargetType.STATIC_LIBRARY : LinkTargetType.DYNAMIC_LIBRARY);
+            builder.setFeatureConfiguration(featureConfiguration);
             return builder.build();
           }
         });
@@ -254,7 +271,8 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 "binary2",
                 objects.build(),
                 ImmutableList.<LibraryToLink>of(),
-                new FeatureConfiguration())
+                new FeatureConfiguration(),
+                false)
             .setFake(true)
             .build();
 
@@ -285,7 +303,8 @@ public class CppLinkActionTest extends BuildViewTestCase {
       String outputPath,
       Iterable<Artifact> nonLibraryInputs,
       ImmutableList<LibraryToLink> libraryInputs,
-      FeatureConfiguration featureConfiguration)
+      FeatureConfiguration featureConfiguration,
+      boolean shouldIncludeToolchain)
       throws Exception {
     RuleContext ruleContext = createDummyRuleContext();
     CppLinkActionBuilder builder =
@@ -294,7 +313,7 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 new Artifact(
                     new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()),
                 ruleContext.getConfiguration(),
-                null)
+                shouldIncludeToolchain ? CppHelper.getToolchain(ruleContext) : null)
             .addNonLibraryInputs(nonLibraryInputs)
             .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs))
             .setLinkType(type)
@@ -306,6 +325,17 @@ public class CppLinkActionTest extends BuildViewTestCase {
             .setFeatureConfiguration(featureConfiguration);
     return builder;
   }
+  
+  private CppLinkActionBuilder createLinkBuilder(Link.LinkTargetType type) throws Exception {
+    PathFragment output = new PathFragment("dummyRuleContext/output/path.xyz");
+    return createLinkBuilder(
+        type,
+        output.getPathString(),
+        ImmutableList.<Artifact>of(),
+        ImmutableList.<LibraryToLink>of(),
+        new FeatureConfiguration(),
+        true);
+  }
 
   public Artifact getOutputArtifact(String relpath) {
     return new Artifact(
@@ -313,4 +343,70 @@ public class CppLinkActionTest extends BuildViewTestCase {
         getTargetConfiguration().getBinDirectory(),
         getTargetConfiguration().getBinFragment().getRelative(relpath));
   }
+
+  private Artifact scratchArtifact(String s) {
+    try {
+      return new Artifact(
+          scratch.overwriteFile(outputBase.getRelative("WORKSPACE").getRelative(s).toString()),
+          Root.asDerivedRoot(scratch.dir(outputBase.getRelative("WORKSPACE").toString())));
+    } catch (IOException e) {
+      throw new RuntimeException(e);
+    }
+  }
+
+  private void assertError(String expectedSubstring, CppLinkActionBuilder builder) {
+    try {
+      builder.build();
+      fail();
+    } catch (RuntimeException e) {
+      assertThat(e.getMessage()).contains(expectedSubstring);
+    }
+  }
+
+  @Test
+  public void testInterfaceOutputWithoutBuildingDynamicLibraryIsError() throws Exception {
+    CppLinkActionBuilder builder =
+        createLinkBuilder(LinkTargetType.EXECUTABLE)
+            .setInterfaceOutput(scratchArtifact("FakeInterfaceOutput"));
+
+    assertError("Interface output can only be used with non-fake DYNAMIC_LIBRARY targets", builder);
+  }
+
+  @Test
+  public void testStaticLinkWithDynamicIsError() throws Exception {
+    CppLinkActionBuilder builder =
+        createLinkBuilder(LinkTargetType.STATIC_LIBRARY).setLinkStaticness(LinkStaticness.DYNAMIC);
+
+    assertError("static library link must be static", builder);
+  }
+
+  @Test
+  public void testStaticLinkWithSymbolsCountOutputIsError() throws Exception {
+    CppLinkActionBuilder builder =
+        createLinkBuilder(LinkTargetType.STATIC_LIBRARY)
+            .setLinkStaticness(LinkStaticness.FULLY_STATIC)
+            .setSymbolCountsOutput(scratchArtifact("dummySymbolCounts"));
+
+    assertError("the symbol counts output must be null for static links", builder);
+  }
+
+  @Test
+  public void testStaticLinkWithNativeDepsIsError() throws Exception {
+    CppLinkActionBuilder builder =
+        createLinkBuilder(LinkTargetType.STATIC_LIBRARY)
+            .setLinkStaticness(LinkStaticness.FULLY_STATIC)
+            .setNativeDeps(true);
+
+    assertError("the native deps flag must be false for static links", builder);
+  }
+
+  @Test
+  public void testStaticLinkWithWholeArchiveIsError() throws Exception {
+    CppLinkActionBuilder builder =
+        createLinkBuilder(LinkTargetType.STATIC_LIBRARY)
+            .setLinkStaticness(LinkStaticness.FULLY_STATIC)
+            .setWholeArchive(true);
+
+    assertError("the need whole archive flag must be false for static links", builder);
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
new file mode 100644
index 0000000000..50fdebad0f
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariablesTest.java
@@ -0,0 +1,134 @@
+// Copyright 2016 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.rules.cpp;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests that {@code CppLinkAction} is populated with the correct build variables. */
+@RunWith(JUnit4.class)
+public class LinkBuildVariablesTest extends BuildViewTestCase {
+
+  private CppLinkAction getCppLinkAction(ConfiguredTarget target, Link.LinkTargetType type) {
+    Artifact linkerOutput = null;
+    switch (type) {
+      case STATIC_LIBRARY:
+      case ALWAYS_LINK_STATIC_LIBRARY:
+        linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + ".a", target);
+        break;
+      case PIC_STATIC_LIBRARY:
+      case ALWAYS_LINK_PIC_STATIC_LIBRARY:
+        linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + "pic.a", target);
+        break;
+      case DYNAMIC_LIBRARY:
+        linkerOutput = getBinArtifact("lib" + target.getLabel().getName() + ".so", target);
+        break;
+      case EXECUTABLE:
+        linkerOutput = getExecutable(target);
+        break;
+      default:
+        throw new IllegalArgumentException(
+            String.format("Cannot get CppLinkAction for link type %s", type));
+    }
+    return (CppLinkAction) getGeneratingAction(linkerOutput);
+  }
+
+  private Variables getLinkBuildVariables(ConfiguredTarget target, Link.LinkTargetType type) {
+    return getCppLinkAction(target, type).getLinkCommandLine().getBuildVariables();
+  }
+
+  private List<String> getVariableValue(Variables variables, String variable) throws Exception {
+    FeatureConfiguration mockFeatureConfiguration =
+        CcToolchainFeaturesTest.buildFeatures(
+                "feature {",
+                "   name: 'a'",
+                "   flag_set {",
+                "   action: 'foo'",
+                "      flag_group {",
+                "         flag: '%{" + variable + "}'",
+                "      }",
+                "   }",
+                "}")
+            .getFeatureConfiguration("a");
+    return mockFeatureConfiguration.getCommandLine("foo", variables);
+  }
+
+  @Test
+  public void testLinkstampBuildVariable() throws Exception {
+    scratch.file(
+        "x/BUILD",
+        "cc_binary(",
+        "   name = 'bin',",
+        "   srcs = ['a.cc'],",
+        "   deps = [':lib'],",
+        ")",
+        "cc_library(",
+        "   name = 'lib',",
+        "   srcs = ['b.cc'],",
+        "   linkstamp = 'c.cc',",
+        ")");
+    scratch.file("x/a.cc");
+    scratch.file("x/b.cc");
+    scratch.file("x/c.cc");
+
+    ConfiguredTarget target = getConfiguredTarget("//x:bin");
+    Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
+    List<String> variableValue =
+        getVariableValue(variables, CppLinkActionBuilder.LINKSTAMP_PATHS_VARIABLE);
+    assertThat(Iterables.getOnlyElement(variableValue)).contains("c.o");
+  }
+
+  @Test
+  public void testForcePicBuildVariable() throws Exception {
+    useConfiguration("--force_pic");
+    scratch.file("x/BUILD", "cc_binary(", "   name = 'bin',", "   srcs = ['a.cc'],", ")");
+    scratch.file("x/a.cc");
+
+    ConfiguredTarget target = getConfiguredTarget("//x:bin");
+    Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
+    List<String> variableValue =
+        getVariableValue(variables, CppLinkActionBuilder.FORCE_PIC_VARIABLE);
+    assertThat(variableValue).contains("");
+  }
+
+  @Test
+  public void testWholeArchiveBuildVariables() throws Exception {
+    scratch.file(
+        "x/BUILD",
+        "cc_binary(",
+        "   name = 'bin.so',",
+        "   srcs = ['a.cc'],",
+        "   linkopts = ['-shared'],",
+        "   linkstatic = 1",
+        ")");
+    scratch.file("x/a.cc");
+
+    ConfiguredTarget target = getConfiguredTarget("//x:bin.so");
+    Variables variables = getLinkBuildVariables(target, Link.LinkTargetType.EXECUTABLE);
+    List<String> variableValue =
+        getVariableValue(variables, CppLinkActionBuilder.GLOBAL_WHOLE_ARCHIVE_VARIABLE);
+    assertThat(variableValue).contains("");
+  }
+}