Skip to content

Commit

Permalink
Add $(rlocationpath(s) ...) expansion
Browse files Browse the repository at this point in the history 
The new location expansion pattern `rlocationpath` and its plural
version `rlocationpaths` resolve to the path(s) suitable for the
Rlocation function offered by runfiles libraries.

Compared to the `rootpath` pattern, which can returns
'../repo_name/pkg/file` for a file in an external repository and
`pkg/file` for a file in the main repository, the path returned by
`rlocationpath` is always of the form `repo_name/pkg/file`.

Work towards #16124
Fixes #10923
  • Loading branch information
@fmeum
fmeum committed Oct 8, 2022
1 parent 7cc4812 commit 618aecd
Show file tree
Hide file tree
Showing 6 changed files with 168 additions and 64 deletions.
99 changes: 67 additions & 32 deletions src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import static java.util.stream.Collectors.joining;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.common.collect.ImmutableCollection;
Expand All @@ -26,7 +27,9 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.LocationExpander.LocationFunction.PathType;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
import com.google.devtools.build.lib.packages.BuildType;
Expand Down Expand Up @@ -69,28 +72,32 @@ public final class LocationExpander {
private final RuleErrorConsumer ruleErrorConsumer;
private final ImmutableMap<String, LocationFunction> functions;
private final RepositoryMapping repositoryMapping;
private final String workspaceRunfilesDirectory;

@VisibleForTesting
LocationExpander(
RuleErrorConsumer ruleErrorConsumer,
Map<String, LocationFunction> functions,
RepositoryMapping repositoryMapping) {
RepositoryMapping repositoryMapping,
String workspaceRunfilesDirectory) {
this.ruleErrorConsumer = ruleErrorConsumer;
this.functions = ImmutableMap.copyOf(functions);
this.repositoryMapping = repositoryMapping;
this.workspaceRunfilesDirectory = workspaceRunfilesDirectory;
}

private LocationExpander(
RuleErrorConsumer ruleErrorConsumer,
RuleContext ruleContext,
Label root,
Supplier<Map<Label, Collection<Artifact>>> locationMap,
boolean execPaths,
boolean legacyExternalRunfiles,
RepositoryMapping repositoryMapping) {
this(
ruleErrorConsumer,
ruleContext,
allLocationFunctions(root, locationMap, execPaths, legacyExternalRunfiles),
repositoryMapping);
repositoryMapping,
ruleContext.getWorkspaceName());
}

/**
Expand Down Expand Up @@ -204,7 +211,7 @@ private String expand(String value, ErrorReporter reporter) {
// (2) Call appropriate function to obtain string replacement.
String functionValue = value.substring(nextWhitespace + 1, end).trim();
try {
String replacement = functions.get(fname).apply(functionValue, repositoryMapping);
String replacement = functions.get(fname).apply(functionValue, repositoryMapping, workspaceRunfilesDirectory);
result.append(replacement);
} catch (IllegalStateException ise) {
reporter.report(ise.getMessage());
Expand Down Expand Up @@ -232,23 +239,29 @@ public String expandAttribute(String attrName, String attrValue) {

@VisibleForTesting
static final class LocationFunction {
enum PathType {
LOCATION,
EXEC,
RLOCATION,
}

private static final int MAX_PATHS_SHOWN = 5;

private final Label root;
private final Supplier<Map<Label, Collection<Artifact>>> locationMapSupplier;
private final boolean execPaths;
private final PathType pathType;
private final boolean legacyExternalRunfiles;
private final boolean multiple;

LocationFunction(
Label root,
Supplier<Map<Label, Collection<Artifact>>> locationMapSupplier,
boolean execPaths,
boolean legacyExternalRunfiles,
boolean multiple) {
Label root,
Supplier<Map<Label, Collection<Artifact>>> locationMapSupplier,
PathType pathType,
boolean legacyExternalRunfiles,
boolean multiple) {
this.root = root;
this.locationMapSupplier = locationMapSupplier;
this.execPaths = execPaths;
this.pathType = Preconditions.checkNotNull(pathType);
this.legacyExternalRunfiles = legacyExternalRunfiles;
this.multiple = multiple;
}
Expand All @@ -259,10 +272,11 @@ static final class LocationFunction {
* using the {@code repositoryMapping}.
*
* @param arg The label-like string to be expanded, e.g. ":foo" or "//foo:bar"
* @param repositoryMapping map of {@code RepositoryName}s defined in the main workspace
* @param repositoryMapping map of apparent repository names to {@code RepositoryName}s
* @param workspaceRunfilesDirectory name of the runfiles directory corresponding to the main repository
* @return The expanded value
*/
public String apply(String arg, RepositoryMapping repositoryMapping) {
public String apply(String arg, RepositoryMapping repositoryMapping, String workspaceRunfilesDirectory) {
Label label;
try {
label = root.getRelativeWithRemapping(arg, repositoryMapping);
Expand All @@ -271,14 +285,14 @@ public String apply(String arg, RepositoryMapping repositoryMapping) {
String.format(
"invalid label in %s expression: %s", functionName(), e.getMessage()), e);
}
Collection<String> paths = resolveLabel(label);
Collection<String> paths = resolveLabel(label, workspaceRunfilesDirectory);
return joinPaths(paths);
}

/**
* Returns all target location(s) of the given label.
*/
private Collection<String> resolveLabel(Label unresolved) throws IllegalStateException {
private Collection<String> resolveLabel(Label unresolved, String workspaceRunfilesDirectory) throws IllegalStateException {
Collection<Artifact> artifacts = locationMapSupplier.get().get(unresolved);

if (artifacts == null) {
Expand All @@ -288,7 +302,7 @@ private Collection<String> resolveLabel(Label unresolved) throws IllegalStateExc
unresolved, functionName()));
}

Set<String> paths = getPaths(artifacts);
Set<String> paths = getPaths(artifacts, workspaceRunfilesDirectory);
if (paths.isEmpty()) {
throw new IllegalStateException(
String.format(
Expand All @@ -313,24 +327,37 @@ private Collection<String> resolveLabel(Label unresolved) throws IllegalStateExc
* Extracts list of all executables associated with given collection of label artifacts.
*
* @param artifacts to get the paths of
* @param workspaceRunfilesDirectory name of the runfiles directory corresponding to the main repository
* @return all associated executable paths
*/
private Set<String> getPaths(Collection<Artifact> artifacts) {
private Set<String> getPaths(Collection<Artifact> artifacts, String workspaceRunfilesDirectory) {
TreeSet<String> paths = Sets.newTreeSet();
for (Artifact artifact : artifacts) {
PathFragment execPath =
execPaths
? artifact.getExecPath()
: legacyExternalRunfiles
? artifact.getPathForLocationExpansion()
: artifact.getRunfilesPath();
if (execPath != null) { // omit middlemen etc
paths.add(execPath.getCallablePathString());
PathFragment path = getPath(artifact, workspaceRunfilesDirectory);
if (path != null) { // omit middlemen etc
paths.add(path.getCallablePathString());
}
}
return paths;
}

private PathFragment getPath(Artifact artifact, String workspaceRunfilesDirectory) {
switch (pathType) {
case LOCATION:
return legacyExternalRunfiles ? artifact.getPathForLocationExpansion() : artifact.getRunfilesPath();
case EXEC:
return artifact.getExecPath();
case RLOCATION:
PathFragment runfilesPath = artifact.getRunfilesPath();
if (runfilesPath.startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX)) {
return runfilesPath.relativeTo(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX);
} else {
return PathFragment.create(workspaceRunfilesDirectory).getRelative(runfilesPath);
}
}
throw new IllegalStateException("Unexpected PathType: " + pathType);
}

private String joinPaths(Collection<String> paths) {
return paths.stream().map(ShellEscaper::escapeString).collect(joining(" "));
}
Expand All @@ -348,27 +375,35 @@ static ImmutableMap<String, LocationFunction> allLocationFunctions(
return new ImmutableMap.Builder<String, LocationFunction>()
.put(
"location",
new LocationFunction(root, locationMap, execPaths, legacyExternalRunfiles, EXACTLY_ONE))
new LocationFunction(root, locationMap, execPaths ? PathType.EXEC : PathType.LOCATION, legacyExternalRunfiles, EXACTLY_ONE))
.put(
"locations",
new LocationFunction(
root, locationMap, execPaths, legacyExternalRunfiles, ALLOW_MULTIPLE))
root, locationMap, execPaths ? PathType.EXEC : PathType.LOCATION, legacyExternalRunfiles, ALLOW_MULTIPLE))
.put(
"rootpath",
new LocationFunction(
root, locationMap, USE_LOCATION_PATHS, legacyExternalRunfiles, EXACTLY_ONE))
root, locationMap, PathType.LOCATION, legacyExternalRunfiles, EXACTLY_ONE))
.put(
"rootpaths",
new LocationFunction(
root, locationMap, USE_LOCATION_PATHS, legacyExternalRunfiles, ALLOW_MULTIPLE))
root, locationMap, PathType.LOCATION, legacyExternalRunfiles, ALLOW_MULTIPLE))
.put(
"execpath",
new LocationFunction(
root, locationMap, USE_EXEC_PATHS, legacyExternalRunfiles, EXACTLY_ONE))
root, locationMap, PathType.EXEC, legacyExternalRunfiles, EXACTLY_ONE))
.put(
"execpaths",
new LocationFunction(
root, locationMap, USE_EXEC_PATHS, legacyExternalRunfiles, ALLOW_MULTIPLE))
root, locationMap, PathType.EXEC, legacyExternalRunfiles, ALLOW_MULTIPLE))
.put(
"rlocationpath",
new LocationFunction(
root, locationMap, PathType.RLOCATION, legacyExternalRunfiles, EXACTLY_ONE))
.put(
"rlocationpaths",
new LocationFunction(
root, locationMap, PathType.RLOCATION, legacyExternalRunfiles, ALLOW_MULTIPLE))
.buildOrThrow();
}

Expand Down
10 changes: 7 additions & 3 deletions src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ final class LocationTemplateContext implements TemplateContext {
private final ImmutableMap<String, LocationFunction> functions;
private final RepositoryMapping repositoryMapping;
private final boolean windowsPath;
private final String workspaceRunfilesDirectory;

private LocationTemplateContext(
TemplateContext delegate,
Expand All @@ -58,12 +59,14 @@ private LocationTemplateContext(
boolean execPaths,
boolean legacyExternalRunfiles,
RepositoryMapping repositoryMapping,
boolean windowsPath) {
boolean windowsPath,
String workspaceRunfilesDirectory) {
this.delegate = delegate;
this.functions =
LocationExpander.allLocationFunctions(root, locationMap, execPaths, legacyExternalRunfiles);
this.repositoryMapping = repositoryMapping;
this.windowsPath = windowsPath;
this.workspaceRunfilesDirectory = workspaceRunfilesDirectory;
}

public LocationTemplateContext(
Expand All @@ -83,7 +86,8 @@ public LocationTemplateContext(
execPaths,
ruleContext.getConfiguration().legacyExternalRunfiles(),
ruleContext.getRule().getPackage().getRepositoryMapping(),
windowsPath);
windowsPath,
ruleContext.getWorkspaceName());
}

@Override
Expand All @@ -108,7 +112,7 @@ private String lookupFunctionImpl(String name, String param) throws ExpansionExc
try {
LocationFunction f = functions.get(name);
if (f != null) {
return f.apply(param, repositoryMapping);
return f.apply(param, repositoryMapping, workspaceRunfilesDirectory);
}
} catch (IllegalStateException e) {
throw new ExpansionException(e.getMessage(), e);
Expand Down
24 changes: 19 additions & 5 deletions src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ public void otherPathExpansion() throws Exception {
.matches("foo expansion/foo.txt bar");
assertThat(expander.expand("foo $(rootpaths :foo) bar"))
.matches("foo expansion/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpath :foo) bar"))
.matches("foo workspace/expansion/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpaths :foo) bar"))
.matches("foo workspace/expansion/foo.txt bar");
}

@Test
Expand Down Expand Up @@ -158,6 +162,10 @@ public void otherPathExternalExpansion() throws Exception {
.matches("foo external/r/p/foo.txt bar");
assertThat(expander.expand("foo $(rootpaths @r//p:foo) bar"))
.matches("foo external/r/p/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpath @r//p:foo) bar"))
.matches("foo r/p/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpath @r//p:foo) bar"))
.matches("foo r/p/foo.txt bar");
}

@Test
Expand All @@ -181,8 +189,10 @@ public void otherPathExternalExpansionNoLegacyExternalRunfiles() throws Exceptio
.matches("foo .*-out/.*/external/r/p/foo\\.txt bar");
assertThat(expander.expand("foo $(execpaths @r//p:foo) bar"))
.matches("foo .*-out/.*/external/r/p/foo\\.txt bar");
assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rootpaths @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).isEqualTo("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rootpaths @r//p:foo) bar")).isEqualTo("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpath @r//p:foo) bar")).isEqualTo("foo r/p/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpath @r//p:foo) bar")).isEqualTo("foo r/p/foo.txt bar");
}

@Test
Expand All @@ -208,8 +218,10 @@ public void otherPathExternalExpansionNoLegacyExternalRunfilesSiblingRepositoryL
.matches("foo .*-out/r/.*/p/foo\\.txt bar");
assertThat(expander.expand("foo $(execpaths @r//p:foo) bar"))
.matches("foo .*-out/r/.*/p/foo\\.txt bar");
assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rootpaths @r//p:foo) bar")).matches("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rootpath @r//p:foo) bar")).isEqualTo("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rootpaths @r//p:foo) bar")).isEqualTo("foo ../r/p/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpath @r//p:foo) bar")).isEqualTo("foo r/p/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpaths @r//p:foo) bar")).isEqualTo("foo r/p/foo.txt bar");
}

@Test
Expand All @@ -223,6 +235,8 @@ public void otherPathMultiExpansion() throws Exception {
assertThat(expander.expand("foo $(execpaths :foo) bar"))
.matches("foo .*-out/.*/expansion/bar\\.txt .*-out/.*/expansion/foo\\.txt bar");
assertThat(expander.expand("foo $(rootpaths :foo) bar"))
.matches("foo expansion/bar.txt expansion/foo.txt bar");
.isEqualTo("foo expansion/bar.txt expansion/foo.txt bar");
assertThat(expander.expand("foo $(rlocationpaths :foo) bar"))
.isEqualTo("foo workspace/expansion/bar.txt workspace/expansion/foo.txt bar");
}
}
12 changes: 7 additions & 5 deletions src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ public boolean hasErrors() {

private LocationExpander makeExpander(RuleErrorConsumer ruleErrorConsumer) throws Exception {
LocationFunction f1 = new LocationFunctionBuilder("//a", false)
.setExecPaths(false)
.setPathType(LocationFunction.PathType.LOCATION)
.add("//a", "/exec/src/a")
.build();

LocationFunction f2 = new LocationFunctionBuilder("//b", true)
.setExecPaths(false)
.setPathType(LocationFunction.PathType.LOCATION)
.add("//b", "/exec/src/b")
.build();

Expand All @@ -74,7 +74,8 @@ private LocationExpander makeExpander(RuleErrorConsumer ruleErrorConsumer) throw
ImmutableMap.<String, LocationFunction>of(
"location", f1,
"locations", f2),
RepositoryMapping.ALWAYS_FALLBACK);
RepositoryMapping.ALWAYS_FALLBACK,
"workspace");
}

private String expand(String input) throws Exception {
Expand Down Expand Up @@ -126,7 +127,7 @@ public void noExpansionOnError() throws Exception {
@Test
public void expansionWithRepositoryMapping() throws Exception {
LocationFunction f1 = new LocationFunctionBuilder("//a", false)
.setExecPaths(false)
.setPathType(LocationFunction.PathType.LOCATION)
.add("@bar//a", "/exec/src/a")
.build();

Expand All @@ -137,7 +138,8 @@ public void expansionWithRepositoryMapping() throws Exception {
new LocationExpander(
new Capture(),
ImmutableMap.<String, LocationFunction>of("location", f1),
RepositoryMapping.createAllowingFallback(repositoryMapping));
RepositoryMapping.createAllowingFallback(repositoryMapping),
"workspace");

String value = locationExpander.expand("$(location @foo//a)");
assertThat(value).isEqualTo("src/a");
Expand Down
Loading

0 comments on commit 618aecd

Please sign in to comment.