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 runApplication() Kotlin top level function #10511

Closed
wants to merge 3 commits into from
Closed

Add runApplication() Kotlin top level function #10511

wants to merge 3 commits into from

Conversation

sdeleuze
Copy link
Contributor

@sdeleuze sdeleuze commented Oct 4, 2017

Since Kotlin extensions do not apply to static methods, this commit introduces a runApplication() Kotlin top level function that acts as a Kotlin shortcut for SpringApplication.run().

This shortcut avoids to require using non-idiomatic code like SpringApplication.run(FooApplication::class.java) and provides a runApplication<FooApplication>() alternative (as well as an array of KClass based alternative when multiple classes need to be passed as parameter).

This commit also adds Kotlin main artifacts to Spring Boot dependency management and will be replaced by Kotlin BOM when it will be available (see KT-18398).

runApplication<FooApplication>() is a form usually preferred in Kotlin to runApplication(FooApplication::class) and consistent with what we do on Spring Framework side.

The custom Maven configuration is the typical way to deal with Java + Kotlin project, a similar configuration has been applied to Spring Data Kay.

Closes gh-8511

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 4, 2017

The build failed for an unrelated flacky WebFlux test, but ./mvnw clean install works fine locally and I have validated the top level function is usable in my MiXiT sample application.

Copy link
Member

@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 for the PR, I've added a few suggestions.

Generally speaking, I am a bit nervous about the maven compiler plugin setup and the impacts on the IDE integration.

</dependency>
<dependency>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-stdlib-jre7</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

We don't support Java 7 so that should go away.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I agree with that. I'd rather that the dependency management was complete.

Copy link
Member

Choose a reason for hiding this comment

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

I am not following. When are we going to use that dependency if we don't support Java 7 anymore?

Copy link
Contributor Author

@sdeleuze sdeleuze Oct 5, 2017

Choose a reason for hiding this comment

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

We support Java 8 so kotlin-stdlib, kotlin-stdlib-jre7 and kotlin-stdlib-jre8 should be specified for consistent versioning. kotlin-stdlib contains all methods usable on Java 6, kotlin-stdlib-jre7 contains Java 7 specific methods which are needed for Java 8 support, same for kotlin-stdlib-jre8 and Java 8 specific methods.

<parameters>true</parameters>
</configuration>
<executions>
<execution>
Copy link
Member

Choose a reason for hiding this comment

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

What are those? Can you explain?

Copy link
Member

Choose a reason for hiding this comment

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

Scratch that. I didn't read your description properly, sorry.


@Bean
open fun webServer(): TomcatServletWebServerFactory {
return TomcatServletWebServerFactory(0)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you need to create a web server for this test? That looks a bit overkill to me.

Copy link
Contributor Author

@sdeleuze sdeleuze Oct 5, 2017

Choose a reason for hiding this comment

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

Yes, because if not the test fails, I just copied what you do in SpringApplicationTests on Java side.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 5, 2017

For Maven plugin configuration, it is the standard way to compile Java + Kotlin projects, I have on purpose only applied it to the module where we need that, we do the same in Spring Data without issues reported, and many other project do that. If you face issue, I can ask Kotlin team to fixit, if not I would recommand go that way since I have no alternative to propose.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 6, 2017

Hum, there is already a kotlin.run top level function, so reusing that name for Boot may cause confusion. I will update the PR with another function name.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 6, 2017

I have splitted this PR in 2 commits, one for dependency management, the other for the extension itself. I have also renamed the top level function to runApplication() which seems to me a good trade off between run() which will cause confusion with kotlin.run() and looks reasonably short (runSpringApplication() looks ugly for such widely used function).

@sdeleuze sdeleuze changed the title Add run() Kotlin top level function Add runApplication() Kotlin top level function Oct 6, 2017
@sdeleuze
Copy link
Contributor Author

sdeleuze commented Oct 6, 2017

I have also rebased this PR on top of the recent module structure changes.

@snicoll snicoll added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 7, 2017
@snicoll snicoll added this to the 2.0.0.M6 milestone Oct 7, 2017
@GreyTeardrop
Copy link
Contributor

Hi! Awesome to see even more Kotlin niceties in Boot!

@sdeleuze,
Do you think it could make sense to also add something like this:

inline fun <reified T : Any> runApplication(vararg args: String,
                                            block: SpringApplication.() -> Unit): ConfigurableApplicationContext =
    SpringApplication(T::class.java).apply(block).run(*args)

for cases when some SpringApplication tuning is required?

@sdeleuze
Copy link
Contributor Author

@GreyTeardrop Great idea, this makes this feature much more flexible.

@snicoll I have updated the PR with this proposal and added related tests. I have also created the related #10712 issue.

This commit adds Kotlin main artifacts to Spring
Boot dependency management and will be replaced by
Kotlin BOM when it will be available (see KT-18398).

gh-9486
Since Kotlin extensions do not apply to static
methods, this commit introduces a runApplication()
Kotlin top level function that acts as a Kotlin
shortcut for SpringApplication.run().

This shortcut avoids to require using non-idiomatic
code like SpringApplication.run(FooApplication::class.java)
and provides a runApplication<FooApplication>() alternative
(as well as an array of KClass based alternative when
multiple classes need to be passed as parameter).

It is possible to customize the application with the
following syntax:

runApplication<FooApplication>() {
    setEnvironment(environment)
}

Closes gh-8511
@sdeleuze
Copy link
Contributor Author

I have added plugin management for Maven plugin, after discussion with @wilkinsona sadly we can't do similar things with Gradle plugin. Unfortunate but IMO we can live with that, it is possible to have different minor versions between dependencies and plugin.

@snicoll
Copy link
Member

snicoll commented Oct 25, 2017

@wilkinsona this would require a Spring Boot version to Kotlin version mapping in start.spring.io for Gradle projecs and I am really not keen to add that.

Would it be possible to extrac the kotlin.version from the BOM and use that as a property to define the version of the plugin to use? Or do something of the kind in our own dependency management plugin?

@wilkinsona
Copy link
Member

I'd have to investigate, but right now I don't know how to do it, particularly with the plugins DSL. We can only extract the kotlin.version from the bom once the Spring Boot and Dependency Management plugins have been applied. By that point, the Kotlin plugin may already have been applied too. I think there's a chicken and egg problem here.

@wilkinsona
Copy link
Member

We avoid the problem for the Dependency Management plugin by making the Spring Boot plugin pull it in transitively. That lets us control the version of the Dependency Management plugin and allows a user to just apply the plugin without worrying about its version. We could do the same for the Kotlin plugin, but then everyone building a Gradle project would download the Kotlin plugin even if they weren't using it. That's similar to what we did for Jackson's Kotlin module so we could decide that it's a reasonable thing to do, probably depending on the weight of the Kotlin plugin and its dependencies.

@snicoll snicoll self-assigned this Oct 26, 2017
snicoll pushed a commit to snicoll/spring-boot that referenced this pull request Oct 27, 2017
Since Kotlin extensions do not apply to static
methods, this commit introduces a runApplication()
Kotlin top level function that acts as a Kotlin
shortcut for SpringApplication.run().

This shortcut avoids to require using non-idiomatic
code like SpringApplication.run(FooApplication::class.java)
and provides a runApplication<FooApplication>() alternative
(as well as an array of KClass based alternative when
multiple classes need to be passed as parameter).

It is possible to customize the application with the
following syntax:

runApplication<FooApplication>() {
    setEnvironment(environment)
}

See spring-projectsgh-10511
snicoll added a commit to snicoll/spring-boot that referenced this pull request Oct 27, 2017
snicoll pushed a commit that referenced this pull request Oct 27, 2017
Since Kotlin extensions do not apply to static
methods, this commit introduces a runApplication()
Kotlin top level function that acts as a Kotlin
shortcut for SpringApplication.run().

This shortcut avoids to require using non-idiomatic
code like SpringApplication.run(FooApplication::class.java)
and provides a runApplication<FooApplication>() alternative
(as well as an array of KClass based alternative when
multiple classes need to be passed as parameter).

It is possible to customize the application with the
following syntax:

runApplication<FooApplication>() {
    setEnvironment(environment)
}

See gh-10511
@snicoll snicoll closed this in fd07bfd Oct 27, 2017
snicoll added a commit that referenced this pull request Oct 27, 2017
* pr/10511:
  Polish "Add runApplication() Kotlin top level function"
  Specify kotlin-maven-plugin version for plugin management
  Add runApplication() Kotlin top level function
  Add Kotlin main artifacts to dependency management
sdeleuze added a commit to sdeleuze/spring-kotlin-deepdive that referenced this pull request Oct 28, 2017
sdeleuze added a commit to sdeleuze/spring-kotlin-deepdive that referenced this pull request Oct 28, 2017
sdeleuze added a commit to sdeleuze/spring-kotlin-deepdive that referenced this pull request Oct 28, 2017
@philwebb philwebb reopened this Nov 1, 2017
@philwebb
Copy link
Member

philwebb commented Nov 1, 2017

I'm guessing 90% of applications are going to do something like this:

fun main(args : Array<String>) { 
    runApplication<ExampleApp>(args)
}

I'm wondering if we shouldn't drop the multiple config version? Doing this:

runApplication(arrayOf(ExampleConfig::class, ExampleWebConfig::class), args)

Doesn't seem much better than:

SpringApplication(ExampleConfig::class, ExampleWebConfig::class).run(args)

@sdeleuze Are there any benefits to the arrayOf version that I'm missing?

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Nov 1, 2017

@philwebb Good point, indeed I think we can remove the Array variant.

@philwebb
Copy link
Member

philwebb commented Nov 1, 2017

I'll take care of that

@snicoll snicoll closed this in e28b98c Nov 2, 2017
@sdeleuze sdeleuze mentioned this pull request Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants