Properly pass Kotlin compiler plugin options to dev-mode #6030
Conversation
|
I tested this and it worked great on a generated project. I also updated the Kotlin integration test to reflect the test case it's supposed to solve. Feedback more than welcome :) |
geoand
force-pushed
the
kotlin-devmode
branch
from
6d27b30 to
18431dd
Compare
|
@ia3andy this doesn't fix your Java 11 issue with Kotlin, but it should make it a lot easier for you or anyone else to implement it. Did you ever open an issue for that? |
geoand
force-pushed
the
kotlin-devmode
branch
from
18431dd to
fa20464
Compare
geoand
force-pushed
the
kotlin-devmode
branch
from
fa20464 to
53414da
Compare
|
@gsmet I updated per your suggestion and added an answer to your question |
| <groupId>org.jetbrains.kotlin</groupId> | ||
| <artifactId>kotlin-compiler</artifactId> | ||
| <version>${kotlin.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
Do we still need kotlin-compiler-embeddable? AFAICS, it's used only in kotlin/deployment/pom.xml. Which also raises a question why it is in the runtime BOM instead of the deployment one.
There was a problem hiding this comment.
Nice catches, thanks!
I think we can remove embeddable. I'll also check and see if moving it to the deployment BOM works.
There was a problem hiding this comment.
Actually, are you sure it should in the deployment BOM? I don't see any other non-quarkus artifacts there
There was a problem hiding this comment.
I removed the embeddable dependency
There was a problem hiding this comment.
Actually, are you sure it should in the deployment BOM? I don't see any other non-quarkus artifacts there
The runtime BOM is for runtime dependencies. It is also imported directly by user applications to align application runtime dependencies with the Quarkus platform's runtime dependencies.
Theoretically, if the artifact is only a deployment dependency then it belongs to the deployment BOM. One caveat in the current implementation (and afaict in the to be implemented classloading model) if there are difference versions of the artifact in the runtime CP and the deployment CP then the runtime one will end up being used during the augmentation.
In this given case, however, I'd probably put it in the deployment BOM. If you are not comfortable doing that then don't :) Could be a separate issue.
The BOMs should probably go through a thorough review.
There was a problem hiding this comment.
Ah, the properties. I think the properties should move to the root pom.xml so that they can be shared across all the subprojects including the tests. Some tests define/duplicate those versions, which is not a good thing.
There was a problem hiding this comment.
@aloubyansky makes sense!
@gsmet I think that what @aloubyansky proposes should be handled in a dedicated issue, WDYT?
There was a problem hiding this comment.
I fully agree about having the properties in their own pom. I suggested that a while ago.
Not entirely sure it will work to have them in the root pom, depends if all the pom requiring them has it as parent. Otherwise, it can be a specific pom.
I agree it should be done in a separate issue.
There was a problem hiding this comment.
Thanks both of you for your input!
There was a problem hiding this comment.
Not entirely sure it will work to have them in the root pom, depends if all the pom requiring them has it as parent. Otherwise, it can be a specific pom.
Either way that properties pom will have to be a parent pom for every subproject. Which is what the root pom is supposed to be, otherwise the hierarchy for the project is going to break which effectively means independent projects.
geoand
force-pushed
the
kotlin-devmode
branch
from
53414da to
42ac250
Compare
|
Looks good. Haven't fully tested yet, but does it work in Gradle? |
Nope, it will require a lot of work to get it working in Gradle as well. That is because in Gradle we can't just read the configuration, we'll need to read the Kotlin Gradle plugin data. It should definitely be done when all the other Gradle improvements things are in place. |
|
@gastaldi would you be interested in doing a follow up for Gradle as some point? |
4 participants
Partially fixes: #2811 (Gradle support will be part of #6035)