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

Remove the kotlin-dsl plugin #112

Merged
merged 5 commits into from
Feb 16, 2020

Conversation

martinbonnin
Copy link
Contributor

Closes #108

It turns out the kotlin-dsl plugin embeds code that's using Gradle 6.1 classes while we shouldn't really need this. Remove it and use the plain java-gradle-plugin instead.

Also add some checks and tests about Gradle version.

@@ -56,7 +56,7 @@ Once you setup the plugin correctly, you can call the `:generateSwagger` gradle

In order to run this gradle plugin you need to fulfill the following requirements:

* Gradle 5.x - This plugin uses Gradle 5 features, and you will need to setup your Gradle wrapper to use 5.0 or more.
* Gradle 5.x - This plugin uses Gradle 5 features, and you will need to setup your Gradle wrapper to use 5.4.1 or more.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bumped from 5.0 to 5.4.1 because the testProject fails on 5.0 due to:

Build file '/home/martin/git/swagger-gradle-codegen/gradle-plugin/plugin/junit7355662627637669365/project/build.gradle.kts' line: 9

* What went wrong:
Script compilation errors:

  Line 09:       content {
                 ^ Unresolved reference: content

  Line 10:         excludeGroup("com.yelp.codegen")
                   ^ Unresolved reference: excludeGroup

I think it should be safe to assume most people are using gradle >= 5.4.1 but we can certainly remove that limitation if really needed.

Copy link
Collaborator

@macisamuele macisamuele left a comment

Choose a reason for hiding this comment

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

@martinbonnin I would personally prefer to use kotlin-dsl as far as possile.
I did open #111 to deal with the problem mentioned in #108 .

The approach proposed on the other PR has one "major" downside: the project itself cannot benefit from the latest gradle features. But I don't think it is going to be a major blocker.

@cortinico
Copy link
Collaborator

@martinbonnin I would personally prefer to use kotlin-dsl as far as possile.

+1

@martinbonnin
Copy link
Contributor Author

@macisamuele no problem if you want to keep it but as you said, it's bringing a downside and I don't see the upside. The amount of code is almost similar and it might bring things like HasImplicitReceiver to allow to use this instead of it but this almost sounds like a downside too. For someone used to Kotlin, this transformation looks a bit magic.
Or are there more advantages ?

@cortinico
Copy link
Collaborator

Or are there more advantages ?

kotlin-dsl plugin allows us to use the precompiled plugins:
https://docs.gradle.org/current/userguide/kotlin_dsl.html#kotdsl:precompiled_plugins
(They actually added a note that covers exactly our scenario)

That is convenient as it simplifies our Gradle setup. I'll try to stick to it as long as it's possible. Although, I don't have a really strong opinion on this, we could even drop if you have a strong argument @martinbonnin 👍

@cortinico
Copy link
Collaborator

Following up the discussions we had on Slack here

We decided for replacing kotlin-dsl plugin with java-gradle-plugin for the reasons aforementioned by @martinbonnin.
I'm merging this and will prepare an upcoming release soon 👍

@cortinico cortinico self-requested a review February 16, 2020 13:50
@cortinico cortinico merged commit 730a39c into Yelp:master Feb 16, 2020
@cortinico cortinico added this to the 1.4.0 milestone Feb 17, 2020
@cortinico cortinico added the infra PR or Issue related to project infrastructure label Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra PR or Issue related to project infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to load class 'org.gradle.kotlin.dsl.precompile.v1.PrecompiledProjectScript'.
3 participants