Allow CommandLine expansion to throw an exception.

This paves the way for Skylark-side compact command lines that can fail during expansion.

In general, expansion happens in these scenarios:

* Action execution expands the command line to execute it. This is fine since we are well equipped already to handle failing actions.

* In the analysis phase we expand command lines to investigate whether we need a params file. This could be moved to the execution phase, which would have the benefit of getting params files out of the action graph and saving memory.

* Key computation expands the command line. This could be fixed by allowing command lines to compute their own keys (which wouldn't try to expand the command line). This could have the benefit of being more efficient.

* Extra actions expand the command line to construct the extra action proto. This could maybe be deferred to the execution phase (writing the extra action), which would also be more memory efficient.

For failed key computations, we return a singleton "error" key. This means multiple actions that will fail will map to the same key. These actions will necessarily fail if executed, so we should not need to worry about these ending up in the action cache. If we do manage to make the command line compute its own keys then this will become moot in the future.

RELNOTES: None
PiperOrigin-RevId: 166266385

2 files changed, 33 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
index dc7326d56e..3b09fc991f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
@@ -44,6 +44,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
+import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.analysis.OutputGroupProvider;
 import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
@@ -590,9 +591,17 @@ public class LegacyCompilationSupport extends CompilationSupport {
   }
 
   private StrippingType getStrippingType(CommandLine commandLine) {
-    return Iterables.contains(commandLine.arguments(), "-dynamiclib")
-        ? StrippingType.DYNAMIC_LIB
-        : StrippingType.DEFAULT;
+    try {
+      return Iterables.contains(commandLine.arguments(), "-dynamiclib")
+          ? StrippingType.DYNAMIC_LIB
+          : StrippingType.DEFAULT;
+    } catch (CommandLineExpansionException e) {
+      // TODO(b/64941219): This code should be rewritten to not expand the command line
+      // in the analysis phase
+      // This can't actually happen, because the command lines used by this class do
+      // not throw.
+      throw new AssertionError("Cannot fail to expand command line but did.", e);
+    }
   }
 
   private void registerLinkAction(
@@ -789,7 +798,7 @@ public class LegacyCompilationSupport extends CompilationSupport {
     }
 
     @Override
-    public Iterable<String> arguments() {
+    public Iterable<String> arguments() throws CommandLineExpansionException {
       return ImmutableList.of(Joiner.on(' ').join(original.arguments()));
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
index dc8e89d56b..bfa06e21f5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactResolver;
+import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
@@ -78,8 +79,8 @@ public class ObjcCompileAction extends SpawnAction {
    */
   public class ObjcCompileActionSpawn extends ActionSpawn {
 
-    public ObjcCompileActionSpawn(Map<String, String> clientEnv) {
-      super(clientEnv);
+    public ObjcCompileActionSpawn(ImmutableList<String> arguments, Map<String, String> clientEnv) {
+      super(arguments, clientEnv);
     }
 
     @Override
@@ -172,7 +173,12 @@ public class ObjcCompileAction extends SpawnAction {
 
   @Override
   public final Spawn getSpawn(Map<String, String> clientEnv) {
-    return new ObjcCompileActionSpawn(clientEnv);
+    try {
+      return new ObjcCompileActionSpawn(
+          ImmutableList.copyOf(getCommandLine().arguments()), clientEnv);
+    } catch (CommandLineExpansionException e) {
+      throw new AssertionError("ObjcCompileAction command line expansion cannot fail");
+    }
   }
 
   @Override
@@ -296,7 +302,12 @@ public class ObjcCompileAction extends SpawnAction {
 
   @Override
   protected SpawnInfo getExtraActionSpawnInfo() {
-    SpawnInfo.Builder info = SpawnInfo.newBuilder(super.getExtraActionSpawnInfo());
+    SpawnInfo.Builder info = null;
+    try {
+      info = SpawnInfo.newBuilder(super.getExtraActionSpawnInfo());
+    } catch (CommandLineExpansionException e) {
+      throw new AssertionError("ObjcCompileAction command line expansion cannot fail");
+    }
     if (!inputsDiscovered()) {
       for (Artifact headerArtifact : filterHeaderFiles()) {
         // As in SpawnAction#getExtraActionSpawnInfo explicitly ignore middleman artifacts here.
@@ -312,7 +323,11 @@ public class ObjcCompileAction extends SpawnAction {
   public String computeKey() {
     Fingerprint f = new Fingerprint();
     f.addString(GUID);
-    f.addString(super.computeKey());
+    try {
+      f.addString(super.computeKey());
+    } catch (CommandLineExpansionException e) {
+      throw new AssertionError("ObjcCompileAction command line expansion cannot fail");
+    }
     f.addBoolean(dotdFile == null || dotdFile.artifact() == null);
     f.addBoolean(dotdPruningPlan == HeaderDiscovery.DotdPruningMode.USE);
     f.addBoolean(headersListFile == null);