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 Gradle Kotlin DSL support #851

Closed
wants to merge 4 commits into from

Conversation

jnizet
Copy link
Contributor

@jnizet jnizet commented Mar 3, 2019

refs #334

This adds support for the Gradle Kotlin DSL in initializr.
I've tried my best to split the changes into atomic commits that can be reviewed and applied one by one.

The first commit applies the recommended way of configuring Kotlin compilation tasks, as suggested in https://github.com/eskatos/start.spring.io-builds/blob/master/new-gradle-groovy-dsl/kotlin-boot-2-minimal/build.gradle

The remaining commits actually add a Gradle KTS build system.

Here are some things I'm not sure about, and that the reviewer should review carefully:

  1. The recommended way in .kts build scripts to apply the dependency management plugin is to use the plugins block. This allows using idiomatic Kotlin configuration of the plugin. Unfortunately, it also requires to specify the dependency management plugin version. I used metadata.getConfiguration().getEnv().getGradle().getDependencyManagementPluginVersion() to get it, but I'm not actually sure this is correct: the value for this property in start.spring.io is not the one I expected (1.0.6.RELEASE). Maybe it isn't used for now. It is in the Kotlin gradle scripts, so if this is the right property, it will have to be updated.
  2. The Kotlin DSL is only usable with Gradle 5. But choosing an old Spring Boot platform version automatically applies Gradle 3 or Gradle 4. I'm not sure how best to handle the situation. I guess we could add some test that would throw an exception if such an invalid combination is chosen, but I don't know where to do that and which exception to throw (or if it's even the best way). Regarding tests (especially the BuildComplianceTests), I've simply disabled such invalid configurations.
  3. I don't know exactly how the web part and the metadata work, but the code of the web module most probably needs some additional changes, and the code of start.spring.io too, in order to actually enable and configure the Gradle KTS build system. I guess some plugins might also have to adapt to this new build system.

Copy link
Contributor

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks a lot for opening the PR. I've some preliminary requests before going any further.

If you don't mind, I'd like to merge 083d28b and aa33970 as a separate polish PR. Would you mind opening a separate PR with those commits?

9db44e1 looks like a bug to me, can we have a separate PR for it please?

I also intended to extend all conditions to accept a value array and support OR semantic (rather than doing this one by one). This may make one commit in this PR outdated.

I haven't given a lot of thoughts but I don't know yet if we should create a dedicated BuildSystem for the Kotlin DSL. At first sight, I'd prefer to keep a single Gradle build system but I'll have a deeper look later.

@@ -39,6 +40,7 @@
* Project generation configuration for projects using any build system.
*
* @author Andy Wilkinson
* @author JB Nizet
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind harmonizing your name to be @author FirstName LastName?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Mar 4, 2019
@jnizet
Copy link
Contributor Author

jnizet commented Mar 4, 2019

@snicoll I've created #852 with the two first commits (missing spaces polish).
I'll do the remaining requested changes once it's merged if you don't mind.

@jnizet
Copy link
Contributor Author

jnizet commented Mar 4, 2019

Created #853 for the Kotlin JPA plugin issue

Instead of configuring compile compileKotlin and compileTestKotlin, configure all tasks of type KotlinCompile at once.
This allows defining beans for both gradle and gradle-kts build systems at once
@jnizet
Copy link
Contributor Author

jnizet commented Mar 4, 2019

Rebased on master, and first name changed as requested.

Refactor the existing Groovy DSL writers in order to share code between the Groovy DSL and Kotlin DSL writers
@sdeleuze
Copy link
Contributor

sdeleuze commented Mar 5, 2019

In term of UI, my 2 proposals are:
screen1
Or
screen2

@jlstrater
Copy link

jlstrater commented Mar 5, 2019

I like the second one better although it might be good to have the left one say Groovy DSL. What do you think @eskatos?

@wilkinsona
Copy link
Contributor

I've gone back and forth on this, but I'm back to thinking that we should do this:

Can we automatically generate a Kotlin-powered build.gradle.kts when the users selects Gradle as the build system and Kotlin as the language? That way we'd avoid another UI option at least.

I still like the idea of avoiding the need for a UI option and those who chose to use Kotlin for their application's code are, presumably, a safe bet for wanting to use Kotlin for their build script too. It gives us a chance to roll out the Kotlin build script support somewhat gently. If there's demand for it from Groovy and Java users, we could consider offering a config option later on. By that time, any kinks in the support should have been ironed out before it's exposed to the significantly large audience of Java developers.

@sdeleuze
Copy link
Contributor

sdeleuze commented Mar 5, 2019

I strongly disagree. The Gradle DSL choice (Groovy or Kotlin) is totally orthogonal to the language chosen. Both are now first class citizen in Gradle documentation. Of course we can expect most Kotlin developers using Gradle project chosing it, but removing the possibility to use Gradle Groovy DSL will be a blocker for Kotlin developers who do not have the choice and some Java developers will want to leverage better autocomplete provided by Gradle Kotlin DSL. Mixing unrelated concerns like that seems confusing and not a good idea to me.

@jnizet
Copy link
Contributor Author

jnizet commented Mar 5, 2019

I agree with Sébastien. In my experience, many companies/developers still don't want to jump to Kotlin for their application code and want to stay with Java. But they dislike Groovy or would prefer static typing and auto-completion for their gradle build (where Java is not an option, and Kotlin is a better alternative than Groovy).
I would like to add that I would find it strange to have initializr choose the Gradle DSL for me based on heuristics, instead of simply asking what I prefer using, when the whole point of initializr is precisely to let the developer choose everything else (build tool, language, dependencies) about their project.

@snicoll
Copy link
Contributor

snicoll commented Mar 5, 2019

We want to experiment and roll out things progressively. Right now we don’t have a choice and I am keen to introduce this feature for kotlin users for a start. This may not please certain users. But some users aren’t pleased now because they can’t generate a kotlin-based build anyway.

Also, let’s not discuss UI here please as it is an unrelated concern to this PR and none of the phrasing exposed in those screenshots are possible without breaking other workflows.

@sdeleuze
Copy link
Contributor

@wilkinsona @snicoll After more thoughts, I think I can understand your position to roll out things progressively and introduce opinionated defaults, especially since we seem to agree on the middle term goal to extend Gradle Kotlin DSL support to other languages as a second step.

One of the reasons I changed my mind is the realization that in the long run, Spring Boot/Initializr opinions could be different depending on the language. For example, with upcoming Coroutines support we could possibly see an increased WebFlux usage ratio (versus Spring MVC) in Kotlin compared to Java. I also see other cultural and technical differences that, in the long run, could lead to different defaults. As far as we maintain the choice to not block users (in this case, it will be true middle term), I can see significant value in this kind of mechanism.

@snicoll snicoll added type: enhancement and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 12, 2019
@jnizet
Copy link
Contributor Author

jnizet commented Mar 15, 2019

Just to be sure: do you expect changes/feedback from me on this PR?

I'm willing to amend the code to make the transition to the Kotlin DSL more progressive, but I'd prefer having a review of what I have already done before going forward.
In particular, if gradle-kts should be modeled as a new build system as I did (and if not, how else).

I can wait if that's not in your priorities, of course, but I'd like to make sure we're not in a deadlock where everybody is waiting for a feedback from the other party :-)

@snicoll
Copy link
Contributor

snicoll commented Mar 15, 2019

Thanks @jnizet for asking. No, we need to review the PR but we have several other items to complete first. And thanks for offering to review the PR once we get to it.

@ghost
Copy link

ghost commented Mar 20, 2019

When will it be released?

@snicoll
Copy link
Contributor

snicoll commented Mar 20, 2019

@Xanthuim we have no timeline to communicate at the moment. The team is very busy and we'll do our best to review this proposal asap.

@snicoll
Copy link
Contributor

snicoll commented Apr 11, 2019

@jnizet could you please submit 386b64e as an independent PR? It looks good to me and a good idea to have regardless of this one.

If you rebase this PR, please drop 1e456fe as I've already pushed something similar on master yesterday, see #888

Copy link
Contributor

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

This is a general/high-level feedback to which I'd like your thoughts on. Part of this may make no sense as my knowledge of Gradle is limited.

I wonder how bad it would be to simply have two writers that are independent rather than the abstraction you've introduced here. I understand that there would be quite some copy/paste involved but I wonder if the implementation wouldn't be more simple if we had two writers. I am concerned about the amount of protected methods that were introduced. Does it make sense to create an abstraction for this? Wouldn't the Kotlin DSL deviate more in the future making this more and more hard to maintain in a single abstraction?

There are also quite a few new types that are introduced for the sole purpose of overriding one or two methods. I wonder how it would look like if we used functions and pass that in the constructor rather than having a dedicated type. Things like GroovyDslSettingsGradleProjectContributor for instance don't need a dedicated type IMO.

I am not sure about creating a different build system for this either. I think we have a flaw in the way the curent abstraction is represented at the moment. We've had something similar for the "jvm version" and introduced a parameter to the Language lookup. I don't see how we could do the same for the build system but I'd rather keep a single Gradle build system if possible.

Flagging for team attention as I'd like more feedback from the team on the above.

@jnizet
Copy link
Contributor Author

jnizet commented Apr 12, 2019

Hi Stéphane.

I've submitted a PR (#890) to apply the same configuration to all tasks of type KotlinCompile at once, as requested.

Regarding your questions, here's my feedback

I wonder how bad it would be to simply have two writers that are independent rather than the abstraction you've introduced here.

That's how I started implementing the Kotlin writer, but the structure of the generated file is so close to the structure of the file generated by the groovy writer that most of the code was just copied and pasted, and the pieces that were actually specific to the Kotlin DSL ended up being hidden into generic code that was in fact common to the groovy and the kotlin DSLs

I understand that there would be quite some copy/paste involved but I wonder if the implementation wouldn't be more simple if we had two writers.

I had the same doubt myself. I still have the feeling that the template pattern used here doesn't make things too hard to understand, and keeps the code DRYer.

I am concerned about the amount of protected methods that were introduced. Does it make sense to create an abstraction for this?

I'm not sure I would really call it an abstraction. The goal is not for this abstract class to have several more subclasses. The goal of the base class is clearly to reuse common code:

  • the public final method defines the structre of the build file, common to both DSLs
  • the protected final methods of the base class are utilities that are helpful for both DSLs
  • the protected abstract methods are the parts that are specific to each DSL. They're usually small syntactic issues (double quotes vs. single quotes, extension methods that exist in the Kotlin DSL but not in Groovy, groovy syntax vs. kotlin syntax of method calls).

I reused the same technique as the one that was already used (with fewer differences of course) to distinguish between the GradleBuildWriter and the Gradle3BuildWriter.

Wouldn't the Kotlin DSL deviate more in the future making this more and more hard to maintain in a single abstraction?

I don't think so. Having translated a big part of the Gradle user guide samples (see https://github.com/gradle/gradle/pulls?q=is%3Apr+author%3Ajnizet+is%3Aclosed) from Groovy to Kotlin, I can say that one of the main design goals is to keep the Groovy and Kotlin DSLs very close to each other, and both based on the same underlying Java API.

There are also quite a few new types that are introduced for the sole purpose of overriding one or two methods. I wonder how it would look like if we used functions and pass that in the constructor rather than having a dedicated type. Things like GroovyDslSettingsGradleProjectContributor for instance don't need a dedicated type IMO.

I also asked myself how best to design that. I chose to use subclasses for 3 reasons:

  • it's consistent with the design or the writers where a subclass specific to each DSL is used.
  • it allows introducing further differences (if needed) between the two implementations without having to modify the class creating the instances of these types
  • it hides the implementation details (i.e. the name of the generated file) from the caller, and keeps it in the specific subtype responsible to contribute to the generated project.

I am not sure about creating a different build system for this either. I think we have a flaw in the way the curent abstraction is represented at the moment. We've had something similar for the "jvm version" and introduced a parameter to the Language lookup. I don't see how we could do the same for the build system but I'd rather keep a single Gradle build system if possible.

That's the toughest issue. I don't know the project well enough to answer.
But I have the feeling that keeping a single gradle build system would need an additional option (DSL) to be chosen, and would thus make the initializr incompatible with previous versions, whereas introducing a new build system makes things easier.

snicoll added a commit to snicoll/initializr that referenced this pull request May 3, 2019
snicoll pushed a commit to snicoll/initializr that referenced this pull request May 10, 2019
This commit adds a BuildSystem implementation for the Gradle Kotlin DSL

See spring-iogh-851
snicoll pushed a commit to snicoll/initializr that referenced this pull request May 10, 2019
This commit refactors the existing Groovy DSL writers in order to share
code between the Groovy DSL and Kotlin DSL writers.

See spring-iogh-851
snicoll added a commit to snicoll/initializr that referenced this pull request May 10, 2019
snicoll added a commit to snicoll/initializr that referenced this pull request May 10, 2019
snicoll added a commit to snicoll/initializr that referenced this pull request May 10, 2019
snicoll added a commit to snicoll/initializr that referenced this pull request May 14, 2019
snicoll added a commit to snicoll/initializr that referenced this pull request May 14, 2019
In particular, this commit replaces the GradleKts dedicated build system
in favor of a dedicated build system dialect.

Closes spring-iogh-851

Co-authored-by: Andy Wilkinson <awilkinson@pivotal.io>
snicoll pushed a commit to snicoll/initializr that referenced this pull request May 15, 2019
This commit adds a BuildSystem implementation for the Gradle Kotlin DSL

See spring-iogh-851
snicoll pushed a commit to snicoll/initializr that referenced this pull request May 15, 2019
This commit refactors the existing Groovy DSL writers in order to share
code between the Groovy DSL and Kotlin DSL writers.

See spring-iogh-851
@snicoll snicoll added this to the 0.8.0 milestone May 16, 2019
@snicoll snicoll changed the title Feat/gradle kotlin dsl Add Gradle Kotlin DSL support May 16, 2019
@snicoll snicoll closed this in c7c1687 May 16, 2019
@snicoll
Copy link
Contributor

snicoll commented May 16, 2019

@jnizet thank you very much for your contribution. This is now merged with a polish commit.

The main change is a better API for build system dialect as we were not keen of having a dedicated build system for the Kotlin DSL variant.

@jnizet
Copy link
Contributor Author

jnizet commented May 16, 2019

Great, thanks. Looking forward to see ths in production.
Has the change on the UI (to choose the dialect) already been made? Do you already know when the change will be released?

@snicoll
Copy link
Contributor

snicoll commented May 16, 2019

It is live for a couple hours now. We just announced this feature at the Spring I/O keynote.

We’ve decided to enable this for kotlin-based project only for now. We will iterate on the feature based on usage and community feedback.

@jnizet
Copy link
Contributor Author

jnizet commented May 16, 2019

Woohoo!

@wilkinsona
Copy link
Contributor

@jnizet Thanks from me too for this. It was a pretty major and much appreciated contribution.

sayeedap pushed a commit to sayeedap/initializr that referenced this pull request Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants