Fix Gradle tests started with Java 23 #3238
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
snjeza
requested review from
fbricon,
mickaelistria,
rgrunber,
jdneo and
testforstephen
|
Excluding the incorrect/test VMs to start Gradle seems like a good idea. |
|
However, I confirm that with this patch, if I have Java 17 available and detected by JDT (which is not so sure to happen on CI though), I can see the GradleProjectMetadataFileTest succeeding while it was previously failing because the test VM was used. |
snjeza
changed the title
|
test this please |
I have fixed it. |
snjeza
changed the title
|
test this please. |
|
@mickaelistria @rgrunber I have updated the PR. Could you, please, review it? |
|
@snjeza , it would be good to have an explanation of why this filtering needs to happen on the javac branch. |
The VM filtering needs to happen in any case. The javac branch has revealed the issue because it's using Java 23 which is incompatible with Gradle, so "fakejdk"s are resolved instead to run Gradle, which is a bug in general. The "regular" CI build seems to use Java 17 and this Java 17 is used to run Gradle because it's compatible. |
|
This has been consumed by ls-with-javac build, but Gradle-based tests are now failing with in log. and All that is visible in the logs of https://ci.eclipse.org/ls/job/jdt-ls-javac/345/ . I think the main issue now is that JDT isn't instructed to find other Java versions available on CI. |
We must upgrade the gradle wrapper from the 7.3.3 version to the 8.10-rc-1 version.
|
|
I see the toolchains defines all the JVM versions, and Maven is supposed to detect them IIRC. I think instead of changing the test projects, we should investigate why the VMs are not detected. |
|
We may just call |
|
There are a couple of lines in the tests that actually prevent from loading known JDT: IEclipsePreferences prefs = InstanceScope.INSTANCE.getNode(MavenJdtPlugin.PLUGIN_ID);
prefs.put(PREFERENCE_LOOKUP_JVM_IN_TOOLCHAINS, Boolean.FALSE.toString());IMO, those like should be removed if we want to leverage capability to test easily against different JDT & Gradle versions |
|
@mickaelistria I have created #3240 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The related issue - eclipse-jdtls/eclipse-jdt-core-incubator#609