-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19402. [JDK11] JDiff Support JDK11. #8038
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
Conversation
|
@steveloughran @pan3793 This PR has been in preparation for quite some time by Hualong, and it consists of two main parts:
I’d appreciate your help reviewing this PR. |
|
💔 -1 overall
This message was automatically generated. |
| </build> | ||
| </profile> | ||
| <profile> | ||
| <id>jdk17</id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <id>jdk17</id> | |
| <id>jdk17+</id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review; I’ll update the code promptly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhtttylz Could you please test this on JDK 21 to confirm it passes? Thanks a lot.
|
given trunk is now java17+ only, do we need to have a java17 source tree and profile? We can just delete all java8 code and put this in its place. I would support this as it keeps the codebase cleaner |
Agreed. I’ll remove all Java 8 code and replace it with the Java 17 implementation. |
|
@zhtttylz Thank you for your contribution! |
8f9c11f to
81bf9d4
Compare
|
💔 -1 overall
This message was automatically generated. |
81bf9d4 to
0bc74ea
Compare
cnauroth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this PR look good to me.
Related JDiff changes (temporary local test build; JDK 17 adaptation implemented)
I'm a little unclear on how we proceed with committing. Do we need to make a new JDiff release somehow first?
Thanks for flagging the release/workflow concern, Chris. I’ve updated this PR to consume the published Maven Central artifact (no locally installed JDiff needed):
The PR description still references the earlier local build; I’ll update it shortly to reflect the released dependency. |
|
@zhtttylz Thanks for completing the JDK 17 compatibility work for jdiff. Overall, this is a solid improvement, so I’m giving it an initial +1, though there are still a few minor issues that need to be addressed. |
cnauroth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @zhtttylz . I'll be ready to approve this after resolving the last issues flagged by pre-commits.
0bc74ea to
20b02b6
Compare
|
💔 -1 overall
This message was automatically generated. |
ef471d0 to
5b2cba6
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
5b2cba6 to
ecd6a35
Compare
|
💔 -1 overall
This message was automatically generated. |
|
@cnauroth Do you have any other suggestions for this PR? |
@slfan1989 , +1 from me. Thank you @zhtttylz . |
|
@zhtttylz Thanks for the contribution! @cnauroth @steveloughran Thanks for the review! |
|
Thanks @slfan1989 for merging, and thanks @cnauroth @steveloughran @pan3793 for the reviews! I’ll keep monitoring the JDK 17 docs CI and will follow up on any regressions. |
|
Maybe I'm hopelessly out of date in terms of the JDK policy, but a simple compile of the trunk (vanilla mvn clean install) is failing because "--add-modules" is not recognized. And I believe it's because we're not making a certain compiler version mandatory in the trunk build. Is it now supposed to be JDK 11? If so, shouldn't we add that to the top pom.xml to be very explicit? cc @zhtttylz @cnauroth |
|
@zhtttylz I mean |
@pan3793 Thanks for pointing that out — website.yml makes sense for the GitHub Actions side. Does that mean the default JDK here is coming from the Jenkins/Yetus Docker image rather than website.yml? |
|
it's different things, GitHub Actions results are located at https://github.com/apache/hadoop/actions/runs/20689444287/job/59395332436 |
|
Is |
@sjlee Apologies for the late reply. The situation is consistent with @pan3793 feedback. We will completely discontinue support for JDK8 on the trunk and transition to JDK17 in the future. jdiff is a feature we need to support on JDK17, as it involves the final jdiff comparison XML file for the release, such as If there are any new issues, feel free to provide feedback here anytime. |
Opened #8174 to fix website.yml |
Description of PR
JIRA:HADOOP-19402. [JDK11] JDiff Support JDK11.
This PR restores the ability to generate JDiff API XML on JDK 17 by porting Hadoop’s in‑tree doclets to the modern JDK 9+ Doclet API (
jdk.javadoc.doclet), while preserving the legacy command‑line surface and XML output contract used in our release process.Background. The original JDiff tool and its legacy doclet APIs predate JPMS and JDK 9; the upstream artifact (
jdiff:1.0.9) is from 2005 and does not support JDK 9+ doclet APIs. Hadoop’s release process, however, still depends on JDiff XML to publish API diffs between releases.Why now. Since JEP 221 (JDK 9), the standard
com.sun.javadocdoclet path was superseded byjdk.javadoc.doclet.*, and the old doclet was eventually removed/replaced in modern JDKs. To run on JDK 17, we need doclets implemented against the new API.How was this patch tested?
Built with JDK 17 and the docs profile to ensure JDiff XML is emitted.
This produces JDiff XML for each module. For example, HDFS:
Apache_Hadoop_HDFS_3.5.0-SNAPSHOT.xml
For code changes:
All changes are scoped to the hadoop-annotations module and only affect build-time doclet code.
java17source tree or ajdk17Maven profile.src/main/java) and replace the oldcom.sun.javadoc-based code.ExcludePrivateAnnotationsJDiffDocletIncludePublicAnnotationsJDiffDocletExcludePrivateAnnotationsStandardDocletIncludePublicAnnotationsStandardDocletDoclet#run(DocletEnvironment).jdk.javadoc.doclet.StandardDocletto keep the same inclusion/exclusion semantics when producing regular Javadoc.RootDocProcessorandStabilityOptionsvariants that operate onDocletEnvironment, preserving:@InterfaceAudience.Private/LimitedPrivate)IncludePublic*doclets)unstable,evolving, etc.Related JDiff changes (published; reproducible)
I implemented the JDK 9+ Doclet API adaptation in JDiff and validated Hadoop’s docs build using the published Maven Central artifact (no local install needed):
io.github.zhtttylz:jdiff:1.1.2-hadoophttps://repo1.maven.org/maven2/io/github/zhtttylz/jdiff/1.1.2-hadoop/
The implementation is available in zhtttylz/jdiff#4.