From a1593309f66f892871e334013815b05350b4188f Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 3 Nov 2022 11:39:44 -0700 Subject: [PATCH] Add `$(rlocationpath(s) ...)` expansion 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`. RELNOTES: The new path variable `$(rlocationpath ...)` and its plural form `$(rlocationpaths ...)` can be used to expand labels to the paths accepted by the `Rlocation` function of runfiles libraries. This is the preferred way to access data dependencies at runtime and works on all platforms, even when runfiles are not enabled (e.g., on Windows by default). Work towards #16124 Fixes #10923 Closes #16428. PiperOrigin-RevId: 485930083 Change-Id: I85c0ef29c8332eeabd8473d2611ede546eb24016 --- .../docgen/templates/be/make-variables.vm | 79 +++++++++--- .../build/lib/analysis/LocationExpander.java | 117 +++++++++++++----- .../lib/analysis/LocationTemplateContext.java | 10 +- .../LocationExpanderIntegrationTest.java | 39 ++++++ .../lib/analysis/LocationExpanderTest.java | 35 +++--- .../lib/analysis/LocationFunctionTest.java | 59 +++++---- src/test/py/bazel/runfiles_test.py | 35 ++++++ 7 files changed, 282 insertions(+), 92 deletions(-) diff --git a/src/main/java/com/google/devtools/build/docgen/templates/be/make-variables.vm b/src/main/java/com/google/devtools/build/docgen/templates/be/make-variables.vm index 096a6eac0aea0d..f5ac7d1c84abcf 100644 --- a/src/main/java/com/google/devtools/build/docgen/templates/be/make-variables.vm +++ b/src/main/java/com/google/devtools/build/docgen/templates/be/make-variables.vm @@ -262,15 +262,15 @@

Output files are staged similarly, but are also prefixed with the subpath - bazel-out/cpu-compilation_mode/bin (or for certain outputs: - bazel-out/cpu-compilation_mode/genfiles, or for the outputs - of host tools: bazel-out/host/bin). In the above example, - //testapp:app is a host tool because it appears in + bazel-out/cpu-compilation_mode/bin (or for the outputs of + tools: bazel-out/cpu-opt-exec-hash/bin). In the above example, + //testapp:app is a tool because it appears in show_app_output's tools attribute. So its output file app is written to - bazel-myproject/bazel-out/host/bin/testapp/app. The exec path - is thus bazel-out/host/bin/testapp/app. This extra prefix + bazel-myproject/bazel-out/cpu-opt-exec-hash/bin/testapp/app. + The exec path is thus + bazel-out/cpu-opt-exec-hash/bin/testapp/app. This extra prefix makes it possible to build the same target for, say, two different CPUs in the same build without the results clobbering each other.

@@ -284,15 +284,57 @@

  • - rootpath: Denotes the runfiles path that a built binary can - use to find its dependencies at runtime. -

    + rootpath: Denotes the path that a built binary can use to + find a dependency at runtime relative to the subdirectory of its runfiles + directory corresponding to the main repository. + Note: This only works if + --enable_runfiles is enabled, which is not the case on + Windows by default. Use rlocationpath instead for + cross-platform support.

    - This is the same as execpath but strips the output prefixes - described above. In the above example this means both + This is similar to execpath but strips the configuration + prefixes described above. In the example from above this means both empty.source and app use pure workspace-relative paths: testapp/empty.source and testapp/app.

    +

    + The rootpath of a file in an external repository + repo will start with ../repo/, followed by the + repository-relative path. +

    +

    + This has the same "one output only" requirements as execpath. +

    +
    • + +
  • +

    + rlocationpath: The path a built binary can pass to the + Rlocation function of a runfiles library to find a dependency at + runtime, either in the runfiles directory (if available) or using the + runfiles manifest. +

    +

    + This is similar to rootpath in that it does not contain + configuration prefixes, but differs in that it always starts with the + name of the repository. In the example from above this means that + empty.source and app result in the following + paths: myproject/testapp/empty.source and + myproject/testapp/app. +

    +

    + The rlocationpath of a file in an external repository + repo will start with repo/, followed by the + repository-relative path. +

    +

    + Passing this path to a binary and resolving it to a file system path using + the runfiles libraries is the preferred approach to find dependencies at + runtime. Compared to rootpath, it has the advantage that it + works on all platforms and even if the runfiles directory is not + available. +

    This has the same "one output only" requirements as execpath.

    @@ -303,24 +345,25 @@ rootpath, depending on the attribute being expanded. This is legacy pre-Starlark behavior and not recommended unless you really know what it does for a particular rule. See #2475 + href="https://github.com/bazelbuild/bazel/issues/2475#issuecomment-339318016">#2475 for details.

    • - execpaths, rootpaths, and locations are - the plural variations of execpath, rootpath, and - location, respectively. They support labels producing multiple - outputs, in which case each output is listed separated by a space. Zero-output - rules and malformed labels produce build errors. + execpaths, rootpaths, rlocationpaths, + and locations are the plural variations of execpath, + rootpath, rlocationpaths, andlocation, + respectively. They support labels producing multiple outputs, in which case + each output is listed separated by a space. Zero-output rules and malformed + labels produce build errors.

    All referenced labels must appear in the consuming target's srcs, output files, or deps. Otherwise the build fails. C++ targets can also reference labels in data. + href="$expander.expandRef("cc_binary.data")">data.

    diff --git a/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java b/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java index d82d3de89a0f7d..ddcf5510ac174d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java @@ -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; @@ -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; @@ -63,34 +66,35 @@ public final class LocationExpander { private static final boolean EXACTLY_ONE = false; private static final boolean ALLOW_MULTIPLE = true; - private static final boolean USE_LOCATION_PATHS = false; - private static final boolean USE_EXEC_PATHS = true; - private final RuleErrorConsumer ruleErrorConsumer; private final ImmutableMap functions; private final RepositoryMapping repositoryMapping; + private final String workspaceRunfilesDirectory; @VisibleForTesting LocationExpander( RuleErrorConsumer ruleErrorConsumer, Map 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>> locationMap, boolean execPaths, boolean legacyExternalRunfiles, RepositoryMapping repositoryMapping) { this( - ruleErrorConsumer, + ruleContext, allLocationFunctions(root, locationMap, execPaths, legacyExternalRunfiles), - repositoryMapping); + repositoryMapping, + ruleContext.getWorkspaceName()); } /** @@ -204,7 +208,10 @@ 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()); @@ -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>> locationMapSupplier; - private final boolean execPaths; + private final PathType pathType; private final boolean legacyExternalRunfiles; private final boolean multiple; LocationFunction( Label root, Supplier>> locationMapSupplier, - boolean execPaths, + 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; } @@ -259,10 +272,13 @@ 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); @@ -271,14 +287,13 @@ public String apply(String arg, RepositoryMapping repositoryMapping) { String.format( "invalid label in %s expression: %s", functionName(), e.getMessage()), e); } - Collection paths = resolveLabel(label); + Set paths = resolveLabel(label, workspaceRunfilesDirectory); return joinPaths(paths); } - /** - * Returns all target location(s) of the given label. - */ - private Collection resolveLabel(Label unresolved) throws IllegalStateException { + /** Returns all target location(s) of the given label. */ + private Set resolveLabel(Label unresolved, String workspaceRunfilesDirectory) + throws IllegalStateException { Collection artifacts = locationMapSupplier.get().get(unresolved); if (artifacts == null) { @@ -288,7 +303,7 @@ private Collection resolveLabel(Label unresolved) throws IllegalStateExc unresolved, functionName())); } - Set paths = getPaths(artifacts); + Set paths = getPaths(artifacts, workspaceRunfilesDirectory); if (paths.isEmpty()) { throw new IllegalStateException( String.format( @@ -313,24 +328,41 @@ private Collection 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 getPaths(Collection artifacts) { + private Set getPaths( + Collection artifacts, String workspaceRunfilesDirectory) { TreeSet 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 paths) { return paths.stream().map(ShellEscaper::escapeString).collect(joining(" ")); } @@ -348,27 +380,44 @@ static ImmutableMap allLocationFunctions( return new ImmutableMap.Builder() .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(); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java b/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java index f3e66781c7dec3..0de4a4e498c49b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java @@ -50,6 +50,7 @@ final class LocationTemplateContext implements TemplateContext { private final ImmutableMap functions; private final RepositoryMapping repositoryMapping; private final boolean windowsPath; + private final String workspaceRunfilesDirectory; private LocationTemplateContext( TemplateContext delegate, @@ -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( @@ -83,7 +86,8 @@ public LocationTemplateContext( execPaths, ruleContext.getConfiguration().legacyExternalRunfiles(), ruleContext.getRule().getPackage().getRepositoryMapping(), - windowsPath); + windowsPath, + ruleContext.getWorkspaceName()); } @Override @@ -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); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java index 4f40434ea75b74..7a31156694281c 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderIntegrationTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.Root; @@ -121,6 +122,12 @@ public void otherPathExpansion() throws Exception { "genrule(name='foo', outs=['foo.txt'], cmd='never executed')", "sh_library(name='lib', srcs=[':foo'])"); + FileSystemUtils.appendIsoLatin1(scratch.resolve("WORKSPACE"), "workspace(name='workspace')"); + // Invalidate WORKSPACE to pick up the name. + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, ModifiedFileSet.EVERYTHING_MODIFIED, Root.fromPath(rootDirectory)); + LocationExpander expander = makeExpander("//expansion:lib"); assertThat(expander.expand("foo $(execpath :foo) bar")) .matches("foo .*-out/.*/expansion/foo\\.txt bar"); @@ -130,6 +137,22 @@ 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")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpaths :foo) bar")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpath //expansion:foo) bar")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpaths //expansion:foo) bar")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpath @//expansion:foo) bar")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpaths @//expansion:foo) bar")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpath @workspace//expansion:foo) bar")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpaths @workspace//expansion:foo) bar")) + .isEqualTo("foo workspace/expansion/foo.txt bar"); } @Test @@ -158,6 +181,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")) + .isEqualTo("foo r/p/foo.txt bar"); + assertThat(expander.expand("foo $(rlocationpath @r//p:foo) bar")) + .isEqualTo("foo r/p/foo.txt bar"); } @Test @@ -183,6 +210,10 @@ public void otherPathExternalExpansionNoLegacyExternalRunfiles() throws Exceptio .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 $(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 @@ -210,6 +241,10 @@ public void otherPathExternalExpansionNoLegacyExternalRunfilesSiblingRepositoryL .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 $(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 @@ -224,5 +259,9 @@ public void otherPathMultiExpansion() throws Exception { .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"); + assertThat(expander.expand("foo $(rlocationpaths :foo) bar")) + .isEqualTo( + "foo __main__/expansion/bar.txt __main__/expansion/foo.txt bar" + .replace("__main__", TestConstants.WORKSPACE_NAME)); } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java index abdeb1ed742df4..9e6b63490ef638 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/LocationExpanderTest.java @@ -59,22 +59,25 @@ public boolean hasErrors() { } private LocationExpander makeExpander(RuleErrorConsumer ruleErrorConsumer) throws Exception { - LocationFunction f1 = new LocationFunctionBuilder("//a", false) - .setExecPaths(false) - .add("//a", "/exec/src/a") - .build(); - - LocationFunction f2 = new LocationFunctionBuilder("//b", true) - .setExecPaths(false) - .add("//b", "/exec/src/b") - .build(); + LocationFunction f1 = + new LocationFunctionBuilder("//a", false) + .setPathType(LocationFunction.PathType.LOCATION) + .add("//a", "/exec/src/a") + .build(); + + LocationFunction f2 = + new LocationFunctionBuilder("//b", true) + .setPathType(LocationFunction.PathType.LOCATION) + .add("//b", "/exec/src/b") + .build(); return new LocationExpander( ruleErrorConsumer, ImmutableMap.of( "location", f1, "locations", f2), - RepositoryMapping.ALWAYS_FALLBACK); + RepositoryMapping.ALWAYS_FALLBACK, + "workspace"); } private String expand(String input) throws Exception { @@ -125,10 +128,11 @@ public void noExpansionOnError() throws Exception { @Test public void expansionWithRepositoryMapping() throws Exception { - LocationFunction f1 = new LocationFunctionBuilder("//a", false) - .setExecPaths(false) - .add("@bar//a", "/exec/src/a") - .build(); + LocationFunction f1 = + new LocationFunctionBuilder("//a", false) + .setPathType(LocationFunction.PathType.LOCATION) + .add("@bar//a", "/exec/src/a") + .build(); ImmutableMap repositoryMapping = ImmutableMap.of("foo", RepositoryName.create("bar")); @@ -137,7 +141,8 @@ public void expansionWithRepositoryMapping() throws Exception { new LocationExpander( new Capture(), ImmutableMap.of("location", f1), - RepositoryMapping.createAllowingFallback(repositoryMapping)); + RepositoryMapping.createAllowingFallback(repositoryMapping), + "workspace"); String value = locationExpander.expand("$(location @foo//a)"); assertThat(value).isEqualTo("src/a"); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java index 38addac4979d01..608b39b34fa105 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/LocationFunctionTest.java @@ -49,16 +49,16 @@ public class LocationFunctionTest { public void absoluteAndRelativeLabels() throws Exception { LocationFunction func = new LocationFunctionBuilder("//foo", false).add("//foo", "/exec/src/bar").build(); - assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo("src/bar"); - assertThat(func.apply(":foo", RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo("src/bar"); - assertThat(func.apply("foo", RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo("src/bar"); + assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)).isEqualTo("src/bar"); + assertThat(func.apply(":foo", RepositoryMapping.ALWAYS_FALLBACK, null)).isEqualTo("src/bar"); + assertThat(func.apply("foo", RepositoryMapping.ALWAYS_FALLBACK, null)).isEqualTo("src/bar"); } @Test public void pathUnderExecRootUsesDotSlash() throws Exception { LocationFunction func = new LocationFunctionBuilder("//foo", false).add("//foo", "/exec/bar").build(); - assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo("./bar"); + assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)).isEqualTo("./bar"); } @Test @@ -67,7 +67,7 @@ public void noSuchLabel() throws Exception { IllegalStateException expected = assertThrows( IllegalStateException.class, - () -> func.apply("//bar", RepositoryMapping.ALWAYS_FALLBACK)); + () -> func.apply("//bar", RepositoryMapping.ALWAYS_FALLBACK, null)); assertThat(expected) .hasMessageThat() .isEqualTo( @@ -81,7 +81,7 @@ public void emptyList() throws Exception { IllegalStateException expected = assertThrows( IllegalStateException.class, - () -> func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)); + () -> func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)); assertThat(expected) .hasMessageThat() .isEqualTo("label '//foo:foo' in $(location) expression expands to no files"); @@ -94,7 +94,7 @@ public void tooMany() throws Exception { IllegalStateException expected = assertThrows( IllegalStateException.class, - () -> func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)); + () -> func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)); assertThat(expected) .hasMessageThat() .isEqualTo( @@ -109,7 +109,7 @@ public void noSuchLabelMultiple() throws Exception { IllegalStateException expected = assertThrows( IllegalStateException.class, - () -> func.apply("//bar", RepositoryMapping.ALWAYS_FALLBACK)); + () -> func.apply("//bar", RepositoryMapping.ALWAYS_FALLBACK, null)); assertThat(expected) .hasMessageThat() .isEqualTo( @@ -121,7 +121,7 @@ public void noSuchLabelMultiple() throws Exception { public void fileWithSpace() throws Exception { LocationFunction func = new LocationFunctionBuilder("//foo", false).add("//foo", "/exec/file/with space").build(); - assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)) + assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)) .isEqualTo("'file/with space'"); } @@ -130,7 +130,7 @@ public void multipleFiles() throws Exception { LocationFunction func = new LocationFunctionBuilder("//foo", true) .add("//foo", "/exec/foo/bar", "/exec/out/foo/foobar") .build(); - assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)) + assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)) .isEqualTo("foo/bar foo/foobar"); } @@ -139,27 +139,41 @@ public void filesWithSpace() throws Exception { LocationFunction func = new LocationFunctionBuilder("//foo", true) .add("//foo", "/exec/file/with space", "/exec/file/with spaces ") .build(); - assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)) + assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)) .isEqualTo("'file/with space' 'file/with spaces '"); } @Test public void execPath() throws Exception { - LocationFunction func = new LocationFunctionBuilder("//foo", true) - .setExecPaths(true) - .add("//foo", "/exec/bar", "/exec/out/foobar") - .build(); - assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK)) + LocationFunction func = + new LocationFunctionBuilder("//foo", true) + .setPathType(LocationFunction.PathType.EXEC) + .add("//foo", "/exec/bar", "/exec/out/foobar") + .build(); + assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, null)) .isEqualTo("./bar out/foobar"); } + @Test + public void rlocationPath() throws Exception { + LocationFunction func = + new LocationFunctionBuilder("//foo", true) + .setPathType(LocationFunction.PathType.RLOCATION) + .add("//foo", "/exec/bar", "/exec/out/foobar") + .build(); + assertThat(func.apply("//foo", RepositoryMapping.ALWAYS_FALLBACK, "workspace")) + .isEqualTo("workspace/bar workspace/foobar"); + } + @Test public void locationFunctionWithMappingReplace() throws Exception { RepositoryName b = RepositoryName.create("b"); ImmutableMap repositoryMapping = ImmutableMap.of("a", b); LocationFunction func = new LocationFunctionBuilder("//foo", false).add("@b//foo", "/exec/src/bar").build(); - assertThat(func.apply("@a//foo", RepositoryMapping.createAllowingFallback(repositoryMapping))) + assertThat( + func.apply( + "@a//foo", RepositoryMapping.createAllowingFallback(repositoryMapping), null)) .isEqualTo("src/bar"); } @@ -170,7 +184,8 @@ public void locationFunctionWithMappingIgnoreRepo() throws Exception { LocationFunction func = new LocationFunctionBuilder("//foo", false).add("@potato//foo", "/exec/src/bar").build(); assertThat( - func.apply("@potato//foo", RepositoryMapping.createAllowingFallback(repositoryMapping))) + func.apply( + "@potato//foo", RepositoryMapping.createAllowingFallback(repositoryMapping), null)) .isEqualTo("src/bar"); } } @@ -178,7 +193,7 @@ public void locationFunctionWithMappingIgnoreRepo() throws Exception { final class LocationFunctionBuilder { private final Label root; private final boolean multiple; - private boolean execPaths; + private LocationFunction.PathType pathType = LocationFunction.PathType.LOCATION; private boolean legacyExternalRunfiles; private final Map> labelMap = new HashMap<>(); @@ -189,12 +204,12 @@ final class LocationFunctionBuilder { public LocationFunction build() { return new LocationFunction( - root, Suppliers.ofInstance(labelMap), execPaths, legacyExternalRunfiles, multiple); + root, Suppliers.ofInstance(labelMap), pathType, legacyExternalRunfiles, multiple); } @CanIgnoreReturnValue - public LocationFunctionBuilder setExecPaths(boolean execPaths) { - this.execPaths = execPaths; + public LocationFunctionBuilder setPathType(LocationFunction.PathType pathType) { + this.pathType = pathType; return this; } diff --git a/src/test/py/bazel/runfiles_test.py b/src/test/py/bazel/runfiles_test.py index 5cfccdc89efd4e..f5bb9b2ff66adb 100644 --- a/src/test/py/bazel/runfiles_test.py +++ b/src/test/py/bazel/runfiles_test.py @@ -312,6 +312,41 @@ def testLegacyExternalRunfilesOption(self): "bin/bin.runfiles_manifest") self.AssertFileContentNotContains(manifest_path, "__main__/external/A") + def testRunfilesLibrariesFindRlocationpathExpansion(self): + self.ScratchDir("A") + self.ScratchFile("A/WORKSPACE") + self.ScratchFile("A/p/BUILD", ["exports_files(['foo.txt'])"]) + self.ScratchFile("A/p/foo.txt", ["Hello, World!"]) + self.ScratchFile("WORKSPACE", ["local_repository(name = 'A', path='A')"]) + self.ScratchFile("pkg/BUILD", [ + "py_binary(", + " name = 'bin',", + " srcs = ['bin.py'],", + " args = [", + " '$(rlocationpath bar.txt)',", + " '$(rlocationpath @A//p:foo.txt)',", + " ],", + " data = [", + " 'bar.txt',", + " '@A//p:foo.txt'", + " ],", + " deps = ['@bazel_tools//tools/python/runfiles'],", + ")", + ]) + self.ScratchFile("pkg/bar.txt", ["Hello, Bazel!"]) + self.ScratchFile("pkg/bin.py", [ + "import sys", + "from tools.python.runfiles import runfiles", + "r = runfiles.Create()", + "for arg in sys.argv[1:]:", + " print(open(r.Rlocation(arg)).read().strip())", + ]) + _, stdout, _ = self.RunBazel(["run", "//pkg:bin"]) + if len(stdout) != 2: + self.fail("stdout: %s" % stdout) + self.assertEqual(stdout[0], "Hello, Bazel!") + self.assertEqual(stdout[1], "Hello, World!") + if __name__ == "__main__": unittest.main()