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

Migrate build scripts to Kotlin-DSL #1980

Draft
wants to merge 3 commits into
base: stable-7
from

Conversation

Projects
None yet
4 participants
@gabizou
Copy link
Member

gabizou commented Mar 8, 2019

Status: WIP

Currently, our scripts across the projects are all for legacy Gradle, and while "they just work", making changes has been cumbersome, and not as self evident as far as where things are done.

In migrating to Kotlin-DSL, the changes also centralize where various dependencies are declared, and provide build script plugins that are type safe for IDE debugging and compilation checks.

TODO:

  • Migrate build.gradle
  • Event-gen-impl changes for Gradle 5 support (requires back porting support for event-gen-impl versions to work without breaking binary changes
  • Migrate remainder of the sponge.gradle build logic so it can be applied to SpongeCommon and other implementations
  • Migrate deploy.gradle
  • Update README.md to notate the usage of build scripts and where dependencies are declared
Initial rewrite of buildscripts to use Kotlin-DSL with Gradle 5.2.1. …
…Not everything is ported.

Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

@gabizou gabizou requested a review from Aaron1011 Mar 8, 2019

object Libs {

const val slf4j = "${Deps.Groups.slf4j}:${Deps.Modules.slf4j}:${Versions.slf4j}"
const val guava = "${Deps.Groups.guava}:${Deps.Modules.guava}:${Versions.guava}"

This comment has been minimized.

@Minecrell

Minecrell Mar 8, 2019

Member

Shared constants are useful if they are used many times and change often. But having more or less every string in a constant actually makes the scripts less readable here in my opinion:

  • Instead of having a plain simple com.google.guava:guava:21.0, you need to search through several files to find the exact dependencies that are being used by a module:

    • Find referenced dependencies in build.gradle.kts
    • Look at the declarations in Libs
    • Get group ID / artifact ID from Deps
    • Get version from Versions

    Usually this won't be a big problem because Gradle and IDEs can display the resolved dependencies, but sometimes you still look at the source code directly (e.g. on GitHub).

  • To declare a new dependency, you need to add lines in up to 5 different files now instead of adding a single line in one file.

Maven group/artifact IDs and Gradle plugin IDs barely ever change and in my opinion they are easier recognizable if referenced directly instead of through a intermediary variable.

As for the versions, I agree that this should be done for the Gradle plugin versions as they still need to be defined in each root Gradle build script, but even dependency versions should be usually only used once. Common dependencies are either added by a shared build script (e.g. test dependencies) or through transitive dependencies (e.g. SpongeAPI depends on plugin-meta, SpongeCommon depends on SpongeAPI).

}

// Plugin meta
compileOnly(Libs.plugin_meta)

This comment has been minimized.

@Minecrell

Minecrell Mar 8, 2019

Member

Why are all these compileOnly now? This means that they will not be exposed to any modules depending on SpongeAPI (plugins, implementations).

val javadocJar by registering(Jar::class) {
dependsOn(javadoc)
archiveClassifier.set("javadoc")
from(javadoc.get().destinationDir)

This comment has been minimized.

@Minecrell

Minecrell Mar 8, 2019

Member

from(javadoc), remove dependsOn(javadoc)

archiveClassifier.set("shaded")
}
val sortClassFields by existing(TaskSortClassMembers::class) {
add(sourceSets.main.get(), "org.spongepowered.api.CatalogTypes")

This comment has been minimized.

@Minecrell

Minecrell Mar 8, 2019

Member

The duplication of sourceSets.main.get() is rather annoying here. With the amount of files we list here, one of the following options might be nicer:

  • Search through all source files for SORTFIELDS:ON. Then we don't need to list any of them here.
  • Add a addAll() method to TaskSortClassMembers
  • Use something like
    listOf(
        "org.spongepowered.api.CatalogTypes",
        "org.spongepowered.api.advancement.AdvancementTypes", 
        "org.spongepowered.api.advancement.criteria.trigger.Triggers",
    ).forEach {
        add(sourceSets.main.get(), it)
    }
}

dependencies {
compileOnly("net.minecraftforge.gradle:ForgeGradle:2.3-SNAPSHOT")

This comment has been minimized.

@Minecrell

Minecrell Mar 8, 2019

Member

Is it possible to move all ForgeGradle related code to SpongeCommon? SpongeAPI does not rely on proprietary Minecraft code and we should try to keep it that way for other implementations (e.g. Lantern).

repositories {
jcenter()
maven("https://repo.spongepowered.org/maven")
gradlePluginPortal()

This comment has been minimized.

@Minecrell

Minecrell Mar 8, 2019

Member

gradlePluginPortal() includes jcenter(), so

repositories {
    gradlePluginPortal()
    maven("https://repo.spongepowered.org/maven")
}

should be equivalent. However, I believe we have most (all?) of our Gradle plugins on the Gradle plugin portal anyway.

That reminds me, I did message them to transfer all plugins to the SpongePowered account, but never got any reply...

dependsOn(genEventImpl.get())
}
compileJava {
dependsOn(genEventImpl.get())

This comment has been minimized.

@Minecrell

Minecrell Mar 8, 2019

Member

Is this necessary? event-impl-gen should do this automatically.

Aaron1011 and others added some commits Mar 8, 2019

Finish porting the remaining buildscripts. Declare versions and other
variables where needed to the root project etc.

Moved the plugin dependencies into the buildSrc buildscript classpath so
that we can access their classes within the buildSrc. I had not previously
known that one could not access plugins defined by the parent project
script without the plugins being exposed to the buildSrc classpath
explicitly. My guess is that the plugins are provided as dependencies for
the project's buildscript because they are already applied to the
buildscript as a "project", so access to the plugins themselves through
that classpath is required.

Merged all the dependency objects into a single file, SpongeAPI.kt to
allow the changes required to add dependencies to remain small (three
lines in a single file is easier than one line in three files).

Signed-off-by: Gabriel Harris-Rouquette <gabizou@me.com>

@gabizou gabizou force-pushed the build/kotlin-dsl branch from 685329e to 3420149 Mar 9, 2019

}

apply(plugin = Plugins.spongegradle)
apply(plugin = Plugins.spongemeta)

This comment has been minimized.

@Barteks2x

Barteks2x Mar 9, 2019

I'm really curious how this doesn't break automatic generated accessors in kotlin dsl. Last time I tried (gradle 4.9 and 4.8) automatic generated accessors wouldn't work if apply plugin was used.

This comment has been minimized.

@gabizou

gabizou Mar 9, 2019

Author Member

@Barteks2x because the main gradle plugin is still depended on in the class path, the "sub plugins" are still available to the class path, but not normally applied. Taking advantage of that allows us to apply it and use the getting or existing accessors with the appropriate classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.