-
Notifications
You must be signed in to change notification settings - Fork 121
Use Multi-Release Type for Caller implementation #2138
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
Test Results 771 files 771 suites 58m 9s ⏱️ Results for commit ef3dd53. ♻️ This comment has been updated with latest results. |
The current implementation for Java 1.8 is rather complex because of only raw string info. With Java 9+ we can use StackWalker to perform faster and easier, to still retain compatibility with Java 1.8 we use a Multi-Release type here to use that implementation on any 9+ JVM.
6087eb5 to
ef3dd53
Compare
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 and I think this is fine for RC1.
Approved from PL.
Still there are a few minor part that should be unified with the Java-1.8 implementation (but that's mainly in the old one) and they can be done later, before RC2 or in the next cycle.
I also tried to test this change with a secondary Eclipse, launch from my PDE workspace and although there is a
org.eclipse.pde.junit.runtime\bin\META-INF\versions\9\org\eclipse\pde\internal\junit\runtime\Caller.class file, when I launch a junit-5/6 test from that secondary workspace only the java-1.8 impl is loaded in the test runtime.
I assume that's not an issue with JDT but with Equinox? Or is that somehow expected?
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <classpath> | ||
| <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.8"/> | ||
| <classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-9"> |
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.
Shouldn't this stay at 1.8 or does it have to be the highest version of all contained source folders?
I assume the general jdt.core settings (with release option enabled) ensure that the main src folder isn't compiling against Java-9?
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.
Yes the JVM is always the highest (as not more the one is supported).
The source/target controls for what byte-code version is used and the release control if the compiler checks what JVM classes can be used (I'm sure @stephan-herrmann can better explain this) so we still get what we want.
Yes I need to improve that part, the main point is that the specification is about Multi-Release JAR (!) and not Multi-Release Folders... so PDE needs to be improved (or maybe we can even do it at equinox) as we are using some special tricks here to inject dev-entries to the bundle. |
Indeed!
I'm fine with either way, but I assume Equinox would be cover more use-cases as otherwise Mutli-Release 'JAR's wouldn't work for bundles with shape |
The current implementation for Java 1.8 is rather complex because of only raw string info.
With Java 9+ we can use StackWalker to perform faster and easier, to still retain compatibility with Java 1.8 we use a Multi-Release type here to use that implementation on any 9+ JVM.
See