Skip to content

Commit

Permalink
[7.3.0] Path map executable paths
Browse files Browse the repository at this point in the history
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 = <some string>)`.

Work towards #6526
Work towards #22366

Closes #22844.

PiperOrigin-RevId: 656258007
Change-Id: Ia046a7cc66aae51aec764e2f1c49e1d4f69e4b37

Closes #23040
  • Loading branch information
fmeum committed Jul 26, 2024
1 parent 088c517 commit b0d7f54
Show file tree
Hide file tree
Showing 7 changed files with 108 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 0 additions & 11 deletions src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ java_library(
"NotifyOnActionCacheHit.java",
"ParamFileInfo.java",
"ParameterFile.java",
"PathStrippable.java",
"RemoteArtifactChecker.java",
"ResourceEstimator.java",
"RunningActionEvent.java",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -541,12 +542,15 @@ public Iterable<String> arguments() throws CommandLineExpansionException, Interr

@Override
public Iterable<String> 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);
});
}
}
}
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/vfs/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ OS_PATH_POLICY_SOURCES = [
PATH_FRAGMENT_SOURCES = [
"PathFragment.java",
"PathSegmentIterator.java",
"PathStrippable.java",
]

OUTPUT_SERVICE_SOURCES = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

/**
Expand All @@ -50,7 +50,7 @@
*/
@Immutable
public abstract class PathFragment
implements Comparable<PathFragment>, FileType.HasFileType, CommandLineItem {
implements Comparable<PathFragment>, FileType.HasFileType, PathStrippable {
private static final OsPathPolicy OS = OsPathPolicy.getFilePathOs();

@SerializationConstant
Expand Down Expand Up @@ -802,8 +802,8 @@ public String filePathForFileTypeMatcher() {
}

@Override
public String expandToCommandLine() {
return normalizedPath;
public String expand(UnaryOperator<PathFragment> stripPaths) {
return stripPaths.apply(this).normalizedPath;
}

private static void checkBaseName(String baseName) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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<PathFragment> stripPaths);

@Override
default String expandToCommandLine() {
return expand(UnaryOperator.identity());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,66 @@ public void starlarkRule_optedInViaModifyExecutionInfo() throws Exception {
"<pkg/source.txt:pkg/source.txt::pkg>")
.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()
.setMetadataProvider(new FakeActionInputFileCache())
.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();
}
}

0 comments on commit b0d7f54

Please sign in to comment.