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

Add support for Gradle Kotlin DSL #9353

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

mgorniew
Copy link
Contributor

Hi,

This will add support for Kotlin DSL in Quarkus Gradle tools.

It will also fix #5776

@boring-cyborg boring-cyborg bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform labels May 15, 2020
@geoand
Copy link
Contributor

geoand commented May 15, 2020

Thanks a lot for this!

@maxandersen @aloubyansky I think you'll probably want to check this out

@aloubyansky
Copy link
Member

Thanks @mgorniew

@mgorniew mgorniew force-pushed the gradle_kotlin_dsl branch 2 times, most recently from d1da13f to b5c6b5b Compare May 16, 2020 20:38
@mgorniew mgorniew marked this pull request as ready for review May 16, 2020 20:39
@mgorniew mgorniew force-pushed the gradle_kotlin_dsl branch from b5c6b5b to 8cf38b8 Compare May 17, 2020 11:50
@gsmet
Copy link
Member

gsmet commented Jul 10, 2020

@aloubyansky what's the status of this one? It has a lot of conflicts now unfortunately :/

@aloubyansky
Copy link
Member

We need to rebase this. @mgorniew sorry, I am not sure why I didn't merge it, tbh. If you could rebase it, it'll be much appreciated, if not then I'll look into this.

@aloubyansky aloubyansky self-assigned this Jul 10, 2020
@mgorniew
Copy link
Contributor Author

mgorniew commented Jul 10, 2020 via email

@aloubyansky
Copy link
Member

@ia3andy fyi gradle kotlin dsl support
Thanks a lot @mgorniew


java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should remove this, since our CI will be also testing it under Java 11. And I remember we had issues with Kotlin and Java 14. If you run into that, the tests may need to be disabled for that version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ia3andy
Copy link
Contributor

ia3andy commented Jul 10, 2020

Just as a quick note, everything related to generating new project using Kotlin DSL is going to be replaced by the new codestart system, so we will soon need to implement it there too:
#10372

@mgorniew mgorniew force-pushed the gradle_kotlin_dsl branch from 8cf38b8 to b8d38a7 Compare July 14, 2020 10:33
@gsmet
Copy link
Member

gsmet commented Jul 29, 2020

@aloubyansky this one got outdated again. Maybe you could sync with @mgorniew at the beginning of the 1.8 cycle?

@ia3andy
Copy link
Contributor

ia3andy commented Jul 29, 2020

@gsmet Or.. we could wait for #10372 to be merged, and add the kotlin-dsl buildtool codestart to it. It will be available from the Maven plugin using -DcodestartsEnabled.

@aloubyansky
Copy link
Member

Looking at my emails I don't see a notification of the pushed update. That is strange. But explains how I missed it. Sorry again @mgorniew At this point though I would take @ia3andy's suggestion. @mgorniew you've got all the reasons to lose your motivation by now. Sorry about that. In case you still would like to get this feature in, please let @ia3andy know. We need it for sure.

@ia3andy
Copy link
Contributor

ia3andy commented Jul 29, 2020

@mgorniew it would be a pleasure to help you to add kotlin dsl as a codestart.. let me know :)

@mgorniew
Copy link
Contributor Author

@ia3andy I think best way is to wait now for #10372 merge. Then I can rebase this PR and add kotlin dsl to codestart. I didn't have time yet to look at codedstart, so for now I've only have one question. With#10372, does all changes made in this PR are still valid and I should just add something more to support codestart? Or some functionality was moved entirely to codestarts, eg. templates from platform-descriptor-json?

@ia3andy
Copy link
Contributor

ia3andy commented Jul 29, 2020

@ia3andy I think best way is to wait now for #10372 merge. Then I can rebase this PR and add kotlin dsl to codestart. I didn't have time yet to look at codedstart, so for now I've only have one question. With#10372, does all changes made in this PR are still valid and I should just add something more to support codestart?
Or some functionality was moved entirely to codestarts, eg. templates from platform-descriptor-json?

The goal with codestarts is to replace all the current codegen (templates from platform-descriptor-json), but we don't plan to migrate in one shot, the current templates will be the default until this is stable enough. So instead of having the Kotlin DSL as part of the future "legacy" codegen, I think it makes sense to have kotlin DSL as experimental in the codestarts to start. It could also be a way to make people use the codestarts so we gather some feedback :)

@mgorniew
Copy link
Contributor Author

mgorniew commented Jul 29, 2020

@ia3andy ok, so I will keep plugins changes (to add/remove extensions, etc.) and move templates/project bootstrapping code to codestarts.

@ia3andy
Copy link
Contributor

ia3andy commented Jul 29, 2020

Yes you need to keep the code the Add/Remove code, this is meant to be kept!

@aloubyansky
Copy link
Member

thanks @mgorniew

@ia3andy
Copy link
Contributor

ia3andy commented Aug 3, 2020

@mgorniew Codestarts are merged :)

I think that kotlin DSL codestart will be really close to the gradle one:
https://github.com/quarkusio/quarkus/blob/master/devtools/platform-descriptor-json/src/main/resources/bundled-codestarts/buildtool/gradle/base/build-layout.include.qute

Let me know if you have questions..

@mgorniew
Copy link
Contributor Author

mgorniew commented Aug 3, 2020

@ia3andy Ok, thanks. I will look at this and try to update this pull request tomorrow.

@mgorniew mgorniew force-pushed the gradle_kotlin_dsl branch from b8d38a7 to 493d066 Compare August 5, 2020 21:25
@mgorniew
Copy link
Contributor Author

mgorniew commented Aug 5, 2020

@aloubyansky @ia3andy I've made changes required to support codestarts, please review.

I've found two possible issues:

  • Scala codestart has dependency "org.scala-lang:scala-library:${version.scala}" and "${version.scala}" doesn't not resolve. For Groovy build files is is changed to "undefined", but this still works - probably because this dependency is managed. With kotlin dsl this property will fail at build file parsing. I've changed this "org.scala-lang:scala-library" in gradle-kotlin-dsl codestart. But I'm not sure if this ok.

  • addExtension task will also add platform bom even if "enforcedPlatform(...)" is there already. Should this work in that way?

@ia3andy
Copy link
Contributor

ia3andy commented Aug 6, 2020

@aloubyansky @ia3andy I've made changes required to support codestarts, please review.

I've found two possible issues:

  • Scala codestart has dependency "org.scala-lang:scala-library:${version.scala}" and "${version.scala}" doesn't not resolve. For Groovy build files is is changed to "undefined", but this still works - probably because this dependency is managed. With kotlin dsl this property will fail at build file parsing. I've changed this "org.scala-lang:scala-library" in gradle-kotlin-dsl codestart. But I'm not sure if this ok.

This was a mistake and will be fixed with: 62312d3 (#11235)

  • addExtension task will also add platform bom even if "enforcedPlatform(...)" is there already. Should this work in that way?

I think that's a bug, why would we add it multiple time?

@mgorniew also note that in the commit I inverted version to name.version instead of version.name to make it a bit more consistent and if you merge after you will also need to add the gradle wrapper:
#11235

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aloubyansky
Copy link
Member

Thanks @mgorniew that's a great contribution. Please create an issue for the duplicated enforcedPlatform.

@mgorniew
Copy link
Contributor Author

mgorniew commented Aug 6, 2020

@aloubyansky I've created #11251 for dependencies issue.

@ia3andy
Copy link
Contributor

ia3andy commented Aug 6, 2020

@mgorniew you can consider your job done here, excellent work! If that's ok to you, I will merge #11235 first because it needs be backported, I'll take care or updating this PR with the changes :)

@mgorniew
Copy link
Contributor Author

@ia3andy @aloubyansky @gastaldi Please check rebased code. This include fix from #11255, but it is handled in slightly different way. I hope to make this less fragile with #11303, but I would like to have this merged into master first.

@ia3andy ia3andy requested a review from gastaldi August 12, 2020 06:52
@ia3andy
Copy link
Contributor

ia3andy commented Aug 12, 2020

@gastaldi could you have a look before I merge, it now takes rebase with your fix for bom.

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgorniew I missed it sorry, but you need to add the Gradle wrappers to the new buildtool codestart, have a look to the Gradle one (don't forget to also add the version data).

It should be a matter of seconds :)

@mgorniew
Copy link
Contributor Author

@ia3andy I've added wrapper.

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@ia3andy
Copy link
Contributor

ia3andy commented Aug 12, 2020

@mgorniew it seems tests are failing, can you have a look?

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I found the problem for JDK 11 tests, you need to use kotlin.version in the templates. For JDK 8, I have no idea..

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crossing fingers this one is the one :)

@ia3andy ia3andy merged commit f90bea3 into quarkusio:master Aug 13, 2020
@ia3andy
Copy link
Contributor

ia3andy commented Aug 13, 2020

Good job @mgorniew!!

@geoand geoand added this to the 1.8.0 - master milestone Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codestarts area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Kotlin DSL Gradle projects when adding/listing/removing extensions
5 participants