-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Removing warning for forbidden apis not supporting java 14 #55359
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
While the current version of forbidden apis still does not support java 14, the warning message has become very noisy as we now require java 14 for the elasticsearch build. This commit replaces the warn log message with a comment in the code.
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
"Forbidden APIs does not support Java versions past 13. Will use the signatures from 13 for {}.", | ||
BuildParams.runtimeJavaVersion | ||
) | ||
// forbidden apis does not yet support java 14 (it will in version 3.0), so we must use java 13 target |
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.
Now that I look at this, why are we basing this on runtime java version? This should simply be hard-coded to be minimumRuntimeVersion
since that's what we pass to java -release
so it's effectively impossible to use APIs from a newer JDK anyhow as it'd fail at compile time. The fact that we might be configuring the build with a later runtime JVM is irrelevant. That only affects runtime behavior (i.e. tests) nothing about how our code is compiled and which JDK APIs are available to it.
We'll need to of course deal with the fallout of forbiddenapi compatibility when we bump the minimum runtime version but I don't see that happening anytime soon.
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.
That's certainly something we should look at, but will likely require more investigation (I recall we tried something like this before and ran into issues, likely with multi release jars). I'm happy to do that as a followup, but this PR still will reduce unnecessary noise that is plaguing all local builds now that we require JDK 14 to run the build.
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.
Yeah, the difficulty was exactly multi-release JARs.
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.
Cool. I'm good with deferring that for now.
While the current version of forbidden apis still does not support java 14, the warning message has become very noisy as we now require java 14 for the elasticsearch build. This commit replaces the warn log message with a comment in the code.
While the current version of forbidden apis still does not support java 14, the warning message has become very noisy as we now require java 14 for the elasticsearch build. This commit replaces the warn log message with a comment in the code.
While the current version of forbidden apis still does not support java
14, the warning message has become very noisy as we now require java 14
for the elasticsearch build. This commit replaces the warn log message
with a comment in the code.