-
Notifications
You must be signed in to change notification settings - Fork 425
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
TEZ-4387: use new doclet API in Java 9+ #296
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@abstractdog If we can get this in, we might be able to have compile time support for JDK-17 |
yeah, let me take a look pushed again to get a new precommit result (old one is not available) |
46ac3dc
to
b2f0b8d
Compare
JDK17 compilation passed successfully locally, added a JDK17 build step to github actions |
This comment was marked as outdated.
This comment was marked as outdated.
b2f0b8d
to
6921a68
Compare
This comment was marked as outdated.
This comment was marked as outdated.
6921a68
to
9793457
Compare
This comment was marked as outdated.
This comment was marked as outdated.
9793457
to
74f7ec5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
e8d405e
to
253913a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
the current issue is that some modules are JDK17+ compatible, and are successfully skipped when you build the whole project, but the precommit enters into the folder of the changed submodule and builds it, hence the sub-module skipping won't work
|
253913a
to
d933382
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ae06048
to
a49cfc4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a49cfc4
to
0ba0316
Compare
This comment was marked as outdated.
This comment was marked as outdated.
0ba0316
to
761622d
Compare
latest patch is okay from precommit point of view, I need to check what's happening on JDK17 in terms of javadoc generation:
the latter command throws:
as the doc generation works on JDK8, maybe we can postpone the solution as a followup ticket, but I'm trying to handle it now hoping that it's an easy one |
This comment was marked as outdated.
This comment was marked as outdated.
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.
one checkstyle warning, rest looks good in the build.
@abstractdog as of now we are just compiling, how do we know if the tests would be fine with higher JDK versions?
...tools/tez-javadoc-tools-base/src/main/java/org/apache/tez/tools/javadoc/util/HtmlWriter.java
Outdated
Show resolved
Hide resolved
...tools/tez-javadoc-tools-base/src/main/java/org/apache/tez/tools/javadoc/util/HtmlWriter.java
Outdated
Show resolved
Hide resolved
thanks @ayushtkn for the comments, I'll address them currently I'm testing a different approach which doesn't involve separate modules, only separate folders in the original tez-javadoc-tools module, which looks a bit more lightweight |
This comment was marked as outdated.
This comment was marked as outdated.
compile time compatibility of tez-javadoc-tools is done |
This comment was marked as outdated.
This comment was marked as outdated.
latest precommit is clean (besides the already known TestAnalyzer failure) |
💔 -1 overall
This message was automatically generated. |
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.
Thanx @abstractdog for the changes, apart from the change in License stuff, changes LGTM
...vadoc-tools/src/main/java-8-16/org/apache/tez/tools/javadoc/doclet/ConfigStandardDoclet.java
Outdated
Show resolved
Hide resolved
tez-tools/tez-javadoc-tools/src/main/java-8-16/org/apache/tez/tools/javadoc/util/Writer.java
Outdated
Show resolved
Hide resolved
tez-tools/tez-javadoc-tools/src/main/java-8-16/org/apache/tez/tools/javadoc/util/XmlWriter.java
Outdated
Show resolved
Hide resolved
f6fafa4
to
43e76d1
Compare
thanks! addressed them in the latest commit |
💔 -1 overall
This message was automatically generated. |
wThis PR ensures about JDK 8-17 compile-time compatibility with JDK dependent source folders in tez-javadoc-tools module:
JDK 8-16:
JDK 17:
original code was contributed by @LA-Toth (which utilized different modules), now I'm trying to hit the PR until all the precommits problems disappear