feat: Gradle toolchains support #19789
Open
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
|
|
|
@mr-serjey thank you for your contribution! |
|
@mvysny asked a review from you as well, as you are our great expert in Gradle integration and may be interested :) |
|
Hi guys, @mvysny, @mshabarov any news on this PR or issue? |
|
I'm looking into it. The very first impression is that the Worker API and Toolchain support could be separated from each other for easier review, but not a blocker IMO. |
mshabarov
requested changes
Sep 9, 2024
| ) | ||
| } | ||
|
|
||
| public companion object |
There was a problem hiding this comment.
Suggested change
| public companion object | |
| companion object |
| ) | ||
| } | ||
|
|
||
| internal fun VaadinTaskConfigurationFactory.Companion.from( |
There was a problem hiding this comment.
Perhaps this is a well-known pattern for Kotlin and I'm lacking Kotlin experience, but does it make sense to move these factory methods to factory classes instead? E.g. move this from implementation to the companion object in VaadinTaskConfigurationFactory.
| * | ||
| * @return `true` to remove created files, `false` to keep the files | ||
| */ | ||
| private fun cleanFrontendFiles(adapter: PluginAdapterBuild, cleanFrontendFiles: Boolean): Boolean { |
There was a problem hiding this comment.
This method became private, should be kept protected to follow the method's contract and javadoc.
|
@mshabarov Please let me know when you finish the review, so I can apply changes you recommend. |
mshabarov
requested changes
Sep 11, 2024
|
|
||
| import java.io.Serializable | ||
|
|
||
| public data class Flags( |
There was a problem hiding this comment.
From where these flags were taken? From GradlePluginAdapter ?
There was a problem hiding this comment.
"BooleanProperties" would be a better name.
Comment on lines
+65
to
+71
| override fun logDebug(p0: CharSequence?): Unit = loggerAdapter.logDebug(p0) | ||
| override fun logDebug(p0: CharSequence?, p1: Throwable?): Unit = loggerAdapter.logDebug(p0, p1) | ||
| override fun logInfo(p0: CharSequence?): Unit = loggerAdapter.logInfo(p0) | ||
| override fun logWarn(p0: CharSequence?): Unit = loggerAdapter.logWarn(p0) | ||
| override fun logWarn(p0: CharSequence?, p1: Throwable?): Unit = loggerAdapter.logWarn(p0, p1) | ||
| override fun logError(p0: CharSequence?): Unit = loggerAdapter.logError(p0) | ||
| override fun logError(p0: CharSequence?, p1: Throwable?): Unit = loggerAdapter.logError(p0, p1) |
There was a problem hiding this comment.
Method parameters should be human-readable.
| override fun logError(p0: CharSequence?): Unit = loggerAdapter.logError(p0) | ||
| override fun logError(p0: CharSequence?, p1: Throwable?): Unit = loggerAdapter.logError(p0, p1) | ||
|
|
||
| public companion object |
There was a problem hiding this comment.
Suggested change
| public companion object | |
| companion object |
Comment on lines
+6
to
+40
| fun logDebug(p0: CharSequence?) { | ||
| logger.debug(asMessage(p0)) | ||
| } | ||
|
|
||
| fun logDebug(p0: CharSequence?, p1: Throwable?) { | ||
| logger.debug(asMessage(p0), asThrowable(p1)) | ||
| } | ||
|
|
||
| fun logInfo(p0: CharSequence?) { | ||
| logger.info(asMessage(p0)) | ||
| } | ||
|
|
||
| fun logWarn(p0: CharSequence?) { | ||
| logger.warn(asMessage(p0)) | ||
| } | ||
|
|
||
| fun logWarn(p0: CharSequence?, p1: Throwable?) { | ||
| logger.warn(asMessage(p0), asThrowable(p1)) | ||
| } | ||
|
|
||
| fun logError(p0: CharSequence?) { | ||
| logger.error(asMessage(p0)) | ||
| } | ||
|
|
||
| fun logError(p0: CharSequence?, p1: Throwable?) { | ||
| logger.error(asMessage(p0), asThrowable(p1)) | ||
| } | ||
|
|
||
| private fun asMessage(p0: CharSequence?): String { | ||
| return p0!!.toString() | ||
| } | ||
|
|
||
| private fun asThrowable(p1: Throwable?): Throwable { | ||
| return p1!! | ||
| } |
There was a problem hiding this comment.
These variables need a human-readable names
| import org.gradle.api.Action | ||
| import org.gradle.workers.ProcessWorkerSpec | ||
|
|
||
| internal interface ProcessWorkerSpecConfigurer : Action<ProcessWorkerSpec> { |
There was a problem hiding this comment.
What is this for? Needs docs to explain it's goals.
|
|
||
| import java.io.Serializable | ||
|
|
||
| public data class Strings( |
There was a problem hiding this comment.
"StringProperties" would be a better name.
| import java.io.Serializable | ||
| import java.net.URI | ||
|
|
||
| public data class Locations( |
There was a problem hiding this comment.
"LocationProperties" would be a better name.
| import org.gradle.api.provider.Property | ||
| import org.gradle.workers.WorkParameters | ||
|
|
||
| public interface VaadinWorkActionParameter : WorkParameters { |
There was a problem hiding this comment.
This needs javadocs, at least about goal of this class.
|
Two general questions:
|
mshabarov
reviewed
Sep 11, 2024
flow-plugins/flow-gradle-plugin/src/main/kotlin/com/vaadin/gradle/VaadinBuildFrontendTask.kt
Show resolved
Hide resolved
|
Drafted a very quick diagram that perhaps would be helpful for further review: 
|
I don't think that this class is used by Worker API. It's rather an another implementation of |
|
❎ Gradle functional tests are failing: with rawly the same exception: |
|
And here is the full stack trace: |
|
I wonder if it's something related to build environment on CI server, or something missing in the source code. |
|
One more observation:
which I don't understand at the moment. |
|
@mr-serjey these are all basically that I have as comments. Overall, it looks very well and promising, excellent contribution! 🥇 Could you please take a look at my comments when appropriate? Also, could you try to reproduce these failures in This feature I'd like to target to Vaadin 24.6, as 24.5 is approaching beta closely. Let me know you opinion and if me or someone from Vaadin Flow can you help you with moving forward with this contribution. Thanks! |
|
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.
Description
Add support of Gradle JVM toolchains into flow-gradle-plugin.
Changes:
GradleWorkerApiAdapterthat used inside Gradle Worker API Action that is running in an isolated JVM (e.g. Java 21 configured by project toolchains, while gradle is running on Java 17 from JAVA_HOME).VaadinTaskConfigurationthat constructed in task configuration, but used by task action in isolated JVM viaGradleWorkerApiAdapter.JavaExecutionServicethat executes task actions in an isolated JVM using project's JVM toolchain configurationVaadinSmokeTestandMiscMultiModuleTestto run the same tests against projects that configured with jvm toolchains.Motivation is to address associated issue and enable better support of standard Gradle functionality, and so to enable better flexibility of using newer java versions.
Fixes issue "Vaadin-gradle-plugin does not support Gradle Toolchains" #19338
Type of change
Checklist