From 181bd1a142ddb06d819f52a9eead4c2cc735616d Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 12 Nov 2020 08:15:06 -0800 Subject: [PATCH] Use Artifact#getRunfilesPath to expand rootpath and rootpaths vars if --nolegacy_external_runfiles is specified. PiperOrigin-RevId: 342051423 --- .../build/lib/analysis/LocationExpander.java | 53 ++++++++++++++----- .../lib/analysis/LocationTemplateContext.java | 5 +- .../LocationExpanderIntegrationTest.java | 25 +++++++++ .../lib/analysis/LocationFunctionTest.java | 9 +++- 4 files changed, 78 insertions(+), 14 deletions(-) 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 e11c070b7d21c7..0b7c755cb3bc81 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 @@ -85,8 +85,12 @@ private LocationExpander( Label root, Supplier>> locationMap, boolean execPaths, + boolean legacyExternalRunfiles, ImmutableMap repositoryMapping) { - this(ruleErrorConsumer, allLocationFunctions(root, locationMap, execPaths), repositoryMapping); + this( + ruleErrorConsumer, + allLocationFunctions(root, locationMap, execPaths, legacyExternalRunfiles), + repositoryMapping); } /** @@ -111,6 +115,7 @@ private LocationExpander( Suppliers.memoize( () -> LocationExpander.buildLocationMap(ruleContext, labelMap, allowData)), execPaths, + ruleContext.getConfiguration().legacyExternalRunfiles(), ruleContext.getRule().getPackage().getRepositoryMapping()); } @@ -230,16 +235,19 @@ static final class LocationFunction { private final Label root; private final Supplier>> locationMapSupplier; private final boolean execPaths; + private final boolean legacyExternalRunfiles; private final boolean multiple; LocationFunction( Label root, Supplier>> locationMapSupplier, boolean execPaths, + boolean legacyExternalRunfiles, boolean multiple) { this.root = root; this.locationMapSupplier = locationMapSupplier; this.execPaths = execPaths; + this.legacyExternalRunfiles = legacyExternalRunfiles; this.multiple = multiple; } @@ -279,7 +287,7 @@ private Collection resolveLabel(Label unresolved) throws IllegalStateExc unresolved, functionName())); } - Set paths = getPaths(artifacts, execPaths); + Set paths = getPaths(artifacts); if (paths.isEmpty()) { throw new IllegalStateException( String.format( @@ -304,14 +312,17 @@ 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 takeExecPath if false, the location path will be taken * @return all associated executable paths */ - private Set getPaths(Collection artifacts, boolean takeExecPath) { + private Set getPaths(Collection artifacts) { TreeSet paths = Sets.newTreeSet(); for (Artifact artifact : artifacts) { PathFragment execPath = - takeExecPath ? artifact.getExecPath() : artifact.getPathForLocationExpansion(); + execPaths + ? artifact.getExecPath() + : legacyExternalRunfiles + ? artifact.getPathForLocationExpansion() + : artifact.getRunfilesPath(); if (execPath != null) { // omit middlemen etc paths.add(execPath.getCallablePathString()); } @@ -329,16 +340,34 @@ private String functionName() { } static ImmutableMap allLocationFunctions( - Label root, Supplier>> locationMap, boolean execPaths) { + Label root, + Supplier>> locationMap, + boolean execPaths, + boolean legacyExternalRunfiles) { return new ImmutableMap.Builder() - .put("location", new LocationFunction(root, locationMap, execPaths, EXACTLY_ONE)) - .put("locations", new LocationFunction(root, locationMap, execPaths, ALLOW_MULTIPLE)) - .put("rootpath", new LocationFunction(root, locationMap, USE_LOCATION_PATHS, EXACTLY_ONE)) + .put( + "location", + new LocationFunction(root, locationMap, execPaths, legacyExternalRunfiles, EXACTLY_ONE)) + .put( + "locations", + new LocationFunction( + root, locationMap, execPaths, legacyExternalRunfiles, ALLOW_MULTIPLE)) + .put( + "rootpath", + new LocationFunction( + root, locationMap, USE_LOCATION_PATHS, legacyExternalRunfiles, EXACTLY_ONE)) .put( "rootpaths", - new LocationFunction(root, locationMap, USE_LOCATION_PATHS, ALLOW_MULTIPLE)) - .put("execpath", new LocationFunction(root, locationMap, USE_EXEC_PATHS, EXACTLY_ONE)) - .put("execpaths", new LocationFunction(root, locationMap, USE_EXEC_PATHS, ALLOW_MULTIPLE)) + new LocationFunction( + root, locationMap, USE_LOCATION_PATHS, legacyExternalRunfiles, ALLOW_MULTIPLE)) + .put( + "execpath", + new LocationFunction( + root, locationMap, USE_EXEC_PATHS, legacyExternalRunfiles, EXACTLY_ONE)) + .put( + "execpaths", + new LocationFunction( + root, locationMap, USE_EXEC_PATHS, legacyExternalRunfiles, ALLOW_MULTIPLE)) .build(); } 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 ad25c1876feba7..494d39a48efa0d 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 @@ -56,10 +56,12 @@ private LocationTemplateContext( Label root, Supplier>> locationMap, boolean execPaths, + boolean legacyExternalRunfiles, ImmutableMap repositoryMapping, boolean windowsPath) { this.delegate = delegate; - this.functions = LocationExpander.allLocationFunctions(root, locationMap, execPaths); + this.functions = + LocationExpander.allLocationFunctions(root, locationMap, execPaths, legacyExternalRunfiles); this.repositoryMapping = repositoryMapping; this.windowsPath = windowsPath; } @@ -78,6 +80,7 @@ public LocationTemplateContext( Suppliers.memoize( () -> LocationExpander.buildLocationMap(ruleContext, labelMap, allowData)), execPaths, + ruleContext.getConfiguration().legacyExternalRunfiles(), ruleContext.getRule().getPackage().getRepositoryMapping(), windowsPath); } 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 577a19c4397cb3..4a229844bd41d2 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 @@ -160,6 +160,31 @@ public void otherPathExternalExpansion() throws Exception { .matches("foo external/r/p/foo.txt bar"); } + @Test + public void otherPathExternalExpansionNoLegacyExternalRunfiles() throws Exception { + scratch.file("expansion/BUILD", "sh_library(name='lib', srcs=['@r//p:foo'])"); + FileSystemUtils.appendIsoLatin1( + scratch.resolve("WORKSPACE"), "local_repository(name='r', path='/r')"); + + // Invalidate WORKSPACE so @r can be resolved. + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, ModifiedFileSet.EVERYTHING_MODIFIED, Root.fromPath(rootDirectory)); + + FileSystemUtils.createDirectoryAndParents(scratch.resolve("/foo/bar")); + scratch.file("/r/WORKSPACE", "workspace(name = 'r')"); + scratch.file("/r/p/BUILD", "genrule(name='foo', outs=['foo.txt'], cmd='never executed')"); + + useConfiguration("--nolegacy_external_runfiles"); + LocationExpander expander = makeExpander("//expansion:lib"); + assertThat(expander.expand("foo $(execpath @r//p:foo) bar")) + .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"); + } + @Test public void otherPathMultiExpansion() throws Exception { scratch.file( 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 8bb793a71fa3f6..7284784323e2ad 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 @@ -164,6 +164,7 @@ final class LocationFunctionBuilder { private final Label root; private final boolean multiple; private boolean execPaths; + private boolean legacyExternalRunfiles; private final Map> labelMap = new HashMap<>(); LocationFunctionBuilder(String rootLabel, boolean multiple) { @@ -172,7 +173,8 @@ final class LocationFunctionBuilder { } public LocationFunction build() { - return new LocationFunction(root, Suppliers.ofInstance(labelMap), execPaths, multiple); + return new LocationFunction( + root, Suppliers.ofInstance(labelMap), execPaths, legacyExternalRunfiles, multiple); } public LocationFunctionBuilder setExecPaths(boolean execPaths) { @@ -180,6 +182,11 @@ public LocationFunctionBuilder setExecPaths(boolean execPaths) { return this; } + public LocationFunctionBuilder setLegacyExternalRunfiles(boolean legacyExternalRunfiles) { + this.legacyExternalRunfiles = legacyExternalRunfiles; + return this; + } + public LocationFunctionBuilder add(String label, String... paths) { labelMap.put( Label.parseAbsoluteUnchecked(label),