diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java index 9a489fc99c16bc..91798cf668e7b1 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java @@ -39,7 +39,7 @@ *

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 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 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 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 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 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 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 arguments() throws CommandLineExpansionException, InterruptedException { + public Iterable arguments() { return arguments(null, PathMapper.NOOP); } @@ -524,8 +537,16 @@ public Iterable 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); }); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/PathStrippable.java b/src/main/java/com/google/devtools/build/lib/actions/PathStrippable.java deleted file mode 100644 index e63efa03a5ed87..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/actions/PathStrippable.java +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright 2023 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.lib.actions; - -import com.google.devtools.build.lib.vfs.PathFragment; -import java.util.function.UnaryOperator; - -/** - * A {@link CommandLineItem} that can apply the {@code stripPaths} map to optionally strip config - * prefixes before returning output artifact exec paths. - */ -public interface PathStrippable extends CommandLineItem { - String expand(UnaryOperator stripPaths); -} diff --git a/src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java b/src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java index 23ee455ccf9c4a..5b28ff5621abed 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java +++ b/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 expectedArgs = + Iterables.concat( + ImmutableList.of(expectedExecutableArg), + Collections.nCopies(numNonExecutableArgs, nonExecutableArg)); + assertThat(commandLines.allArguments(pathMapper)) + .containsExactlyElementsIn(expectedArgs) + .inOrder(); + } }