-
Notifications
You must be signed in to change notification settings - Fork 67
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
Move Equinox Launcher to Java-17 #829
base: master
Are you sure you want to change the base?
Conversation
This pull request changes some projects for the first time in this development cycle.
An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patch
Further information are available in Common Build Issues - Missing version increments. |
Thanks for this work @HannesWell What is the reason not to jump directly to Java 21? I think @tjwatson indicated that that would also be fine |
I can see that
Large parts of Eclipse/Equinox can still run on Java 17, so what would be the immediate benefits here to break those? |
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.
I would wait until next release for this change, at least warnings/deprecation should be fixed and not suppressed.
@@ -2609,6 +2592,7 @@ private void setMultiValueProperty(String property, String[] values) { | |||
* nobody will actually call them (unless they casted the policy to EclipsePolicy and | |||
* called our methods) | |||
*/ | |||
@SuppressWarnings({"removal", "deprecation"}) |
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 we have new warnings due to updated java version it should not be suppressed but fixed along side with the java update.
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 new warning is because the Policy
class is deprecated along with the SecurityManager
. I'd more than happy to just remove it, but I assume that's not yet desired, is it?
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.
🙈
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.
I do think there are scenarios (ant?) that still optionally set a security manager when using Java 21 so I don't think we can really remove this policy stuff just yet.
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.
Ant isn't such scenario as it just sets the security-manager if the running JVM is blow 18:
But there could be other scenarios that require it. So I assume that part should stay as long as the security-manager code is removed.
Btw. is there a plan for how to deal with that? Or is the plan to keep it as long as it works/compiles on all released Java versions?
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.
Btw. is there a plan for how to deal with that? Or is the plan to keep it as long as it works/compiles on all released Java versions?
It depends on the component. For the most part, there should be little references to the actual SecurityManager
or Policy
in our code itself. The Framework
itself has many places it is referenced, but they should be able to be contained into an internal utility that fails gracefully on a future JVM where the actual classes are removed.
The biggest issue is code all over the place that have references to java.security.AccessController.doPrivileged(PrivilegedAction<String>, AccessControlContext)
and hundreds of implementations of java.security.PrivilegedAction<T>
or java.security.PrivilegedExceptionAction<T>
My suspicion is that these APIs will be around much longer than the SecurityManager
and Policy
classes, but will become no-op methods when calling doPrivileged
. I don't see them being able to truly remove the doPrivileged
APIs until all the long term support of Java 8 finally end.
I'm fine postponing the entire change to the next dev-cycle, I don't think it's urgent. Maybe I'll add some follow-up changes in separate commits in the meantime, but they can later be moved to separate PRs as well. |
Move Equinox Launcher to Java-17 as suggested in #782 (comment).
And leverage the new language features to modernize the code a bit.