Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi-release jar problem #11185

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

liurenjie1024
Copy link
Collaborator

Close #11175 .

Multi-release-jar is introduced in jdk to support conditional compilation. This fix includes two things:

  1. Fix maven shade plugin to support multi-release-jar, see https://stackoverflow.com/questions/53049346/is-log4j2-compatible-with-java-11/54713830#54713830
  2. Fix jacoco to stop complaining about it.

See apache/pulsar#22964 as an example.

Signed-off-by: liurenjie1024 <liurenjie2008@gmail.com>
@liurenjie1024
Copy link
Collaborator Author

build

@@ -197,7 +197,8 @@ git --no-pager diff --name-only HEAD \$BASE -- ${PREMERGE_DOCKERFILE} || true"""
execPattern : '**/target/jacoco.exec',
classPattern : 'target/jacoco_classes/',
sourceInclusionPattern: '**/*.java,**/*.scala',
sourcePattern : sourcePattern
sourcePattern : sourcePattern,
exclude : '*/META-INF/versions/*',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @NvTimLiu Could you help to confirm this part?

Copy link
Collaborator

@NvTimLiu NvTimLiu Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the exclude param should be exclusionPattern, let me double conform

Also exclude only not cp the dup classes under target/jacoco_classes/*/META-INF/versions/* dir, the due classes still exist in the dist jar

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this pr is not removing due classes, and they are in fact not duplicated classes, they are shim layer of jdk 11.

Copy link
Collaborator

@NvTimLiu NvTimLiu Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liurenjie1024
exclusionPattern : 'target/jacoco_classes/*/META-INF/versions/*', does not work here,
Looks like [Jacoco plugin] run below steps

1, firstly tries to gather all classes together;

2 ,then it excludes classes defined by exclusionPattern

The first step fails due to the dup ArraysShim.class

16:06:54  java.lang.IllegalStateException: Can't add different class with same name: com/nvidia/shaded/spark/org/roaringbitmap/ArraysShim
16:06:54  	at org.jacoco.core.analysis.CoverageBuilder.visitCoverage(CoverageBuilder.java:106)
16:06:54  	at org.jacoco.core.analysis.Analyzer$1.visitEnd(Analyzer.java:99)
16:06:54  	at org.objectweb.asm.ClassVisitor.visitEnd(ClassVisitor.java:377)
16:06:54  	at org.jacoco.core.internal.flow.ClassProbesAdapter.visitEnd(ClassProbesAdapter.java:100)
16:06:54  	at org.objectweb.asm.ClassReader.accept(ClassReader.java:725)
16:06:54  	at org.objectweb.asm.ClassReader.accept(ClassReader.java:401)
16:06:54  	at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:116)
16:06:54  	at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:132)
16:06:54  Caused: java.io.IOException: Error while analyzing /var/jenkins/jobs/rapids_premerge-github/builds/9756/jacoco/classes/spark-shared/com/nvidia/shaded/spark/org/roaringbitmap/ArraysShim.class.
16:06:54  	at org.jacoco.core.analysis.Analyzer.analyzerError(Analyzer.java:162)
16:06:54  	at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:134)
16:06:54  	at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:157)

I guess this is why instead of with param exclusionPattern for [Jacoco Plugin],
we remove folders in advance here: https://github.com/NVIDIA/spark-rapids/blob/branch-24.08/jenkins/spark-premerge-build.sh#L95-L96

BTW, the pre-merge CI failed, because the path of ArraysShim.class in sparkxxx/MATE-INFO has been changed with this PR:

spark320/META-INF/versions/11/org/roaringbitmap/ -- > /META-INF/versions/11/com/nvidia/shaded/spark/org/roaringbitmap/

So please help to update the line?

https://github.com/NVIDIA/spark-rapids/blob/branch-24.08/jenkins/spark-premerge-build.sh#L95

rm -rf com/nvidia/shaded/ org/openucx/          spark${SPK_VER}/META-INF/versions/*           spark-shared/com/nvidia/spark/udf/ spark${SPK_VER}/com/nvidia/spark/udf/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for update, I've fixed the script.

gerashegalov
gerashegalov previously approved these changes Jul 15, 2024
Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pom changes LGTM

@pxLi
Copy link
Collaborator

pxLi commented Jul 16, 2024

please also help revert the TODO (spark${SPK_VER}/META-INF/versions/*/org/roaringbitmap/) at
https://github.com/NVIDIA/spark-rapids/blob/branch-24.08/jenkins/spark-premerge-build.sh#L95-L96 thanks

@liurenjie1024
Copy link
Collaborator Author

please also help revert the TODO (spark${SPK_VER}/META-INF/versions/*/org/roaringbitmap/) at https://github.com/NVIDIA/spark-rapids/blob/branch-24.08/jenkins/spark-premerge-build.sh#L95-L96 thanks

Done, PTAL. cc @pxLi

@liurenjie1024
Copy link
Collaborator Author

build

pxLi
pxLi previously approved these changes Jul 16, 2024
Copy link
Collaborator

@pxLi pxLi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI passed, thanks

@NvTimLiu
Copy link
Collaborator

CI still failed.
As Jacoco's exclusion issue, see comment : #11185 (comment)
We need to change https://github.com/NVIDIA/spark-rapids/blob/branch-24.08/jenkins/spark-premerge-build.sh#L95 to

rm -rf ... spark${SPK_VER}/META-INF/versions/* ...

@liurenjie1024
Copy link
Collaborator Author

build

Copy link
Collaborator

@NvTimLiu NvTimLiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@liurenjie1024 liurenjie1024 merged commit 6d7d4df into NVIDIA:branch-24.08 Jul 17, 2024
43 checks passed
@liurenjie1024 liurenjie1024 deleted the ray/issue-11175 branch July 17, 2024 14:09
@sameerz sameerz added the build Related to CI / CD or cleanly building label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to CI / CD or cleanly building
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Clean up unused and duplicated 'org/roaringbitmap' folder in the spark3xx shims
5 participants