[8.0.0] Only process executable path for path mapping (#24599)
This fixes a few issues introduced in
282ac623df3523e2e31a2de9c002e5c50da19fec:
* 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 #24576.
PiperOrigin-RevId: 703536848
Change-Id: I358696bc268ab1f400798ef55ea4db8d556f07dd
Commit
https://github.com/bazelbuild/bazel/commit/b00576de25ad5eed2af6607f51ad004874079519
Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
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 9a489fc..91798cf 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 @@
* <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 @@
}
}
- 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 @@
@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 @@
@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 @@
@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 @@
@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 @@
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 @@
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 @@
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 e63efa0..0000000
--- 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<PathFragment> 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 23ee455..5b28ff5 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 @@
.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();
+ }
}
