Use feature configuration to construct command line for 'strip' action

This cl introduces new action_config named 'strip' for the strip action. While
at it, it fixes support for executionRequirements.

Fixed #209

RELNOTES: 'strip' action is now configured via feature configuration
PiperOrigin-RevId: 164110478

7 files changed, 102 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index 12e660fe47..b0c1a42921 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -332,7 +332,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     Artifact strippedFile = ruleContext.getImplicitOutputArtifact(
         CppRuleClasses.CC_BINARY_STRIPPED);
     CppHelper.createStripAction(
-        ruleContext, ccToolchain, cppConfiguration, executable, strippedFile);
+        ruleContext, ccToolchain, cppConfiguration, executable, strippedFile, featureConfiguration);
 
     DwoArtifactsCollector dwoArtifacts =
         collectTransitiveDwoArtifacts(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index dfc3e7200c..690de23037 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -104,6 +104,7 @@ public final class CcLibraryHelper {
             CppFileTypes.ASSEMBLER_WITH_C_PREPROCESSOR),
         ImmutableSet.<String>of(
             CppCompileAction.CC_FLAGS_MAKE_VARIABLE_ACTION_NAME,
+            CppCompileAction.STRIP_ACTION_NAME,
             CppCompileAction.C_COMPILE,
             CppCompileAction.CPP_COMPILE,
             CppCompileAction.CPP_HEADER_PARSING,
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
index e1669ddca7..0922c08800 100644
--- 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
@@ -37,6 +37,7 @@ public class CppActionConfigs {
       String gccToolPath,
       String cppLinkDynamicLibraryToolPath,
       String arToolPath,
+      String stripToolPath,
       boolean supportsEmbeddedRuntimes,
       boolean supportsInterfaceSharedLibraries) {
     String cppDynamicLibraryLinkerTool = "";
@@ -843,6 +844,46 @@ public class CppActionConfigs {
                 "      flag: '%{copts}'",
                 "    }",
                 "  }",
+                "}",
+                "action_config {",
+                "  config_name: 'strip'",
+                "  action_name: 'strip'",
+                "  tool {",
+                "    tool_path: '" + stripToolPath + "'",
+                "  }",
+                "  flag_set {",
+                "    flag_group {",
+                "      flag: '-S'",
+                ifLinux(platform, "flag: '-p'"),
+                "      flag: '-o'",
+                "      flag: '%{output_file}'",
+                "    }",
+                "    flag_group {",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.quote_paths'",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.bracket_paths'",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.system_paths'",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.cpp_defines'",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.cpp_includes'",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.cl_args'",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.lipo_info'",
+                "      flag: '-R'",
+                "      flag: '.gnu.switches.text.annotation'",
+                "    }",
+                "    flag_group {",
+                "      iterate_over: 'stripopts'",
+                "      flag: '%{stripopts}'",
+                "    }",
+                "    flag_group {",
+                "      flag: '%{input_file}'",
+                "    }",
+                "  }",
                 "}"));
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index b4cc77960c..2862ade4e6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -121,6 +121,9 @@ public class CppCompileAction extends AbstractAction
   public static final java.lang.String CC_FLAGS_MAKE_VARIABLE_ACTION_NAME =
       "cc-flags-make-variable";
 
+  /** A string constant for the strip action name. */
+  public static final String STRIP_ACTION_NAME = "strip";
+
   /**
    * A string constant for the c compilation action.
    */
@@ -438,7 +441,8 @@ public class CppCompileAction extends AbstractAction
     return discoversInputs;
   }
 
-  @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
+  @Override
+  @VisibleForTesting // productionVisibility = Visibility.PRIVATE
   public Iterable<Artifact> getPossibleInputsForTesting() {
     return Iterables.concat(getInputs(), prunableInputs);
   }
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 11f05ac28f..5fb4589bb1 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
@@ -641,6 +641,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
         String gccToolPath = "DUMMY_GCC_TOOL";
         String linkerToolPath = "DUMMY_LINKER_TOOL";
         String arToolPath = "DUMMY_AR_TOOL";
+        String stripToolPath = "DUMMY_STRIP_TOOL";
         for (ToolPath tool : toolchain.getToolPathList()) {
           if (tool.getName().equals(Tool.GCC.getNamePart())) {
             gccToolPath = tool.getPath();
@@ -652,6 +653,9 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
           if (tool.getName().equals(Tool.AR.getNamePart())) {
             arToolPath = tool.getPath();
           }
+          if (tool.getName().equals(Tool.STRIP.getNamePart())) {
+            stripToolPath = tool.getPath();
+          }
         }
         TextFormat.merge(
             CppActionConfigs.getCppActionConfigs(
@@ -660,9 +664,10 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
                 gccToolPath,
                 linkerToolPath,
                 arToolPath,
+                stripToolPath,
                 supportsEmbeddedRuntimes,
                 toolchain.getSupportsInterfaceSharedObjects()),
-              toolchainBuilder);
+            toolchainBuilder);
       } catch (ParseException e) {
         // Can only happen if we change the proto definition without changing our
         // configuration above.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
index 47d0964d2b..8e2b7ed94c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -18,7 +18,9 @@ import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MiddlemanFactory;
@@ -40,6 +42,9 @@ import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.RuleErrorConsumer;
 import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier;
 import com.google.devtools.build.lib.rules.cpp.CcLinkParams.Linkstamp;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Tool;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppCompilationContext.Builder;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.shell.ShellUtils;
@@ -601,32 +606,49 @@ public class CppHelper {
         .getChild(configuration.getGenfilesFragment().getBaseName());
   }
 
-  /**
-   * Creates an action to strip an executable.
-   */
-  public static void createStripAction(RuleContext context, CcToolchainProvider toolchain,
-      CppConfiguration cppConfiguration, Artifact input, Artifact output) {
-    context.registerAction(
+  /** Creates an action to strip an executable. */
+  public static void createStripAction(
+      RuleContext context,
+      CcToolchainProvider toolchain,
+      CppConfiguration cppConfiguration,
+      Artifact input,
+      Artifact output,
+      FeatureConfiguration featureConfiguration) {
+    if (!featureConfiguration.actionIsConfigured(CppCompileAction.STRIP_ACTION_NAME)) {
+      context.ruleError("Expected action_config for 'strip' to be configured.");
+      return;
+    }
+
+    Tool stripTool =
+        Preconditions.checkNotNull(
+            featureConfiguration.getToolForAction(CppCompileAction.STRIP_ACTION_NAME));
+    Variables variables =
+        new Variables.Builder()
+            .addStringVariable(CppModel.OUTPUT_FILE_VARIABLE_NAME, output.getExecPathString())
+            .addStringSequenceVariable(
+                CppModel.STRIPOPTS_VARIABLE_NAME, cppConfiguration.getStripOpts())
+            .addStringVariable(CppModel.INPUT_FILE_VARIABLE_NAME, input.getExecPathString())
+            .build();
+    ImmutableList<String> commandLine =
+        ImmutableList.copyOf(
+            featureConfiguration.getCommandLine(CppCompileAction.STRIP_ACTION_NAME, variables));
+    ImmutableMap.Builder<String, String> executionInfoBuilder = ImmutableMap.builder();
+    for (String executionRequirement : stripTool.getExecutionRequirements()) {
+      executionInfoBuilder.put(executionRequirement, "");
+    }
+    Action[] stripAction =
         new SpawnAction.Builder()
             .addInput(input)
             .addTransitiveInputs(toolchain.getStrip())
             .addOutput(output)
             .useDefaultShellEnvironment()
-            .setExecutable(cppConfiguration.getStripExecutable())
-            .addArguments("-S", "-p", "-o", output.getExecPathString())
-            .addArguments("-R", ".gnu.switches.text.quote_paths")
-            .addArguments("-R", ".gnu.switches.text.bracket_paths")
-            .addArguments("-R", ".gnu.switches.text.system_paths")
-            .addArguments("-R", ".gnu.switches.text.cpp_defines")
-            .addArguments("-R", ".gnu.switches.text.cpp_includes")
-            .addArguments("-R", ".gnu.switches.text.cl_args")
-            .addArguments("-R", ".gnu.switches.text.lipo_info")
-            .addArguments("-R", ".gnu.switches.text.annotation")
-            .addArguments(cppConfiguration.getStripOpts())
-            .addArgument(input.getExecPathString())
-            .setProgressMessage("Stripping %s for %s", output.prettyPrint(), context.getLabel())
+            .setExecutable(stripTool.getToolPath(cppConfiguration.getCrosstoolTopPathFragment()))
+            .addArguments(commandLine)
+            .setExecutionInfo(executionInfoBuilder.build())
+            .setProgressMessage("Stripping " + output.prettyPrint() + " for " + context.getLabel())
             .setMnemonic("CcStrip")
-            .build(context));
+            .build(context);
+    context.registerAction(stripAction);
   }
 
   public static void maybeAddStaticLinkMarkerProvider(RuleConfiguredTargetBuilder builder,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 1ca69d798e..7bd76742af 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -66,6 +66,9 @@ public final class CppModel {
   /** Name of the build variable for the path to the source file being compiled. */
   public static final String SOURCE_FILE_VARIABLE_NAME = "source_file";
 
+  /** Name of the build variable for the path to the input file being processed. */
+  public static final String INPUT_FILE_VARIABLE_NAME = "input_file";
+
   /** Name of the build variable for the path to the compilation output file. */
   public static final String OUTPUT_FILE_VARIABLE_NAME = "output_file";
 
@@ -137,6 +140,9 @@ public final class CppModel {
   /** Build variable for all user provided copt flags. */
   public static final String COPTS_VARIABLE_VALUE = "copts";
 
+  /** Name of the build variable for stripopts for the strip action. */
+  public static final String STRIPOPTS_VARIABLE_NAME = "stripopts";
+
   private final CppSemantics semantics;
   private final RuleContext ruleContext;
   private final BuildConfiguration configuration;