Skip to content

Commit afcd86b

Browse files
shs96cshs96c
committed
Handle GMM types more gracefully
1 parent 5482558 commit afcd86b

10 files changed

Lines changed: 263 additions & 6 deletions

File tree

‎private/rules/v3_lock_file.bzl‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,23 @@ def _compute_lock_file_hash_v3_impl(lock_file_contents, repo_keys):
151151

152152
for repo in repo_keys:
153153
for artifact in lock_file_contents["repositories"][repo]:
154+
if artifact not in all_infos:
155+
# Mirrors the guard in AbstractMain.calculateArtifactHash: an
156+
# artifact may be tracked in `repositories` under an un-suffixed
157+
# coord but only registered in `all_infos` under a classifier-
158+
# suffixed key (e.g. shasums = {"unshaded": ...}). Skip rather
159+
# than fail; the BUILD-file generator still has the suffixed
160+
# entry to emit a target.
161+
# buildifier: disable=print
162+
print("[WARNING]: skipping repository assignment for '%s' (referenced by repository '%s' but not present in the artifact set)" % (artifact, repo))
163+
continue
154164
all_infos[artifact]["repository"] = repo
155165

156166
for dep, dep_info in lock_file_contents["dependencies"].items():
167+
if dep not in all_infos:
168+
# buildifier: disable=print
169+
print("[WARNING]: skipping dependencies for '%s' (declares %d dependencies but is not present in the artifact set)" % (dep, len(dep_info)))
170+
continue
157171
all_infos[dep]["dependencies"] = sorted(dep_info)
158172

159173
return _compute_final_hash(all_infos)
@@ -231,9 +245,37 @@ def _from_key(key, spoofed_version):
231245

232246
return to_return
233247

248+
def _artifact_keys(raw_artifacts):
249+
keys = {}
250+
for (root, data) in raw_artifacts.items():
251+
parts = root.split(":")
252+
root_unpacked = {
253+
"group": parts[0],
254+
"artifact": parts[1],
255+
"version": data["version"],
256+
}
257+
if len(parts) > 2:
258+
root_unpacked["packaging"] = parts[2]
259+
else:
260+
root_unpacked["packaging"] = "jar"
261+
262+
for classifier in data.get("shasums", {}).keys():
263+
root_unpacked["classifier"] = classifier
264+
keys[to_key(root_unpacked)] = True
265+
return keys
266+
267+
def _filter_dependency_targets(raw_dependencies, valid_keys):
268+
filtered_dependencies = {}
269+
for (dep, dependencies) in raw_dependencies.items():
270+
filtered_dependencies[dep] = [target for target in dependencies if target in valid_keys]
271+
return filtered_dependencies
272+
234273
def _get_artifacts(lock_file_contents):
235274
raw_artifacts = lock_file_contents.get("artifacts", {})
236-
dependencies = lock_file_contents.get("dependencies", {})
275+
dependencies = _filter_dependency_targets(
276+
lock_file_contents.get("dependencies", {}),
277+
_artifact_keys(raw_artifacts),
278+
)
237279
repositories = lock_file_contents.get("repositories", {})
238280
files = lock_file_contents.get("files", {})
239281
skipped = lock_file_contents.get("skipped", [])

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

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.github.bazelbuild.rules_jvm_external.resolver.ResolutionResult;
2727
import com.github.bazelbuild.rules_jvm_external.resolver.Resolver;
2828
import com.github.bazelbuild.rules_jvm_external.resolver.events.EventListener;
29+
import com.github.bazelbuild.rules_jvm_external.resolver.events.LogEvent;
2930
import com.github.bazelbuild.rules_jvm_external.resolver.events.PhaseEvent;
3031
import com.github.bazelbuild.rules_jvm_external.resolver.lockfile.DependencyIndex;
3132
import com.github.bazelbuild.rules_jvm_external.resolver.lockfile.V3LockFile;
@@ -281,7 +282,7 @@ private static void writeLockFile(
281282

282283
if (config.getInputHash() != null) {
283284
toReturn.put("__INPUT_ARTIFACTS_HASH", config.getInputHash());
284-
toReturn.put("__RESOLVED_ARTIFACTS_HASH", calculateArtifactHash(rendered));
285+
toReturn.put("__RESOLVED_ARTIFACTS_HASH", calculateArtifactHash(listener, rendered));
285286
}
286287

287288
String converted =
@@ -312,7 +313,8 @@ private static void writeDependencyIndex(ResolverConfig config, Set<DependencyIn
312313
}
313314

314315
@SuppressWarnings("unchecked")
315-
public static Map<String, Integer> calculateArtifactHash(Map<String, Object> rendered) {
316+
public static Map<String, Integer> calculateArtifactHash(
317+
EventListener listener, Map<String, Object> rendered) {
316318
Map<String, Map<String, Object>> allInfos = new LinkedHashMap<>();
317319

318320
Map<String, Map<String, Object>> artifacts =
@@ -345,14 +347,38 @@ public static Map<String, Integer> calculateArtifactHash(Map<String, Object> ren
345347
for (Map.Entry<String, Iterable<String>> repo : repositories.entrySet()) {
346348
Iterable<String> repoArtifacts = repo.getValue();
347349
for (String art : repoArtifacts) {
348-
allInfos.get(art).put("repository", repo.getKey());
350+
Map<String, Object> info = allInfos.get(art);
351+
if (info == null) {
352+
listener.onEvent(
353+
new LogEvent(
354+
"resolver",
355+
String.format(
356+
"skipping repository assignment for '%s' (referenced by repository '%s' but"
357+
+ " not present in the artifact set)",
358+
art, repo.getKey()),
359+
null));
360+
continue;
361+
}
362+
info.put("repository", repo.getKey());
349363
}
350364
}
351365

352366
Map<String, Set<String>> dependencies =
353367
sortMapRecursively((Map<?, ?>) rendered.get("dependencies"));
354368
for (Map.Entry<String, Set<String>> dep : dependencies.entrySet()) {
355-
allInfos.get(dep.getKey()).put("dependencies", dep.getValue());
369+
Map<String, Object> info = allInfos.get(dep.getKey());
370+
if (info == null) {
371+
listener.onEvent(
372+
new LogEvent(
373+
"resolver",
374+
String.format(
375+
"skipping dependencies for '%s' (declares %d dependencies but is not present in"
376+
+ " the artifact set)",
377+
dep.getKey(), dep.getValue().size()),
378+
null));
379+
continue;
380+
}
381+
info.put("dependencies", dep.getValue());
356382
}
357383

358384
Map<String, Integer> finalHash = new TreeMap<>();

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,13 @@ private DownloadResult performDownload(Coordinates coordsToUse, String path) {
126126
Path knownPath = knownPaths.get(coordsToUse);
127127

128128
if (knownPath != null && Files.exists(knownPath)) {
129+
// Some resolvers can associate a module's POM with a non-POM coordinate
130+
// (for example a Gradle metadata umbrella that delegates to a JVM sibling).
131+
// Treat those as "no binary for this coordinate" so the lockfile renders a
132+
// wrapper target instead of hashing the POM as though it were the module JAR.
133+
if (isPomPathForNonPomCoordinates(coordsToUse, knownPath)) {
134+
return new DownloadResult(coordsToUse, Set.of(), null, null);
135+
}
129136
pathInRepo = knownPath;
130137
} else {
131138
// Check the local cache for the path first
@@ -221,6 +228,10 @@ private boolean isFallbackAvailable(Coordinates coords) {
221228
return !JAR_PACKAGINGS.contains(extension);
222229
}
223230

231+
private boolean isPomPathForNonPomCoordinates(Coordinates coords, Path path) {
232+
return path.getFileName().toString().endsWith(".pom") && !"pom".equals(coords.getExtension());
233+
}
234+
224235
private String calculateSha256(Path path) {
225236
try {
226237
byte[] bytes = Files.readAllBytes(path);

‎tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ java_test(
3838
"//private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle",
3939
"//private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/models",
4040
"//private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/netrc",
41+
"//private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote",
42+
"//private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ui",
4143
"//tests/com/github/bazelbuild/rules_jvm_external/resolver",
4244
artifact(
4345
"junit:junit",

‎tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolverTest.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@
2727
import com.github.bazelbuild.rules_jvm_external.resolver.cmd.ResolverConfig;
2828
import com.github.bazelbuild.rules_jvm_external.resolver.events.EventListener;
2929
import com.github.bazelbuild.rules_jvm_external.resolver.netrc.Netrc;
30+
import com.github.bazelbuild.rules_jvm_external.resolver.remote.DownloadResult;
31+
import com.github.bazelbuild.rules_jvm_external.resolver.remote.Downloader;
32+
import com.github.bazelbuild.rules_jvm_external.resolver.ui.NullListener;
3033
import com.google.common.graph.Graph;
3134
import com.google.devtools.build.runfiles.AutoBazelRepository;
3235
import com.google.devtools.build.runfiles.Runfiles;
@@ -136,6 +139,62 @@ public void resolvesJvmButNotAndroidVariant() throws IOException, XMLStreamExcep
136139
assertEquals(Set.of(baseCoordinates, jvmCoordinates), resolved.nodes());
137140
}
138141

142+
@Test
143+
public void gmmUmbrellaWithOnlyPomKnownPathBecomesNoBinaryWrapper()
144+
throws IOException, XMLStreamException {
145+
Coordinates baseCoordinates = new Coordinates("com.example:sample:1.0");
146+
Coordinates jvmCoordinates = new Coordinates("com.example:sample-jvm:1.0");
147+
MavenRepo mavenRepo = MavenRepo.create();
148+
GradleModuleMetadataHelper moduleMetadataHelper = new GradleModuleMetadataHelper(mavenRepo);
149+
150+
Runfiles runfiles =
151+
Runfiles.preload().withSourceRepository(AutoBazelRepository_GradleResolverTest.NAME);
152+
Path baseMetadataPath =
153+
Paths.get(
154+
runfiles.rlocation(
155+
"rules_jvm_external/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/fixtures/simpleJvmVariant/sample-1.0.module"));
156+
moduleMetadataHelper.addToMavenRepo(baseCoordinates, Files.readString(baseMetadataPath));
157+
158+
Path jvmMetadataPath =
159+
Paths.get(
160+
runfiles.rlocation(
161+
"rules_jvm_external/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/fixtures/simpleJvmVariant/sample-jvm-1.0.module"));
162+
moduleMetadataHelper.addToMavenRepo(jvmCoordinates, Files.readString(jvmMetadataPath));
163+
164+
// Real GMM umbrellas can publish a POM and module metadata without Gradle resolving a base
165+
// JAR for the umbrella coordinate. Simulate that shape by removing the umbrella JAR.
166+
Files.delete(mavenRepo.getPath().resolve(baseCoordinates.toRepoPath()));
167+
168+
ResolutionResult result =
169+
resolver.resolve(prepareRequestFor(mavenRepo.getPath().toUri(), baseCoordinates));
170+
assertEquals(Set.of(baseCoordinates, jvmCoordinates), result.getResolution().nodes());
171+
172+
Path localRepo = Files.createTempDirectory("local");
173+
DownloadResult baseDownload =
174+
new Downloader(
175+
Netrc.fromUserHome(),
176+
localRepo,
177+
Set.of(mavenRepo.getPath().toUri()),
178+
new NullListener(),
179+
false,
180+
result.getPaths())
181+
.download(baseCoordinates);
182+
assertTrue(baseDownload.getPath().isEmpty());
183+
assertTrue(baseDownload.getSha256().isEmpty());
184+
185+
DownloadResult jvmDownload =
186+
new Downloader(
187+
Netrc.fromUserHome(),
188+
localRepo,
189+
Set.of(mavenRepo.getPath().toUri()),
190+
new NullListener(),
191+
false,
192+
result.getPaths())
193+
.download(jvmCoordinates);
194+
assertTrue(jvmDownload.getPath().isPresent());
195+
assertTrue(jvmDownload.getSha256().isPresent());
196+
}
197+
139198
@Test
140199
public void throwsAnExceptionIfASingleDependencyWasNotResolved() throws IOException {
141200
Coordinates validCoordinates = new Coordinates("com.example:sample:1.0");

‎tests/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,5 +42,6 @@ java_test(
4242
),
4343
artifact("org.hamcrest:hamcrest"),
4444
artifact("org.hamcrest:hamcrest_core"),
45+
"//private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/ui",
4546
],
4647
)

‎tests/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFileTest.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import com.github.bazelbuild.rules_jvm_external.resolver.Conflict;
2424
import com.github.bazelbuild.rules_jvm_external.resolver.DependencyInfo;
2525
import com.github.bazelbuild.rules_jvm_external.resolver.cmd.AbstractMain;
26+
import com.github.bazelbuild.rules_jvm_external.resolver.ui.NullListener;
2627
import com.google.gson.Gson;
2728
import com.google.gson.GsonBuilder;
2829
import java.io.IOException;
@@ -236,7 +237,8 @@ public void testCalculateArtifactHashMatchesStoredHash() throws IOException {
236237
}
237238
lockFileData.put("dependencies", convertedDeps);
238239

239-
Map<String, Integer> calculatedHash = AbstractMain.calculateArtifactHash(lockFileData);
240+
Map<String, Integer> calculatedHash =
241+
AbstractMain.calculateArtifactHash(new NullListener(), lockFileData);
240242

241243
assertEquals(
242244
"Hash mismatch: calculated hash does not match stored hash",

‎tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,32 @@
3030
import org.junit.Test;
3131

3232
public class DownloaderTest {
33+
@Test
34+
public void downloaderTreatsPomKnownPathAsNoBinaryForNonPomCoordinate() throws IOException {
35+
Coordinates coords = new Coordinates("com.example:sample:1.0");
36+
37+
MavenRepo repo = MavenRepo.create().add(coords);
38+
Path pomPath =
39+
repo.getPath()
40+
.resolve(coords.toRepoPath())
41+
.getParent()
42+
.resolve(coords.getArtifactId() + "-" + coords.getVersion() + ".pom");
43+
Path localRepo = Files.createTempDirectory("local");
44+
45+
DownloadResult downloadResult =
46+
new Downloader(
47+
Netrc.fromUserHome(),
48+
localRepo,
49+
Set.of(repo.getPath().toUri()),
50+
new NullListener(),
51+
false,
52+
Map.of(coords, pomPath))
53+
.download(coords);
54+
55+
assertTrue(downloadResult.getPath().isEmpty());
56+
assertTrue(downloadResult.getSha256().isEmpty());
57+
}
58+
3359
@Test
3460
public void downloaderHandleUndeclaredCharacterEntityInPOM() throws IOException {
3561
Coordinates coords = new Coordinates("com.example:characterentity:1.0");

‎tests/unit/BUILD‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ load(":java_utilities_test.bzl", "java_utilities_test_suite")
66
load(":maven_version_test.bzl", "maven_version_test_suite")
77
load(":proxy_test.bzl", "proxy_test_suite")
88
load(":specs_test.bzl", "artifact_specs_test_suite")
9+
load(":v3_lock_file_test.bzl", "v3_lock_file_test_suite")
910
load(":version_catalogs_test.bzl", "version_catalogs_test_suite")
1011

1112
artifact_specs_test_suite()
@@ -24,4 +25,6 @@ maven_version_test_suite()
2425

2526
proxy_test_suite()
2627

28+
v3_lock_file_test_suite()
29+
2730
version_catalogs_test_suite()

‎tests/unit/v3_lock_file_test.bzl‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
2+
load("//private/rules:v3_lock_file.bzl", "v3_lock_file")
3+
4+
def _get_artifacts_ignores_dependency_targets_without_artifacts_test_impl(ctx):
5+
env = unittest.begin(ctx)
6+
7+
artifacts = v3_lock_file.get_artifacts({
8+
"artifacts": {
9+
"com.example:dep": {
10+
"shasums": {
11+
"jar": "def",
12+
},
13+
"version": "1.0",
14+
},
15+
"com.example:root": {
16+
"shasums": {
17+
"jar": "abc",
18+
},
19+
"version": "1.0",
20+
},
21+
},
22+
"dependencies": {
23+
"com.example:root": [
24+
"com.example:dep",
25+
"com.example:missing-native",
26+
],
27+
},
28+
"repositories": {},
29+
"services": {},
30+
"version": "3",
31+
})
32+
33+
root = [artifact for artifact in artifacts if artifact["coordinates"] == "com.example:root:1.0"][0]
34+
asserts.equals(env, ["com.example:dep:spoofed-version"], root["deps"])
35+
36+
return unittest.end(env)
37+
38+
get_artifacts_ignores_dependency_targets_without_artifacts_test = unittest.make(
39+
_get_artifacts_ignores_dependency_targets_without_artifacts_test_impl,
40+
)
41+
42+
def _get_artifacts_keeps_classifier_dependency_targets_test_impl(ctx):
43+
env = unittest.begin(ctx)
44+
45+
artifacts = v3_lock_file.get_artifacts({
46+
"artifacts": {
47+
"com.example:dep": {
48+
"shasums": {
49+
"test-fixtures": "def",
50+
},
51+
"version": "1.0",
52+
},
53+
"com.example:root": {
54+
"shasums": {
55+
"jar": "abc",
56+
},
57+
"version": "1.0",
58+
},
59+
},
60+
"dependencies": {
61+
"com.example:root": [
62+
"com.example:dep:jar:test-fixtures",
63+
"com.example:missing-native",
64+
],
65+
},
66+
"repositories": {},
67+
"services": {},
68+
"version": "3",
69+
})
70+
71+
root = [artifact for artifact in artifacts if artifact["coordinates"] == "com.example:root:1.0"][0]
72+
asserts.equals(env, ["com.example:dep:spoofed-version:test-fixtures@jar"], root["deps"])
73+
74+
return unittest.end(env)
75+
76+
get_artifacts_keeps_classifier_dependency_targets_test = unittest.make(
77+
_get_artifacts_keeps_classifier_dependency_targets_test_impl,
78+
)
79+
80+
def v3_lock_file_test_suite():
81+
unittest.suite(
82+
"v3_lock_file_tests",
83+
get_artifacts_ignores_dependency_targets_without_artifacts_test,
84+
get_artifacts_keeps_classifier_dependency_targets_test,
85+
)

0 commit comments

Comments
 (0)