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

Make Gretty aware of Gradle Java Toolchain (v3.x) #306

Merged
merged 21 commits into from
Jun 3, 2024

Conversation

mr-serjey
Copy link

@mr-serjey mr-serjey commented May 13, 2024

Gretty plugin run containers as a separate java process using the same java that is used by Gradle.

However since Gradle introduced Toolchains for JVM projects, the expectation is that Gretty plugin will also respect the java toolchain DSL and run containers using the specified JDK instead of one that used by Gradle.

Currently if java toolchain DSL is declared in a project, the classes compiled by the JDK specified there (e.g. v21), at the same time Gretty plugin uses Gradle JDK (e.g. v11), as result the container fails to start since JVM v11 can't load classes of java v21.

This PR purposes changes into Gretty plugin to respect java toolchain DSL:

  • add ServerConfig.jvmExecutable String (optional value)
  • StartBaseTask sets ServerConfig.jvmExecutable (if java toolchain DSL specified)
  • DefaultLauncher sets JavaExecSpec.executable for Gradles javaexec to start the right java version (if jvmExecutable set)
  • If no toolchain DSL specified the default JavaExecParams.executable is used (Gradle JVM)

With the following DSL the project going to use java 21 both to compile classes and to run them in container using Gretty plugin:

build.gradle
java {
    toolchain {
        languageVersion = JavaLanguageVersion.of(21)
    }
}

Related issue: #261

Complementary changed of tests:

  • added tests for Gradle Java Toolchain functionality
  • added extra matrix cases in ci.yml to cover Gradle Java Toolchain functionality (Java 11+17 and java 17+21)
  • added docker_integration_tests.sh that runs all tests inside docker similarly to ci.yml (helped a lot locally on Windows machine)
  • migrated groovy classes of test applications into java, to avoid groovy bytecode incompatibility on high java versions

@boris-petrov boris-petrov requested a review from f4lco May 14, 2024 03:15
@boris-petrov
Copy link
Member

@mr-serjey awesome, thanks for that PR, that's definitely an important addition. Tests are failing though, can you take a look? Also, do you think there is a way to add a test for this? I wouldn't think so but if you have any ideas, that would be great.

@mr-serjey
Copy link
Author

@boris-petrov Thanks, I will take care of failing tests. Please hint how to run all these tests locally?

@boris-petrov
Copy link
Member

boris-petrov commented May 14, 2024

Make sure you're using JDK 11+ (actually it's best to use 11, not sure if it will work correctly with later versions) and run:

./gradlew clean build
cd integrationTests
../gradlew -PgeckoDriverPlatform=linux64 testAll

We should document that somewhere...

@mr-serjey
Copy link
Author

@boris-petrov, @f4lco I've finished with my changes (see updated first comment for more details).
Please let me know if anything needs to be improved.

Once this PR merged (and released), I'm planning to prepare another PR with these changes for the version 4 (master).

Please tell what is ETA for this PR to be released?

@boris-petrov
Copy link
Member

@mr-serjey thank you and well done on the great work here! I'll review it in the next few days and hopefully @f4lco too. After that we'll merge and release - it won't take long. :)

@boris-petrov
Copy link
Member

Awesome job @mr-serjey!

Just a couple of minor notes:

  1. the way enableOnlyJavaToolchainAwareProjects is implemented is rather strange. It's going through all tasks in a project and disabling them one by one - isn't there a better Gradle-style way to "remove" the whole project? @f4lco any ideas about that?

  2. This comment tripped me in the beginning:
    The caller project integration test would react on -PtoolchainJavaVersion=17 parameter and define appropriate toolchain DSL
    I initially thought that the caller project is supposed to define something which I wasn't seeing in the three old projects that were marked as toolchain-aware. Perhaps a slight rephrasing would be good.

  3. Speaking of the three integration test projects - I guess you chose these at random - no particular reason for these specific ones?

  4. What are the groovy jvm version limitations/groovy bytecode incompatibility on high java versions that you mention? For example using an older Groovy version when a JDK 21 is used as a toolchain?

  5. The README in the gradle-java-toolchain is copied from spring-boot-simple and must be modified (or removed).

  6. I think I understand why you've forced some of the tasks to use the running Gradle's Java version - but shouldn't at least the compilation tasks of the integration tests run with the specified toolchain? I'm talking about the forceSourceSetToUseGradleJvm call.

Otherwise, again, great work!

mr-serjey added 2 commits May 27, 2024 16:14
…lchains task depend only on those tasks/projects that support java toolchain.
@mr-serjey
Copy link
Author

Thanks for such a great review @boris-petrov , all the items make total sense and I did my best to address them:

  1. As for enableOnlyJavaToolchainAwareProjects, I re-implemented the logic, so additionally to testAll task that depends on all integrationTest tasks of all projects, now we have testAllJavaToolchain task that depends on all integrationTest tasks of thouse projects that call enableOnlyJavaToolchainAwareProjects. (basically we have two test entry points now)

  2. Comment re-written, please let me know if it needs more attension.

  3. I picked tests with simple names and with the logic I can easy understand, but the range of tests can be extended if needed. I tried to add all projects initially, but faced with libraries to jvm compatibility issues, such as spring-boot 2.x doesn't work with java 21, and some other issues. So I excluded the projects from tests, since It is up to user of gretty to figure out third party dependencies.

  4. Test projects are based (or was based) on few libraries that are not compatible with Java 21. e.g. groovy, spring-boot. I mitigated groovy issue by converting all app controllers into java classes. Spring boot applications was excluded from tests of java toolchains.

  5. Updated the README in the gradle-java-toolchain.

  6. I had to force tests to use gradle's jvm since they are based on groovy, so that they must be compiled and executed with compatible Jvm. At the same time the applications that are started in background use toolchain java version (when specified), and have no such limit. I think current situation is ok, when we have tests of this project running on older setup then the actual plugin and container that we test.

Thanks for the feedback, please let me know if anything else needed.

@boris-petrov
Copy link
Member

@mr-serjey thanks for the quick fixes!

1, 2, 5. Seems much better now, thanks!

  1. I think these three projects you've chosen are fine, no need to run the others too.

  2. What confused me was the fact that you've migrated from Groovy to Java test files in projects that are after that skipped/not-marked as toolchain projects. So I was wondering what was the problem with them. But it doesn't matter that much.

  3. Yes, let's leave it as it is.

Great work again! The tests seem to have failed, do you mind taking a look?

Also, before merging, I would like @f4lco to take a look too as he's the smart one here. 😄 Hopefully he can do it soon. :)

@mr-serjey
Copy link
Author

  1. What confused me was the fact that you've migrated from Groovy to Java test files in projects that are after that skipped/not-marked as toolchain projects. So I was wondering what was the problem with them. But it doesn't matter that much.

@boris-petrov Now I guess I understand you more... I migrated all application classes because initially (on local) toolchain tests was running against all projects. Now the tests are running against only 3 projects, so this groovy->java migration is needed only for them. My personal preference is to leave all application classes in java to have same approach for all of them and to distantiate from groovy/java versions alignment. But I'm ok to revert n-3 apps back to groovy if this makes more sense.

PS: I'm still fixing the tests and let you guys know when I think I'm done.

@mr-serjey
Copy link
Author

@boris-petrov, @f4lco all review items has been addressed. Please let me know if anything else need to be changed.

@boris-petrov
Copy link
Member

But I'm ok to revert n-3 apps back to groovy if this makes more sense.

No, let's leave it as it is now.

I'm fine with the PR, great work! Let's give @f4lco a few days - I'll merge it after that if he doesn't answer. We can always make improvements after that. :)

@boris-petrov
Copy link
Member

OK, I'll merge this now.

@mr-serjey thanks again for the great work! I'll be releasing this in the next few days. If you can and want, maybe you can port the same PR for the master branch (Gretty 4).

@f4lco whenever you can, please take a look at this code and write here or anywhere your comments/suggestions. :)

@boris-petrov boris-petrov merged commit 5557fbf into gretty-gradle-plugin:gretty-3.x Jun 3, 2024
7 checks passed
@mr-serjey
Copy link
Author

Thanks @boris-petrov, I will prepare master PR in a while.

@boris-petrov
Copy link
Member

@mr-serjey I've released 3.1.4. Thanks again for the great work!

@mr-serjey mr-serjey changed the title Make Gretty aware of Gradle Java Toolchain Make Gretty aware of Gradle Java Toolchain (v3.x) Jun 21, 2024
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.

2 participants