Put files in ResolutionResult (#1484)

This will allow an optimisation for when the resolver has already downloaded all
the files so we can avoid a "double download"

Author: shs96c

private/rules/coursier.bzl

@@ -456,9 +456,11 @@ def get_direct_dependencies(all_artifacts, input_artifacts):
         if coords:
             full_key = to_key(coords)
             resolved_lookup[full_key] = coords
+
             # Also store by simple group:artifact for fallback matching
             unpacked = unpack_coordinates(coords)
             simple_key = "%s:%s" % (unpacked.group, unpacked.artifact)
+
             # Only use simple key if no classifier (classifiers are intentional)
             classifier = getattr(unpacked, "classifier", None)
             if not classifier:

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ResolutionResult.java

@@ -16,6 +16,8 @@
 
 import com.github.bazelbuild.rules_jvm_external.Coordinates;
 import com.google.common.graph.Graph;
+import java.nio.file.Path;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -26,10 +28,15 @@ public class ResolutionResult {
 
   private final Graph<Coordinates> resolution;
   private final Set<Conflict> conflicts;
+  private final Map<Coordinates, Path> paths;
 
-  public ResolutionResult(Graph<Coordinates> resolution, Set<Conflict> conflicts) {
+  public ResolutionResult(
+      Graph<Coordinates> resolution,
+      Set<Conflict> conflicts,
+      Map<Coordinates, Path> artifactPaths) {
     this.resolution = resolution;
-    this.conflicts = conflicts;
+    this.conflicts = Set.copyOf(conflicts);
+    this.paths = artifactPaths != null ? Map.copyOf(artifactPaths) : Map.of();
   }
 
   public Graph<Coordinates> getResolution() {
@@ -39,4 +46,8 @@ public Graph<Coordinates> getResolution() {
   public Set<Conflict> getConflicts() {
     return conflicts;
   }
+
+  public Map<Coordinates, Path> getPaths() {
+    return paths;
+  }
 }

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/cmd/AbstractMain.java

@@ -70,7 +70,7 @@ public void doMain(String[] args) {
 
       ResolutionResult resolutionResult = resolver.resolve(request);
 
-      infos = fulfillDependencyInfos(resolver, listener, config, resolutionResult.getResolution());
+      infos = fulfillDependencyInfos(resolver, listener, config, resolutionResult);
 
       writeLockFile(listener, config, request, infos, resolutionResult.getConflicts());
 
@@ -87,7 +87,7 @@ private static Set<DependencyInfo> fulfillDependencyInfos(
       Resolver resolver,
       EventListener listener,
       ResolverConfig config,
-      Graph<Coordinates> resolved) {
+      ResolutionResult resolutionResult) {
     listener.onEvent(new PhaseEvent("Downloading dependencies"));
 
     ResolutionRequest request = config.getResolutionRequest();
@@ -103,10 +103,13 @@ private static Set<DependencyInfo> fulfillDependencyInfos(
             request.getLocalCache(resolver.getName()),
             request.getRepositories(),
             listener,
-            cacheResults);
+            cacheResults,
+            resolutionResult.getPaths());
 
     List<CompletableFuture<Set<DependencyInfo>>> futures = new LinkedList<>();
 
+    Graph<Coordinates> resolved = resolutionResult.getResolution();
+
     ExecutorService downloadService =
         Executors.newFixedThreadPool(
             config.getMaxThreads(),

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/events/DownloadEvent.java

@@ -20,10 +20,16 @@ public class DownloadEvent implements Event {
 
   private final Stage stage;
   private final String target;
+  private final String method;
 
   public DownloadEvent(Stage stage, String target) {
+    this(stage, null, target);
+  }
+
+  public DownloadEvent(Stage stage, String method, String target) {
     this.stage = stage;
     this.target = Objects.requireNonNull(target);
+    this.method = method;
   }
 
   public Stage getStage() {
@@ -34,6 +40,10 @@ public String getTarget() {
     return target;
   }
 
+  public String getMethod() {
+    return method;
+  }
+
   public enum Stage {
     STARTING,
     COMPLETE,

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java

@@ -172,10 +172,11 @@ private ResolutionResult parseDependencies(
     Map<Coordinates, String> coordinateHashes = new HashMap<>();
     // Track artifacts per node so we can inspect POM files for relocation later
     Map<Coordinates, List<GradleResolvedArtifact>> artifactsByNode = new HashMap<>();
+    Map<Coordinates, Path> paths = new HashMap<>();
     List<GradleResolvedDependency> implementationDependencies = resolved.getResolvedDependencies();
     List<GradleUnresolvedDependency> unresolvedDependencies = resolved.getUnresolvedDependencies();
     if (implementationDependencies == null) {
-      return new ResolutionResult(graph, null);
+      return new ResolutionResult(graph, Set.of(), Map.of());
     }
 
     for (GradleResolvedDependency dependency : implementationDependencies) {
@@ -203,6 +204,12 @@ private ResolutionResult parseDependencies(
                 gradleCoordinates.getVersion());
         // Track artifact for this node
         artifactsByNode.computeIfAbsent(coordinates, k -> new ArrayList<>()).add(artifact);
+
+        File artifactFile = artifact.getFile();
+        if (artifactFile != null && artifactFile.exists()) {
+          paths.put(coordinates, artifactFile.toPath());
+        }
+
         addDependency(
             graph, coordinates, dependency, conflicts, requestedDepKeys, visited, artifactsByNode);
         // if there's a conflict and the conflicting version isn't one that's actually requested
@@ -270,7 +277,7 @@ private ResolutionResult parseDependencies(
     // After building the graph, contract relocation stubs (keep aggregating POMs)
     collapseRelocations(graph, coordinateHashes, conflicts, artifactsByNode);
 
-    return new ResolutionResult(graph, conflicts);
+    return new ResolutionResult(graph, conflicts, paths);
   }
 
   private String makeDepKey(String group, String artifact, String version) {

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/maven/MavenResolver.java

@@ -267,7 +267,7 @@ public ResolutionResult resolve(ResolutionRequest request) {
             getConflicts(request.getDependencies(), resolvedDependencies),
             graphNormalizationResult.getConflicts());
 
-    return new ResolutionResult(graphNormalizationResult.getNormalizedGraph(), conflicts);
+    return new ResolutionResult(graphNormalizationResult.getNormalizedGraph(), conflicts, Map.of());
   }
 
   private GraphNormalizationResult makeVersionsConsistent(Graph<Coordinates> dependencyGraph) {

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java

@@ -32,6 +32,7 @@
 import java.nio.file.Paths;
 import java.util.Collection;
 import java.util.LinkedHashSet;
+import java.util.Map;
 import java.util.Set;
 import java.util.logging.Logger;
 import org.apache.maven.model.Model;
@@ -53,17 +54,20 @@ public class Downloader {
   private final Set<URI> repos;
   private final boolean cacheDownloads;
   private final HttpDownloader httpDownloader;
+  private final Map<Coordinates, Path> knownPaths;
 
   public Downloader(
       Netrc netrc,
       Path localRepository,
       Collection<URI> repositories,
       EventListener listener,
-      boolean cacheDownloads) {
+      boolean cacheDownloads,
+      Map<Coordinates, Path> knownPaths) {
     this.localRepository = localRepository;
     this.repos = Set.copyOf(repositories);
     this.cacheDownloads = cacheDownloads;
     this.httpDownloader = new HttpDownloader(netrc, listener);
+    this.knownPaths = knownPaths != null ? Map.copyOf(knownPaths) : Map.of();
   }
 
   public DownloadResult download(Coordinates coords) {
@@ -119,11 +123,16 @@ private DownloadResult performDownload(Coordinates coordsToUse, String path) {
     Set<URI> repos = new LinkedHashSet<>();
 
     Path pathInRepo = null;
-
-    // Check the local cache for the path first
-    Path cachedResult = localRepository.resolve(path);
-    if (Files.exists(cachedResult)) {
-      pathInRepo = cachedResult;
+    Path knownPath = knownPaths.get(coordsToUse);
+
+    if (knownPath != null && Files.exists(knownPath)) {
+      pathInRepo = knownPath;
+    } else {
+      // Check the local cache for the path first
+      Path cachedResult = localRepository.resolve(path);
+      if (Files.exists(cachedResult)) {
+        pathInRepo = cachedResult;
+      }
     }
 
     String rjeAssumePresent = System.getenv("RJE_ASSUME_PRESENT");
@@ -141,6 +150,7 @@ private DownloadResult performDownload(Coordinates coordsToUse, String path) {
           repos.add(repo);
           downloaded = true;
 
+          Path cachedResult = localRepository.resolve(path);
           if (cacheDownloads && !cachedResult.equals(pathInRepo)) {
             try {
               Files.createDirectories(cachedResult.getParent());
@@ -150,10 +160,10 @@ private DownloadResult performDownload(Coordinates coordsToUse, String path) {
             }
           }
         }
-      } else if (assumedDownloaded) { // path is set
+      } else if (assumedDownloaded) {
         LOG.fine(String.format("Assuming %s is cached%n", coordsToUse));
         downloaded = true;
-      } else if (httpDownloader.head(buildUri(repo, path))) { // path is set
+      } else if (httpDownloader.head(buildUri(repo, path))) {
         LOG.fine(String.format("Checking head of %s%n", coordsToUse));
         repos.add(repo);
         downloaded = true;

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/HttpDownloader.java

@@ -159,7 +159,7 @@ private <X> HttpResponse<X> makeRequest(
 
   private <X> HttpResponse<X> doRequest(
       int attemptCount, HttpRequest request, HttpResponse.BodyHandler<X> handler) {
-    listener.onEvent(new DownloadEvent(STARTING, request.uri().toString()));
+    listener.onEvent(new DownloadEvent(STARTING, request.method(), request.uri().toString()));
     LOG.fine(String.format("Downloading (attempt %d): %s", attemptCount, request.uri()));
 
     // Slight pause, in case a previous attempt overwhelmed a server. We may be about to do it
@@ -209,7 +209,7 @@ private <X> HttpResponse<X> doRequest(
       // Don't panic. Just have another go.
 
       if (attemptCount < MAX_RETRY_COUNT) {
-        listener.onEvent(new DownloadEvent(COMPLETE, request.uri().toString()));
+        listener.onEvent(new DownloadEvent(COMPLETE, request.method(), request.uri().toString()));
         return doRequest(++attemptCount, request, handler);
       }
 
@@ -221,7 +221,7 @@ private <X> HttpResponse<X> doRequest(
       throw new RuntimeException(e);
     } finally {
       LOG.fine(String.format("Downloaded (attempt %d): %s", attemptCount, request.uri()));
-      listener.onEvent(new DownloadEvent(COMPLETE, request.uri().toString()));
+      listener.onEvent(new DownloadEvent(COMPLETE, request.method(), request.uri().toString()));
     }
   }
 

private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ui/PlainConsoleListener.java

@@ -29,7 +29,9 @@ public void onEvent(Event event) {
     if (event instanceof DownloadEvent) {
       DownloadEvent de = (DownloadEvent) event;
       if (de.getStage() == STARTING) {
-        System.err.println("Downloading: " + de.getTarget());
+        String method = de.getMethod();
+        String prefix = method != null ? method + " " : "";
+        System.err.println(prefix + de.getTarget());
       }
     }
 

tests/com/github/bazelbuild/rules_jvm_external/resolver/ResolverTestBase.java

@@ -252,7 +252,12 @@ public void shouldHandlePackagingPomsInDependencies() throws IOException {
 
     DownloadResult parentDownload =
         new Downloader(
-                Netrc.fromUserHome(), localRepo, Set.of(repo.toUri()), new NullListener(), false)
+                Netrc.fromUserHome(),
+                localRepo,
+                Set.of(repo.toUri()),
+                new NullListener(),
+                false,
+                Map.of())
             .download(parentCoords);
 
     assertTrue(parentDownload.getPath().isEmpty());