search

bazelbuild / rules_jvm_external

Bazel rules to resolve, fetch and export Maven artifacts
Apache License 2.0
316 stars 236 forks source link

Use MavenXpp3Reader to parse pom.xml files because it handles undeclared character entities #1156

Closed cheister closed 3 weeks ago

cheister commented 1 month ago

Fix https://github.com/bazelbuild/rules_jvm_external/issues/1144 and https://github.com/bazelbuild/rules_jvm_external/issues/1152

tharakadesilva commented 4 weeks ago

Thanks for this @cheister! I tried porting this as a patch and encountered the following error:

WARN org.eclipse.aether.internal.impl.DefaultRepositoryEventDispatcher - Failed to dispatch repository event to com.github.bazelbuild.rules_jvm_external.resolver.maven.CompoundListener
java.lang.RuntimeException: Unable to determine packaging
        at com.github.bazelbuild.rules_jvm_external.resolver.maven.CoordinateGatheringListener.artifactResolved(CoordinateGatheringListener.java:86)
        at com.github.bazelbuild.rules_jvm_external.resolver.maven.CompoundListener.lambda$artifactResolved$3(CompoundListener.java:46)
        at java.base/java.lang.Iterable.forEach(Iterable.java:75)
        at com.github.bazelbuild.rules_jvm_external.resolver.maven.CompoundListener.artifactResolved(CompoundListener.java:46)
        at org.eclipse.aether.internal.impl.DefaultRepositoryEventDispatcher.dispatch(DefaultRepositoryEventDispatcher.java:120)
        at org.eclipse.aether.internal.impl.DefaultRepositoryEventDispatcher.dispatch(DefaultRepositoryEventDispatcher.java:88)
        at org.eclipse.aether.internal.impl.DefaultArtifactResolver.artifactResolved(DefaultArtifactResolver.java:669)
        at org.eclipse.aether.internal.impl.DefaultArtifactResolver.evaluateDownloads(DefaultArtifactResolver.java:641)
        at org.eclipse.aether.internal.impl.DefaultArtifactResolver.performDownloads(DefaultArtifactResolver.java:545)
        at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:449)
        at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifacts(DefaultArtifactResolver.java:261)
        at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifact(DefaultArtifactResolver.java:243)
        at org.apache.maven.repository.internal.DefaultModelResolver.resolveModel(DefaultModelResolver.java:158)
        at org.apache.maven.repository.internal.DefaultModelResolver.resolveModel(DefaultModelResolver.java:204)
        at org.apache.maven.model.building.DefaultModelBuilder.readParentExternally(DefaultModelBuilder.java:1009)
        at org.apache.maven.model.building.DefaultModelBuilder.readParent(DefaultModelBuilder.java:801)
        at org.apache.maven.model.building.DefaultModelBuilder.build(DefaultModelBuilder.java:327)
        at org.apache.maven.model.building.DefaultModelBuilder.build(DefaultModelBuilder.java:243)
        at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.loadPom(DefaultArtifactDescriptorReader.java:284)
        at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.readArtifactDescriptor(DefaultArtifactDescriptorReader.java:175)
        at org.eclipse.aether.internal.impl.collect.bf.BfDependencyCollector.resolveCachedArtifactDescriptor(BfDependencyCollector.java:464)
        at org.eclipse.aether.internal.impl.collect.bf.BfDependencyCollector.resolveDescriptorForVersion(BfDependencyCollector.java:450)
        at org.eclipse.aether.internal.impl.collect.bf.BfDependencyCollector.lambda$resolveArtifactDescriptorAsync$1(BfDependencyCollector.java:417)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
        at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
        at org.eclipse.aether.internal.impl.collect.bf.BfDependencyCollector.lambda$resolveArtifactDescriptorAsync$4(BfDependencyCollector.java:416)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: org.codehaus.plexus.util.xml.pull.XmlPullParserException: UTF-8 BOM plus xml decl of ISO-8859-1 is incompatible (position: START_DOCUMENT seen <?xml version="1.0" encoding="ISO-8859-1"... @1:42) 
        at org.codehaus.plexus.util.xml.pull.MXParser.parseXmlDeclWithVersion(MXParser.java:3439)
        at org.codehaus.plexus.util.xml.pull.MXParser.parseXmlDecl(MXParser.java:3361)
        at org.codehaus.plexus.util.xml.pull.MXParser.parsePI(MXParser.java:3213)
        at org.codehaus.plexus.util.xml.pull.MXParser.parseProlog(MXParser.java:1828)
        at org.codehaus.plexus.util.xml.pull.MXParser.nextImpl(MXParser.java:1757)
        at org.codehaus.plexus.util.xml.pull.MXParser.next(MXParser.java:1375)
Currently: resolving and fetching the transitive closure of 586 artifact(s)

The coordinate that is having ISO-8859-1 is org.hamcrest:hamcrest-core:1.3.

tharakadesilva commented 4 weeks ago

I also have RJE_UNSAFE_CACHE=1 and I saw a bunch of (might be unrelated):

[ParallelDescriptorResolver-0-9] INFO org.eclipse.aether.internal.impl.DefaultArtifactResolver - Artifact com.github.luben:zstd-jni:pom:1.4.5-6 is present in the local repository, but cached from a remote repository ID that is unavailable in current build context, verifying that is downloadable from [file:///Users/tdesilva/.m2/repository/ (file:///Users/tdesilva/.m2/repository/, default, releases), https://artifactory.com/artifactory/maven/ (https://artifactory.com/artifactory/maven/, default, releases), https://artifactory.com/artifactory/maven-snapshots/ (https://artifactory.com/artifactory/maven-snapshots/, default, releases)]
shs96c commented 3 weeks ago

Nit: can we please have a reduced test case added to ResolverTestBase too?

cheister commented 3 weeks ago

The coordinate that is having ISO-8859-1 is org.hamcrest:hamcrest-core:1.3.

I added this test case and included a fix for it as well.

tharakadesilva commented 3 weeks ago

The coordinate that is having ISO-8859-1 is org.hamcrest:hamcrest-core:1.3.

I added this test case and included a fix for it as well.

Tested and things are working as expected!! 🔥 Thanks @cheister!!!

cheister commented 3 weeks ago

Nit: can we please have a reduced test case added to ResolverTestBase too?

I wrote some unit tests but I don't think they belong in ResolverTestBase. I'll merge this PR and open another PR with the proposed unit test changes.