Put tool path into Starlark args for C++ actions

In e7ce105 I missed that the args should also contain the compiler/linker path in the args. This CL adds it. RELNOTES: None. PiperOrigin-RevId: 283738466
bazelbuild · Dec 4, 2019 · b78ef52 · b78ef52
1 parent 701e465
commit b78ef52
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 9 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
@@ -116,7 +116,8 @@ public CommandLine getFilteredFeatureConfigurationCommandLine(CppCompileAction c
       @Override
       public Iterable<String> arguments() throws CommandLineExpansionException {
         CcToolchainVariables overwrittenVariables = cppCompileAction.getOverwrittenVariables();
-        return ImmutableList.copyOf(getCompilerOptions(overwrittenVariables));
+        List<String> compilerOptions = getCompilerOptions(overwrittenVariables);
+        return ImmutableList.<String>builder().add(getToolPath()).addAll(compilerOptions).build();
       }
     };
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FeatureConfigurationCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FeatureConfigurationCommandLine.java
@@ -39,6 +39,7 @@ public class FeatureConfigurationCommandLine extends CommandLine {
   private final CcToolchainVariables buildVariables;
   private final String actionName;
   private final boolean usePic;
+  private final boolean inLegacyThinltoBackendMode;
 
   /**
    * Create a {@link CommandLine} instance from given {@link FeatureConfiguration} and {@link
@@ -49,7 +50,11 @@ public static FeatureConfigurationCommandLine from(
       CcToolchainVariables ccToolchainVariables,
       String actionName) {
     return new FeatureConfigurationCommandLine(
-        featureConfiguration, ccToolchainVariables, actionName, /* usePic= */ false);
+        featureConfiguration,
+        ccToolchainVariables,
+        actionName,
+        /* inLegacyThinltoBackendMode= */ false,
+        /* usePic= */ false);
   }
 
   /**
@@ -65,18 +70,24 @@ public static FeatureConfigurationCommandLine forLtoBackendAction(
       CcToolchainVariables ccToolchainVariables,
       boolean usePic) {
     return new FeatureConfigurationCommandLine(
-        featureConfiguration, ccToolchainVariables, CppActionNames.LTO_BACKEND, usePic);
+        featureConfiguration,
+        ccToolchainVariables,
+        CppActionNames.LTO_BACKEND,
+        /* inLegacyThinltoBackendMode= */ true,
+        usePic);
   }
 
   private FeatureConfigurationCommandLine(
       FeatureConfiguration featureConfiguration,
       CcToolchainVariables ccToolchainVariables,
       String actionName,
+      boolean inLegacyThinltoBackendMode,
       boolean usePic) {
     this.featureConfiguration = featureConfiguration;
     this.buildVariables = ccToolchainVariables;
     this.actionName = actionName;
     this.usePic = usePic;
+    this.inLegacyThinltoBackendMode = inLegacyThinltoBackendMode;
   }
   @Override
   public Iterable<String> arguments() throws CommandLineExpansionException {
@@ -87,6 +98,9 @@ public Iterable<String> arguments() throws CommandLineExpansionException {
   public Iterable<String> arguments(ArtifactExpander artifactExpander)
       throws CommandLineExpansionException {
     ImmutableList.Builder<String> args = ImmutableList.builder();
+    if (!inLegacyThinltoBackendMode) {
+      args.add(featureConfiguration.getToolPathForAction(actionName));
+    }
     try {
       args.addAll(
           featureConfiguration.getCommandLine(actionName, buildVariables, artifactExpander));

diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh
@@ -677,7 +677,7 @@ tree_art_rule = rule(implementation = _tree_art_impl,
             default = "//${package}:write.sh")})
 
 def _actions_test_impl(target, ctx):
-    action = target.actions[1] # digest action
+    action = target.actions[1]
     if action.mnemonic != "CppLink":
       fail("Expected the second action to be CppLink.")
     aspect_out = ctx.actions.declare_file('aspect_out')
@@ -705,8 +705,11 @@ EOF
       --aspects="//${package}:lib.bzl%actions_test_aspect" \
       --output_groups=out --experimental_action_args
 
-  cat "${PRODUCT_NAME}-bin/${package}/aspect_out" | grep "a.*o .*b.*o .*c.*o" \
-      || fail "aspect Args do not contain tree artifact args"
+  cat "bazel-bin/${package}/aspect_out" | grep "\(ar\|libtool\)" \
+      || fail "args didn't contain the tool path"
+
+  cat "bazel-bin/${package}/aspect_out" | grep "a.*o .*b.*o .*c.*o" \
+      || fail "args didn't contain tree artifact paths"
 }
 
 function test_directory_arg_compile_action() {
@@ -715,7 +718,7 @@ function test_directory_arg_compile_action() {
 
   cat > "${package}/lib.bzl" <<EOF
 def _actions_test_impl(target, ctx):
-    action = target.actions[0] # digest action
+    action = target.actions[0]
     if action.mnemonic != "CppCompile":
       fail("Expected the first action to be CppCompile.")
     aspect_out = ctx.actions.declare_file('aspect_out')
@@ -740,8 +743,12 @@ EOF
       --aspects="//${package}:lib.bzl%actions_test_aspect" \
       --output_groups=out --experimental_action_args
 
-  cat "${PRODUCT_NAME}-bin/${package}/aspect_out" | grep "a.*o .*b.*o .*c.*o" \
-      || fail "aspect Args do not contain tree artifact args"
+  cat "bazel-bin/${package}/aspect_out" | \
+      grep "\(gcc\|clang\|clanc-cl.exe\|cl.exe\)" \
+      || fail "args didn't contain the tool path"
+
+  cat "bazel-bin/${package}/aspect_out" | grep "a.*o .*b.*o .*c.*o" \
+      || fail "args didn't contain tree artifact paths"
 }
 
 run_suite "cc_integration_test"