Use tool from action_config for link-executable and link-dynamic-lib actions

hlopko · philwo · commit a81264e1043d · 2017-06-21T14:48:49.000+02:00
This cl finishes the last bit of c++ linking actions migration to crosstool's
action_configs. From now on, the action_config { tool_path: ... } will be used,
instead of top level tool { path: ... }.

RELNOTES: Bazel now uses tools from action_configs in Crosstool by default (as
oposed to using top level tools).
PiperOrigin-RevId: 159677525
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
@@ -118,7 +118,7 @@ public static String getCppActionConfigs(
                 "  config_name: 'c++-link-executable'",
                 "  action_name: 'c++-link-executable'",
                 "  tool {",
-                "    tool_path: 'DUMMY_TOOL'",
+                "    tool_path: '" + gccToolPath + "'",
                 "  }",
                 "  implies: 'symbol_counts'",
                 "  implies: 'strip_debug_symbols'",
@@ -136,7 +136,7 @@ public static String getCppActionConfigs(
                 "  config_name: 'c++-link-dynamic-library'",
                 "  action_name: 'c++-link-dynamic-library'",
                 "  tool {",
-                "    tool_path: 'DUMMY_TOOL'",
+                "    tool_path: '" + gccToolPath + "'",
                 "  }",
                 "  implies: 'build_interface_libraries'",
                 "  implies: 'dynamic_library_linker_tool'",
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -755,9 +755,15 @@ public CppLinkAction build() throws InterruptedException {
             .setToolchain(toolchain)
             .setFdoSupport(fdoSupport.getFdoSupport())
             .setBuildVariables(buildVariables)
-            .setToolPath(getToolPath())
             .setFeatureConfiguration(featureConfiguration);
 
+    // TODO(b/62693279): Cleanup once internal crosstools specify ifso building correctly.
+    if (linkType.equals(LinkTargetType.DYNAMIC_LIBRARY)
+        && !featureConfiguration.hasConfiguredLinkerPathInActionConfig()) {
+      linkCommandLineBuilder.forceToolPath(
+          toolchain.getLinkDynamicLibraryTool().getExecPathString());
+    }
+
     if (!isLTOIndexing) {
       linkCommandLineBuilder
           .setOutput(output)
@@ -889,26 +895,6 @@ public CppLinkAction build() throws InterruptedException {
         executionRequirements.build());
   }
 
-  /**
-   * Returns the tool path from feature configuration, if the tool in the configuration is sane, or
-   * builtin tool, if configuration has a dummy value.
-   */
-  private String getToolPath() {
-    if (!featureConfiguration.actionIsConfigured(linkType.getActionName())) {
-      return null;
-    }
-    String toolPath =
-        featureConfiguration
-            .getToolForAction(linkType.getActionName())
-            .getToolPath(cppConfiguration.getCrosstoolTopPathFragment())
-            .getPathString();
-    if (linkType.equals(LinkTargetType.DYNAMIC_LIBRARY)
-        && !featureConfiguration.hasConfiguredLinkerPathInActionConfig()) {
-      toolPath = toolchain.getLinkDynamicLibraryTool().getExecPathString();
-    }
-    return toolPath;
-  }
-
   /** The default heuristic on whether we need to use whole-archive for the link. */
   private static boolean needWholeArchive(
       LinkStaticness staticness,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
@@ -53,7 +53,7 @@
 @Immutable
 public final class LinkCommandLine extends CommandLine {
   private final String actionName;
-  private final String toolPath;
+  private final String forcedToolPath;
   private final boolean codeCoverageEnabled;
   private final CppConfiguration cppConfiguration;
   private final ActionOwner owner;
@@ -80,7 +80,7 @@ public final class LinkCommandLine extends CommandLine {
 
   private LinkCommandLine(
       String actionName,
-      String toolPath,
+      String forcedToolPath,
       BuildConfiguration configuration,
       ActionOwner owner,
       Artifact output,
@@ -103,7 +103,7 @@ private LinkCommandLine(
       CcToolchainProvider ccProvider) {
 
     this.actionName = actionName;
-    this.toolPath = toolPath;
+    this.forcedToolPath = forcedToolPath;
     this.codeCoverageEnabled = configuration.isCodeCoverageEnabled();
     this.cppConfiguration = configuration.getFragment(CppConfiguration.class);
     this.variables = variables;
@@ -320,6 +320,9 @@ public static void extractArgumentsForDynamicLinkParamFile(
   }
   
   private ImmutableList<String> getToolchainFlags() {
+    if (Staticness.STATIC.equals(linkTargetType.staticness())) {
+      return ImmutableList.of();
+    }
     boolean fullyStatic = (linkStaticness == LinkStaticness.FULLY_STATIC);
     boolean mostlyStatic = (linkStaticness == LinkStaticness.MOSTLY_STATIC);
     boolean sharedLinkopts =
@@ -379,52 +382,23 @@ private ImmutableList<String> getToolchainFlags() {
    */
   public List<String> getRawLinkArgv() {
     List<String> argv = new ArrayList<>();
-
-    // TODO(b/30109612): Extract this switch into individual crosstools once action configs are no
-    // longer hardcoded in CppActionConfigs.
-    switch (linkTargetType) {
-      case EXECUTABLE:
-        argv.add(cppConfiguration.getCppExecutable().getPathString());
-        argv.addAll(
-            featureConfiguration.getCommandLine(
-                actionName,
-                new Variables.Builder()
-                    .addAll(variables)
-                    .addStringSequenceVariable(
-                        CppLinkActionBuilder.LEGACY_LINK_FLAGS_VARIABLE, getToolchainFlags())
-                    .build()));
-        break;
-
-      case STATIC_LIBRARY:
-      case PIC_STATIC_LIBRARY:
-      case ALWAYS_LINK_STATIC_LIBRARY:
-      case ALWAYS_LINK_PIC_STATIC_LIBRARY:
-        argv.add(toolPath);
-        argv.addAll(featureConfiguration.getCommandLine(actionName, variables));
-        break;
-
-      // Since the objc case/dynamic libs is not hardcoded in CppConfiguration, we can use the
-      // actual tool.
-      // TODO(b/30109612): make this pattern the case for all link variants.
-      case DYNAMIC_LIBRARY:
-      case OBJC_ARCHIVE:
-      case OBJC_FULLY_LINKED_ARCHIVE:
-      case OBJC_EXECUTABLE:
-      case OBJCPP_EXECUTABLE:
-        argv.add(toolPath);
-        argv.addAll(
-            featureConfiguration.getCommandLine(
-                actionName,
-                new Variables.Builder()
-                    .addAll(variables)
-                    .addStringSequenceVariable(
-                        CppLinkActionBuilder.LEGACY_LINK_FLAGS_VARIABLE, getToolchainFlags())
-                    .build()));
-        break;
-
-      default:
-        throw new IllegalArgumentException();
-    }
+    if (forcedToolPath != null) {
+      argv.add(forcedToolPath);
+    } else {
+      argv.add(
+          featureConfiguration
+              .getToolForAction(linkTargetType.getActionName())
+              .getToolPath(cppConfiguration.getCrosstoolTopPathFragment())
+              .getPathString());
+    }
+    argv.addAll(
+        featureConfiguration.getCommandLine(
+            actionName,
+            new Variables.Builder()
+                .addAll(variables)
+                .addStringSequenceVariable(
+                    CppLinkActionBuilder.LEGACY_LINK_FLAGS_VARIABLE, getToolchainFlags())
+                .build()));
     return argv;
   }
 
@@ -635,7 +609,7 @@ public static final class Builder {
     private final ActionOwner owner;
     private final RuleContext ruleContext;
 
-    @Nullable private String toolPath;
+    private String forcedToolPath;
     @Nullable private Artifact output;
     private ImmutableList<Artifact> buildInfoHeaderArtifacts = ImmutableList.of();
     private Iterable<? extends LinkerInput> linkerInputs = ImmutableList.of();
@@ -714,7 +688,7 @@ public LinkCommandLine build() {
 
       return new LinkCommandLine(
           actionName,
-          toolPath,
+          forcedToolPath,
           configuration,
           owner,
           output,
@@ -751,9 +725,9 @@ public Builder setFdoSupport(FdoSupport fdoSupport) {
       return this;
     }
 
-    /** Sets the tool path, with tool being the first thing on the command line */
-    public Builder setToolPath(String toolPath) {
-      this.toolPath = toolPath;
+    /** Use given tool path instead of the one from feature configuration */
+    public Builder forceToolPath(String forcedToolPath) {
+      this.forcedToolPath = forcedToolPath;
       return this;
     }
 
diff --git a/tools/osx/crosstool/CROSSTOOL.tpl b/tools/osx/crosstool/CROSSTOOL.tpl
