Skip to content
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 to Java 17 #2118

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Move to Java 17 #2118

merged 1 commit into from
Jun 7, 2022

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Jun 3, 2022

Fixes #2117

You can test the PR locally.
Jenkins fails because it uses Java 11

SendNotificationTest tests and AdvancedOrganizeImportsHandlerTest.testAmbiguousStaticImports require review.

Signed-off-by: Snjezana Peco snjezana.peco@redhat.com

@rgrunber
Copy link
Contributor

rgrunber commented Jun 3, 2022

Nice. I agree with most of this. What about the Bundle-RequiredExecutionEnvironment: JavaSE-11 statements in our manifests? Should we not change them to JavaSE-17 given we can't really run on a lesser version after this ?

@snjeza
Copy link
Contributor Author

snjeza commented Jun 3, 2022

Should we not change them to JavaSE-17 given we can't really run on a lesser version after this ?

I have changed it.

@rgrunber
Copy link
Contributor

rgrunber commented Jun 3, 2022

There's a few is missing the Java nature failures.

mvn-build-of-pr.txt

@snjeza
Copy link
Contributor Author

snjeza commented Jun 3, 2022

There's a few is missing the Java nature failures.

Checking...
I have updated gradle to the 7.3 version, tycho to 2.7.2, mockito to 4.x. Please check it again.
gradle supports Java 17 from the 7.3 version.
SendNotificationTest tests fail with java 17. I will check them.

@snjeza snjeza force-pushed the issue-2117 branch 4 times, most recently from c0b6cdb to 14697b8 Compare June 6, 2022 00:25
@rgrunber
Copy link
Contributor

rgrunber commented Jun 6, 2022

Most of the errors seem to be resolve and I was able to run once with everything passing. However, on some occasions I see the following failures/errors :

ProjectUtilsTest.testGetMaxProjectProblemSeverity:51 expected:<0> but was:<1>

GradleProjectImporterTest.importGradleKtsProject:509->AbstractProjectsManagerBasedTest.assertNoErrors:309 kradle has errors:
Type=org.eclipse.jdt.core.problem:Message=The project was not built since its build path is incomplete. Cannot find the class file for java.lang.Object. Fix the build path then try building this project:LineNumber=null, Type=org.eclipse.jdt.core.problem:Message=The type java.lang.Object cannot be resolved. It is indirectly referenced from required .class files:LineNumber=1 expected:<0> but was:<2>
  MavenProjectImporterTest.testJava17Project:313->AbstractProjectsManagerBasedTest.assertHasErrors:299 Preview features enabled at an invalid source release level was not found in:
Type=org.eclipse.jdt.core.problem:Message=The project was not built due to "Failed to init ct.sym for /home/rgrunber/git/eclipse.jdt.ls/org.eclipse.jdt.ls.tests/fakejdk2/17a/lib/jrt-fs.jar". Fix the problem, then try refreshing this project and building it since it may be inconsistent:LineNumber=null

org.eclipse.jdt.ls.core.internal.handlers.DocumentLifeCycleHandlerTest.testReconcile(org.eclipse.jdt.ls.core.internal.handlers.DocumentLifeCycleHandlerTest)
  Run 1: DocumentLifeCycleHandlerTest.setup:101->mockPreferences:198 WrongTypeOfReturnValue
  Run 2: DocumentLifeCycleHandlerTest.tearDown:114 NullPointer Cannot invoke "org.eclip...

Import-Package: org.osgi.framework;version="1.3.0"
Bundle-Localization: plugin
Bundle-ActivationPolicy: lazy
Import-Package: org.osgi.framework;version="1.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 10 duplicates with Line 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@testforstephen
Copy link
Contributor

We also need to update the minimum JDK requirement in client side in case someone is using java.jdt.ls.java.home to specify tooling JDK.

https://github.com/redhat-developer/vscode-java/blob/6ec891f313d2ee30f754c80914e9a7e618b942d9/src/requirements.ts#L12

@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

Most of the errors seem to be resolve and I was able to run once with everything passing. However, on some occasions I see the following failures/errors :

@rgrunber I can reproduce it. Working on that.

@snjeza snjeza force-pushed the issue-2117 branch 2 times, most recently from f97575b to ed9b391 Compare June 6, 2022 20:14
@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

test this please

1 similar comment
@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

test this please

@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

We also need to update the minimum JDK requirement in client side in case someone is using java.jdt.ls.java.home to specify tooling JDK.

@testforstephen I have created redhat-developer/vscode-java#2495

@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

Most of the errors seem to be resolve and I was able to run once with everything passing. However, on some occasions I see the following failures/errors :

@rgrunber I have updated the PR. Could you, please, test it again? I have also updated Jenkins job to JDK 17.

@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

test this please

@rgrunber
Copy link
Contributor

rgrunber commented Jun 6, 2022

Similar to https://ci.eclipse.org/ls/job/jdt-ls-pr/3322/testReport/junit/org.eclipse.jdt.ls.core.internal.handlers/CompletionHandlerTest/testCompletion_AnonymousTypeMoreMethods/ , I saw :

Failures: 
  CompletionHandlerTest.testCompletion_AnonymousType:2147 expected:<IFoo() {
[	${0}]
};> but was:<IFoo() {
[]
};>

locally. Everything else is passing.

@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

Similar to https://ci.eclipse.org/ls/job/jdt-ls-pr/3322/testReport/junit/org.eclipse.jdt.ls.core.internal.handlers/CompletionHandlerTest/testCompletion_AnonymousTypeMoreMethods/ , I saw :

CompletionHandlerTest fails randomly

I will check it.

You can try to add -Dcompletion.timeout=30000 (30 seconds) when starting the Java LS tests. I have added it to Jenkins job

@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

test this please

@rgrunber rgrunber changed the title [WIP] Move to Java 17 Move to Java 17 Jun 6, 2022
@rgrunber
Copy link
Contributor

rgrunber commented Jun 6, 2022

I think this is in a good state now, and ready to be merged. I would just ask for @testforstephen to confirm.

@snjeza , I would just add the following to the message body (below the title) :

- Update to Tycho 2.7.2
- Update to jacoco-maven-plugin 0.8.7
- Update to org.mockito.mockito-core 4.x
- Update BREE from JavaSE-11 to JavaSE-17
- Use Gradle 7.3 for test projects to ensure compatibility with Java 17

We may want to later revisit if it's possible to specify a different JDK than the one used to launch Maven (or Tycho Surefire) for the Gradle daemon used for a particular test. This way we could optionally continue to test older versions of Gradle. Could we have just used the "gradle java home" property through preferences ?

For now I think we should get this change in to get the builds working again.

- Update to Tycho 2.7.2
- Update to jacoco-maven-plugin 0.8.7
- Update to org.mockito.mockito-core 4.x
- Update BREE from JavaSE-11 to JavaSE-17
- Use Gradle 7.3 for test projects to ensure compatibility with Java 17

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@snjeza
Copy link
Contributor Author

snjeza commented Jun 6, 2022

... I would just add the following to the message body (below the title) :

@rgrunber I have added it

@snjeza snjeza removed the in progress label Jun 6, 2022
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I think we should merge. Any additional modifications we can make once the build is back to normal.

@snjeza snjeza merged commit 6d63361 into eclipse-jdtls:master Jun 7, 2022
@snjeza
Copy link
Contributor Author

snjeza commented Jun 7, 2022

...Any additional modifications we can make once the build is back to normal.

@rgrunber I have updated the jdt-ls-master build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to Java 17
3 participants