From 4515833469f14a5334ab144e97e3bf50cf784008 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 25 Jul 2024 23:02:59 -0700 Subject: [PATCH] [7.3.0] Path map executable paths In `CommandLines`, the very first argument of the first command line is always a path to an executable. As such, it should be path mapped, even when it is a string. This wasn't the case for `SpawnAction`'s created via `ctx.actions.run(executable = )`. Work towards #6526 Work towards #22366 Closes #22844. PiperOrigin-RevId: 656258007 Change-Id: Ia046a7cc66aae51aec764e2f1c49e1d4f69e4b37 Closes #23040 --- .../devtools/build/lib/actions/Artifact.java | 1 + .../google/devtools/build/lib/actions/BUILD | 11 ---- .../build/lib/actions/CommandLines.java | 16 +++-- .../com/google/devtools/build/lib/vfs/BUILD | 1 + .../devtools/build/lib/vfs/PathFragment.java | 8 +-- .../build/lib/vfs/PathStrippable.java | 30 ++++++++++ .../actions/StrippingPathMapperTest.java | 58 +++++++++++++++++++ 7 files changed, 104 insertions(+), 21 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/vfs/PathStrippable.java diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 1bfe963b2dc919..12f87cc1da543b 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.util.HashCodes; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.PathStrippable; import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.skyframe.ExecutionPhaseSkyKey; import com.google.devtools.build.skyframe.SkyFunctionName; diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/src/main/java/com/google/devtools/build/lib/actions/BUILD index 26f34dcf22f7d8..1c6250ace7be5a 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BUILD +++ b/src/main/java/com/google/devtools/build/lib/actions/BUILD @@ -114,7 +114,6 @@ java_library( "NotifyOnActionCacheHit.java", "ParamFileInfo.java", "ParameterFile.java", - "PathStrippable.java", "RemoteArtifactChecker.java", "ResourceEstimator.java", "RunningActionEvent.java", @@ -315,7 +314,6 @@ java_library( ":commandline_item", ":fileset_output_symlink", ":package_roots", - ":path_strippable", "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/collect", @@ -472,15 +470,6 @@ java_library( ], ) -java_library( - name = "path_strippable", - srcs = ["PathStrippable.java"], - deps = [ - ":commandline_item", - "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", - ], -) - java_library( name = "resource_manager", srcs = [ 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 61ff1f2cfe7142..d2be926068effe 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 @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.vfs.PathStrippable; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.protobuf.ByteString; import java.io.IOException; @@ -541,12 +542,15 @@ public Iterable arguments() throws CommandLineExpansionException, Interr @Override public Iterable arguments( - @Nullable ArtifactExpander artifactExpander, PathMapper pathMapper) - throws CommandLineExpansionException, InterruptedException { - if (arg instanceof PathStrippable) { - return ImmutableList.of(((PathStrippable) arg).expand(pathMapper::map)); - } - return ImmutableList.of(CommandLineItem.expandToCommandLine(arg)); + @Nullable ArtifactExpander artifactExpander, PathMapper pathMapper) { + return ImmutableList.of( + 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(); + default -> CommandLineItem.expandToCommandLine(arg); + }); } } } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BUILD b/src/main/java/com/google/devtools/build/lib/vfs/BUILD index 141ce7f5daa71d..9fd10921354be1 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/BUILD +++ b/src/main/java/com/google/devtools/build/lib/vfs/BUILD @@ -20,6 +20,7 @@ OS_PATH_POLICY_SOURCES = [ PATH_FRAGMENT_SOURCES = [ "PathFragment.java", "PathSegmentIterator.java", + "PathStrippable.java", ] OUTPUT_SERVICE_SOURCES = [ diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java index c899720419e81e..74ad36b951f8b5 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java @@ -15,7 +15,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.actions.CommandLineItem; import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; @@ -26,6 +25,7 @@ import com.google.protobuf.CodedInputStream; import com.google.protobuf.CodedOutputStream; import java.io.IOException; +import java.util.function.UnaryOperator; import javax.annotation.Nullable; /** @@ -50,7 +50,7 @@ */ @Immutable public abstract class PathFragment - implements Comparable, FileType.HasFileType, CommandLineItem { + implements Comparable, FileType.HasFileType, PathStrippable { private static final OsPathPolicy OS = OsPathPolicy.getFilePathOs(); @SerializationConstant @@ -802,8 +802,8 @@ public String filePathForFileTypeMatcher() { } @Override - public String expandToCommandLine() { - return normalizedPath; + public String expand(UnaryOperator stripPaths) { + return stripPaths.apply(this).normalizedPath; } private static void checkBaseName(String baseName) { diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathStrippable.java b/src/main/java/com/google/devtools/build/lib/vfs/PathStrippable.java new file mode 100644 index 00000000000000..9d228ad198c236 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/vfs/PathStrippable.java @@ -0,0 +1,30 @@ +// 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.vfs; + +import com.google.devtools.build.lib.actions.CommandLineItem; +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); + + @Override + default String expandToCommandLine() { + return expand(UnaryOperator.identity()); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java index 0cce3471f5aa1e..ef20556420156f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java @@ -210,4 +210,62 @@ public void starlarkRule_optedInViaModifyExecutionInfo() throws Exception { "") .inOrder(); } + + @Test + public void starlarkRule_stringExecutablePath() throws Exception { + scratch.file("defs/BUILD"); + scratch.file( + "defs/defs.bzl", + """ + def my_rule_impl(ctx): + out = ctx.actions.declare_file(ctx.label.name) + ctx.actions.run( + executable = ctx.executable.tool.path, + arguments = [ctx.actions.args().add(out)], + outputs = [out], + tools = [ctx.executable.tool], + execution_requirements = {"supports-path-mapping": "1"}, + ) + return DefaultInfo(files = depset([out])) + + my_rule = rule( + implementation = my_rule_impl, + attrs = { + "tool": attr.label( + default = "//foo:script", + cfg = "exec", + executable = True, + ), + }, + ) + """); + scratch.file( + "foo/BUILD", + """ + sh_binary( + name = 'script', + srcs = ['script.sh'], + visibility = ['//visibility:public'], + ) + """); + scratch.file( + "BUILD", + """ + load("//defs:defs.bzl", "my_rule") + my_rule(name = "my_rule") + """); + + ConfiguredTarget configuredTarget = getConfiguredTarget("//:my_rule"); + Artifact outputArtifact = + configuredTarget.getProvider(FileProvider.class).getFilesToBuild().toList().get(0); + SpawnAction action = (SpawnAction) getGeneratingAction(outputArtifact); + Spawn spawn = action.getSpawn(new ActionExecutionContextBuilder().build()); + + assertThat(spawn.getPathMapper().isNoop()).isFalse(); + String outDir = analysisMock.getProductName() + "-out"; + assertThat(spawn.getArguments()) + .containsExactly( + "%s/cfg/bin/foo/script".formatted(outDir), "%s/cfg/bin/my_rule".formatted(outDir)) + .inOrder(); + } }