Use Artifact#getRunfilesPath to expand rootpath and rootpaths vars if…

… --nolegacy_external_runfiles is specified.

PiperOrigin-RevId: 342051423

src/main/java/com/google/devtools/build/lib/analysis/LocationExpander.java

@@ -85,8 +85,12 @@ private LocationExpander(
       Label root,
       Supplier<Map<Label, Collection<Artifact>>> locationMap,
       boolean execPaths,
+      boolean legacyExternalRunfiles,
       ImmutableMap<RepositoryName, RepositoryName> 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<Map<Label, Collection<Artifact>>> locationMapSupplier;
     private final boolean execPaths;
+    private final boolean legacyExternalRunfiles;
     private final boolean multiple;
 
     LocationFunction(
         Label root,
         Supplier<Map<Label, Collection<Artifact>>> 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<String> resolveLabel(Label unresolved) throws IllegalStateExc
                 unresolved, functionName()));
       }
 
-      Set<String> paths = getPaths(artifacts, execPaths);
+      Set<String> paths = getPaths(artifacts);
       if (paths.isEmpty()) {
         throw new IllegalStateException(
             String.format(
@@ -304,14 +312,17 @@ 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 takeExecPath if false, the location path will be taken
      * @return all associated executable paths
      */
-    private Set<String> getPaths(Collection<Artifact> artifacts, boolean takeExecPath) {
+    private Set<String> getPaths(Collection<Artifact> artifacts) {
       TreeSet<String> 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<String, LocationFunction> allLocationFunctions(
-      Label root, Supplier<Map<Label, Collection<Artifact>>> locationMap, boolean execPaths) {
+      Label root,
+      Supplier<Map<Label, Collection<Artifact>>> locationMap,
+      boolean execPaths,
+      boolean legacyExternalRunfiles) {
     return new ImmutableMap.Builder<String, LocationFunction>()
-        .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();
   }
 

src/main/java/com/google/devtools/build/lib/analysis/LocationTemplateContext.java

@@ -56,10 +56,12 @@ private LocationTemplateContext(
       Label root,
       Supplier<Map<Label, Collection<Artifact>>> locationMap,
       boolean execPaths,
+      boolean legacyExternalRunfiles,
       ImmutableMap<RepositoryName, RepositoryName> 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);
   }

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(

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<Label, Collection<Artifact>> labelMap = new HashMap<>();
 
   LocationFunctionBuilder(String rootLabel, boolean multiple) {
@@ -172,14 +173,20 @@ 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) {
     this.execPaths = 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),