Build on latest Java, test through lowest possible - approach 2 #298
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
szpak
commented
Jan 16, 2024
| @@ -38,4 +39,5 @@ jobs: | |||
| - name: Test cross Java versions compatibility | |||
| run: | | |||
| ./gradlew --version | |||
| ./gradlew --stacktrace build compatTestJava${{ matrix.jdk }} | |||
| ./gradlew -q javaToolchains | |||
| ./gradlew --stacktrace -PnexusPublishPlugin.test.java=${{ matrix.testJdk }} build compatTestJava${{ matrix.jdk }} | |||
There was a problem hiding this comment.
To avoid "javaauto" in stutter, we could add testJdk matrix elements manually, adjusting "auto" version to a given JDK version.
Also a shell command could replace "auto" with a proper version.
However, I don't know which of this 3 options is best :-/
szpak
commented
Jan 16, 2024
| jdk: [11, 17] | ||
| name: "OpenJDK ${{ matrix.jdk }}" | ||
| jdk: [11, 17, 19] | ||
| testJdk: [auto, 8] |
There was a problem hiding this comment.
I wonder if we shouldn't have "fail-fast" disabled for that workflow?
3flex
reviewed
Jan 20, 2024
| @@ -261,6 +288,19 @@ tasks { | |||
| withType<Test>().matching { it.name.startsWith("compatTest") }.configureEach { | |||
| systemProperty("plugin.version", project.version) | |||
| } | |||
| named<Test>("test").configure { | |||
There was a problem hiding this comment.
This is equivalent:
Suggested change
| named<Test>("test").configure { | |
| test { |
There was a problem hiding this comment.
Is that tasks.test {} or project.test {} just to make sure it's at the right level in the build.gradle.
3flex
reviewed
Jan 20, 2024
| @@ -174,7 +190,7 @@ kotlin.target.compilations.configureEach { | |||
| compilerOptions.configure { | |||
| // Gradle fully supports running on Java 8: https://docs.gradle.org/current/userguide/compatibility.html, | |||
| // so we should allow users to do that too. | |||
| jvmTarget = JvmTarget.fromTarget(JavaVersion.VERSION_1_8.toString()) | |||
| jvmTarget = JvmTarget.fromTarget(project.java.targetCompatibility.toString()) | |||
There was a problem hiding this comment.
Consider merging #300 then rebasing.
|
I'm yet to do a full review, thanks for looking @3flex ! |
Merged
1 task
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.
This is a reworked variant of #254 (I really liked the approach with overriding javaLauncher for test @TWiStErRob! - originally I wanted to publish binaries locally and pass it to another jobs, which javaLauncher is much easier) with the following changes:
This is not as reliable as separate e2eTests executed with lower JDK, but the binaries are built with higher JDK and running tests leverages that binaries.
Downsides:
compatTestJavaautoadded)Maybe you will have an idea how to improve it further.