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"
diff --git a/private/rules/coursier.bzl b/private/rules/coursier.bzl index 84b83c8..73d54cf 100644 --- a/private/rules/coursier.bzl +++ b/private/rules/coursier.bzl
@@ -456,9 +456,11 @@ 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:
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ResolutionResult.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ResolutionResult.java index 820db70..52bb456 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ResolutionResult.java +++ b/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 @@ 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 Set<Conflict> getConflicts() { return conflicts; } + + public Map<Coordinates, Path> getPaths() { + return paths; + } }
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/cmd/AbstractMain.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/cmd/AbstractMain.java index 26559a6..2420111 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/cmd/AbstractMain.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/cmd/AbstractMain.java
@@ -70,7 +70,7 @@ 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 @@ 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 @@ 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(),
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/events/DownloadEvent.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/events/DownloadEvent.java index d1c89f2..06e541a 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/events/DownloadEvent.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/events/DownloadEvent.java
@@ -20,10 +20,16 @@ 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 @@ return target; } + public String getMethod() { + return method; + } + public enum Stage { STARTING, COMPLETE,
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java index b68cc2b..8d928bb 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java
@@ -172,10 +172,11 @@ 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 @@ 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 @@ // 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) {
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/maven/MavenResolver.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/maven/MavenResolver.java index 9b4f22e..24ed229 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/maven/MavenResolver.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/maven/MavenResolver.java
@@ -267,7 +267,7 @@ 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) {
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java index 746b8da..c5aa48e 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java +++ b/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 @@ 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 @@ Set<URI> repos = new LinkedHashSet<>(); Path pathInRepo = null; + Path knownPath = knownPaths.get(coordsToUse); - // Check the local cache for the path first - Path cachedResult = localRepository.resolve(path); - if (Files.exists(cachedResult)) { - pathInRepo = cachedResult; + 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 @@ repos.add(repo); downloaded = true; + Path cachedResult = localRepository.resolve(path); if (cacheDownloads && !cachedResult.equals(pathInRepo)) { try { Files.createDirectories(cachedResult.getParent()); @@ -150,10 +160,10 @@ } } } - } 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;
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/HttpDownloader.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/HttpDownloader.java index f5a18b1..9768965 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/HttpDownloader.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/HttpDownloader.java
@@ -159,7 +159,7 @@ 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 @@ // 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 @@ 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())); } }
diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ui/PlainConsoleListener.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ui/PlainConsoleListener.java index a4c597c..3039c94 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ui/PlainConsoleListener.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ui/PlainConsoleListener.java
@@ -29,7 +29,9 @@ 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()); } }
diff --git a/tests/com/github/bazelbuild/rules_jvm_external/resolver/ResolverTestBase.java b/tests/com/github/bazelbuild/rules_jvm_external/resolver/ResolverTestBase.java index 27953ab..6e9f318 100644 --- a/tests/com/github/bazelbuild/rules_jvm_external/resolver/ResolverTestBase.java +++ b/tests/com/github/bazelbuild/rules_jvm_external/resolver/ResolverTestBase.java
@@ -252,7 +252,12 @@ 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());
diff --git a/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java b/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java index 2e501a7..79aecf7 100644 --- a/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java +++ b/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java
@@ -25,6 +25,7 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Map; import java.util.Set; import org.junit.Test; @@ -58,7 +59,12 @@ DownloadResult downloadResult = 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(coords); assertTrue(downloadResult.getPath().isEmpty());