Only process executable path for path mapping

This fixes a few issues introduced in 282ac62:
* Only the executable path in the first argument of a `CommandLines` instance should be subject to path mapping.
* String arguments that are not normalized as path strings and thus can't possibly be the optimized memory representation used for the executable arg by `StarlarkAction` should not be modified.

Also removes an class that became unused in that commit.

Closes bazelbuild#24576.

PiperOrigin-RevId: 703536848
Change-Id: I358696bc268ab1f400798ef55ea4db8d556f07dd

src/main/java/com/google/devtools/build/lib/actions/CommandLines.java

@@ -39,7 +39,7 @@
  * <p>This class is used by {@link com.google.devtools.build.lib.exec.SpawnRunner} implementations
  * to expand the command lines into a master argument list + any param files needed to be written.
  */
-public abstract class CommandLines {
+public abstract sealed class CommandLines {
 
   // A (hopefully) conservative estimate of how much long each param file arg would be
   // eg. the length of '@path/to/param_file'.
@@ -381,8 +381,18 @@ public CommandLines build() {
     }
   }
 
-  private static CommandLine toCommandLine(Object obj) {
-    return obj instanceof CommandLine ? (CommandLine) obj : new SingletonCommandLine(obj);
+  private static CommandLine toExecutableCommandLine(Object obj) {
+    return toCommandLine(obj, /* hasExecutablePath= */ true);
+  }
+
+  private static CommandLine toNonExecutableCommandLine(Object obj) {
+    return toCommandLine(obj, /* hasExecutablePath= */ false);
+  }
+
+  private static CommandLine toCommandLine(Object obj, boolean hasExecutablePath) {
+    return obj instanceof CommandLine commandLine
+        ? commandLine
+        : new SingletonCommandLine(obj, hasExecutablePath);
   }
 
   private static final class OnePartCommandLines extends CommandLines {
@@ -394,7 +404,8 @@ private static final class OnePartCommandLines extends CommandLines {
 
     @Override
     public ImmutableList<CommandLineAndParamFileInfo> unpack() {
-      return ImmutableList.of(new CommandLineAndParamFileInfo(toCommandLine(part1), null));
+      return ImmutableList.of(
+          new CommandLineAndParamFileInfo(toExecutableCommandLine(part1), null));
     }
   }
 
@@ -412,8 +423,8 @@ private static final class TwoPartCommandLines extends CommandLines {
     @Override
     public ImmutableList<CommandLineAndParamFileInfo> unpack() {
       return ImmutableList.of(
-          new CommandLineAndParamFileInfo(toCommandLine(part1), null),
-          new CommandLineAndParamFileInfo(toCommandLine(part2), part2ParamFileInfo));
+          new CommandLineAndParamFileInfo(toExecutableCommandLine(part1), null),
+          new CommandLineAndParamFileInfo(toNonExecutableCommandLine(part2), part2ParamFileInfo));
     }
   }
 
@@ -431,9 +442,9 @@ private static final class ThreePartCommandLinesWithoutParamsFiles extends Comma
     @Override
     public ImmutableList<CommandLineAndParamFileInfo> unpack() {
       return ImmutableList.of(
-          new CommandLineAndParamFileInfo(toCommandLine(part1), null),
-          new CommandLineAndParamFileInfo(toCommandLine(part2), null),
-          new CommandLineAndParamFileInfo(toCommandLine(part3), null));
+          new CommandLineAndParamFileInfo(toExecutableCommandLine(part1), null),
+          new CommandLineAndParamFileInfo(toNonExecutableCommandLine(part2), null),
+          new CommandLineAndParamFileInfo(toNonExecutableCommandLine(part3), null));
     }
   }
 
@@ -460,9 +471,9 @@ private static final class ThreePartCommandLines extends CommandLines {
     @Override
     public ImmutableList<CommandLineAndParamFileInfo> unpack() {
       return ImmutableList.of(
-          new CommandLineAndParamFileInfo(toCommandLine(part1), null),
-          new CommandLineAndParamFileInfo(toCommandLine(part2), part2ParamFileInfo),
-          new CommandLineAndParamFileInfo(toCommandLine(part3), part3ParamFileInfo));
+          new CommandLineAndParamFileInfo(toExecutableCommandLine(part1), null),
+          new CommandLineAndParamFileInfo(toNonExecutableCommandLine(part2), part2ParamFileInfo),
+          new CommandLineAndParamFileInfo(toNonExecutableCommandLine(part3), part3ParamFileInfo));
     }
   }
 
@@ -496,7 +507,7 @@ public ImmutableList<CommandLineAndParamFileInfo> unpack() {
             paramFileInfo = (ParamFileInfo) commandLines[++i];
           }
         } else {
-          commandLine = new SingletonCommandLine(obj);
+          commandLine = new SingletonCommandLine(obj, /* hasExecutablePath= */ i == 0);
         }
 
         result.add(new CommandLineAndParamFileInfo(commandLine, paramFileInfo));
@@ -507,13 +518,15 @@ public ImmutableList<CommandLineAndParamFileInfo> unpack() {
 
   private static class SingletonCommandLine extends AbstractCommandLine {
     private final Object arg;
+    private final boolean hasExecutablePath;
 
-    SingletonCommandLine(Object arg) {
+    SingletonCommandLine(Object arg, boolean hasExecutablePath) {
       this.arg = arg;
+      this.hasExecutablePath = hasExecutablePath;
     }
 
     @Override
-    public Iterable<String> arguments() throws CommandLineExpansionException, InterruptedException {
+    public Iterable<String> arguments() {
       return arguments(null, PathMapper.NOOP);
     }
 
@@ -524,8 +537,16 @@ public Iterable<String> arguments(
           switch (arg) {
             case PathStrippable ps -> ps.expand(pathMapper::map);
             // StarlarkAction stores the executable path as a string to save memory, but it should
-            // still be mapped just like a PathFragment.
-            case String s -> pathMapper.map(PathFragment.create(s)).getPathString();
+            // still be mapped just like a PathFragment. In this case, the string always represents
+            // a normalized path, so if it isn't (e.g. because it is an absolute path, possibly for
+            // another OS), don't normalize or map it.
+            case String s when hasExecutablePath -> {
+              PathFragment pathFragment = PathFragment.create(s);
+              if (!pathFragment.getPathString().equals(s)) {
+                yield s;
+              }
+              yield pathMapper.map(pathFragment).getPathString();
+            }
             default -> CommandLineItem.expandToCommandLine(arg);
           });
     }

src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java

@@ -16,15 +16,18 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.CommandLines.ExpandedCommandLines;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import java.util.Collections;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 
 /** Tests for {@link CommandLines}. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
 public class CommandLinesTest {
 
   private final ArtifactExpander artifactExpander = null;
@@ -206,4 +209,39 @@ public void expand_flagsOnly_movesOnlyDashDashPrefixedFlagsToParamFile() throws
         .containsExactly("--a", "--b=c")
         .inOrder();
   }
+
+  @Test
+  public void expand_onlyExecutableArgProcessedForPathMapping(
+      @TestParameter({"0", "1", "2", "3"}) int numNonExecutableArgs,
+      @TestParameter boolean normalizedExecutablePath,
+      @TestParameter boolean mappableNonExecutablePath)
+      throws Exception {
+    CommandLines.Builder builder = CommandLines.builder();
+    String executableArg =
+        normalizedExecutablePath
+            ? "bazel-out/k8-fastbuild/bin/my_binary"
+            : "bazel-out/some/path/../my_binary";
+    String nonExecutableArg =
+        mappableNonExecutablePath ? "bazel-out/k8-fastbuild/bin/unrelated" : "hello/../world";
+    builder.addSingleArgument(executableArg);
+    for (int i = 0; i < numNonExecutableArgs; i++) {
+      builder.addSingleArgument(nonExecutableArg);
+    }
+    CommandLines commandLines = builder.build();
+    PathMapper pathMapper =
+        execPath ->
+            execPath.startsWith(PathFragment.create("bazel-out"))
+                ? PathFragment.create("mapped").getRelative(execPath)
+                : execPath;
+
+    String expectedExecutableArg =
+        normalizedExecutablePath ? "mapped/bazel-out/k8-fastbuild/bin/my_binary" : executableArg;
+    Iterable<String> expectedArgs =
+        Iterables.concat(
+            ImmutableList.of(expectedExecutableArg),
+            Collections.nCopies(numNonExecutableArgs, nonExecutableArg));
+    assertThat(commandLines.allArguments(pathMapper))
+        .containsExactlyElementsIn(expectedArgs)
+        .inOrder();
+  }
 }