-
Notifications
You must be signed in to change notification settings - Fork 628
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
Target java17 #5045
Target java17 #5045
Conversation
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
✅ Deploy Preview for nextflow-docs-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@bentsherman when you have some chance, it would be nice to fix the failing tests so we can move to Java 17 as target bytecode |
So this will drop support for Java 11? |
Yes, it may be needed to drop support for Java 11 since it's becoming more and more complicated to make coexisting different projects with Java 11 as baseline |
Not needed in the short term. Closing for now |
The factor connecting all of the failing tests is that they are mocking or spying
|
Signed-off-by: Ben Sherman <[email protected]>
modules/nextflow/src/main/groovy/nextflow/scm/RepositoryProvider.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
@bentsherman do you have a concrete need for Java 17? I guess there are still people running on Java 11 |
The main feature I'm using is this pattern matching syntax: if( node instanceof Expression exp ) {
// ...
} Whereas with Java 11 I would have to do: if( node instanceof Expression ) {
var exp = (Expression) node;
// ...
} I use this a lot in the parsing and AST code, so it does make the code more concise. But I can go back to the old way if lots of people still rely on Java 11. I figured it should be pretty easy to upgrade to Java 17 these days, I don't know how many people would actually be stuck with 11 |
Understand. Are you sure that this requires Java 17 bytecode generation and not Java 17 lang compatibility? |
I did try that: java {
sourceCompatibility = JavaVersion.VERSION_17
targetCompatibility = JavaVersion.VERSION_11
} But I was surprised to find it wasn't allowed:
|
But maybe there is some other way to do it that I don't know about? I would expect Java to still be able to generate Java 11 bytecode with such a simple change, but maybe there are other deeper changes that make it impossible |
Java 11 seems to be in a similar EOL status as Java 8, at least for Oracle Java: https://endoflife.date/oracle-jdk So I think it would be safe to require Java >=17. Users always have the previous versions of Nextflow Maybe it would be helpful to look up the compatibility matrices of other Java projects of similar size to see what they're doing |
I agree this is a nice to have. I'd like just postpone merging this to post 24.10.x to not fragment too much Java requirement for this year stable releases. |
Fine by me. I was actually going to suggest waiting until 25.04 . We can merge the new config parser then too. That will give me more time to stabilize some things |
|
Signed-off-by: Paolo Di Tommaso <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Moving forward this |
This set Java 17 as target bytecode generation and lang compatibility.
TLDR; so far mock created by Spock used cglib, which is not maintained anymore and does not support Java 17. Bytebuddy should replace it, but some tests fail.
https://spockframework.org/spock/docs/2.0-M4/faq.html