-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add JDK Api extractor tool to help with entitlement reviews #132920
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
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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.
A couple questions, but this looks pretty good!
| "jdk.javadoc", | ||
| // "jdk.jpackage", // Do we want to include this? | ||
| // "jdk.jlink", // Do we want to include this? | ||
| "jdk.localedata" // noise, change here are not interesting |
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.
maybe not interesting for entitlements, though note that we see test failures often because of localedata changes, so maybe having something similar as a comparison is useful for jdk upgrades?
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.
interesting point, though not sure how actionable the changes are. but worth investigating. I'll leave this as a point in the future automation jira
| mainClass.set(javaMainClass) | ||
| applicationDefaultJvmArgs = [ | ||
| '--add-exports', 'java.base/sun.security.util=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.lang=ALL-UNNAMED', |
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.
What about potentially new packages we don't know about? Could we scan the jdk itself for packages (eg at the filesystem level by opening up the archives that contain modules)?
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.
If the JDK on which you are running the tool have new packages, it will try to access them and it will break (so you'll need to come back here and add them to the list).
What you are suggesting is definitely possible though; packages will just be directories inside the modules we are scanning.
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.
Right 👍 I'll note it as future improvement in the automatic task 👍
...-api-extractor/src/main/java/org/elasticsearch/entitlement/tools/jdkapi/JdkApiExtractor.java
Outdated
Show resolved
Hide resolved
...-api-extractor/src/main/java/org/elasticsearch/entitlement/tools/jdkapi/JdkApiExtractor.java
Show resolved
Hide resolved
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.
Looks good! It's almost exactly what I had in mind when we discussed it
| mainClass.set(javaMainClass) | ||
| applicationDefaultJvmArgs = [ | ||
| '--add-exports', 'java.base/sun.security.util=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.lang=ALL-UNNAMED', |
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.
If the JDK on which you are running the tool have new packages, it will try to access them and it will break (so you'll need to come back here and add them to the list).
What you are suggesting is definitely possible though; packages will just be directories inside the modules we are scanning.
...-api-extractor/src/main/java/org/elasticsearch/entitlement/tools/jdkapi/JdkApiExtractor.java
Outdated
Show resolved
Hide resolved
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.
LGTM
This tool scans the JDK on which it is running to extract its public accessible API.
That is:
The output of this tool is meant to be diffed against the output for another JDK
version to identify changes that need to be reviewed for entitlements.
Usage example:
To review the diff of deprecations, use
--deprecations-onlyas 2nd argument../gradlew :libs:entitlement:tools:jdk-api-extractor:run -Druntime.java=24 --args="deprecations-jdk24.tsv --deprecations-only"Note: This tool considers more methods than the public-callers-finder as it also returns methods of internal, invisible, or non-exported classes that overwrite / implement a publicly visible method via an exported public class or interface.
We might want to consider expanding
public-callers-finderthe same way.Relates to ES-12408, ES-12407