chore[build]: convert facades/PC to gradle to kts #4175

keturn · 2020-10-02T16:32:49Z

This converts the gradle config from groovy to kotlin.

This PR is intended to be a near line-for-line port. Hold any further changes for another branch that won't be so cluttered by the conversion.

blocking #4157, as it needs to make changes to these JavaExec tasks.

How to test

Run gradlew :facades:PC:tasks for the list of tasks.

test the Terasology run tasks
- See Playing#Configuring a Server for information about how to configure the server settings from the gradle command line.
test the Terasology dist tasks
- dists should include jar files for all the modules you have sources checked out for in /modules/

keturn · 2020-10-02T16:42:40Z

facades/PC/build.gradle.kts

-ext {
-    // Default path to store server data if running headless via Gradle
-    localServerDataPath = 'terasology-server'
+// Default path to store server data if running headless via Gradle
+val localServerDataPath by extra("terasology-server")

-    // General props
-    mainClassName = 'org.terasology.engine.Terasology'
-    subDirLibs = 'libs'
-    templatesDir = new File(rootDir, 'templates')
-    rootDirDist = new File(rootDir, 'build/distributions')
+// General props
+val mainClassName by extra("org.terasology.engine.Terasology")
+val subDirLibs = "libs"
+val templatesDir = File(rootDir, "templates")
+val rootDirDist = File(rootDir, "build/distributions")

-    // Read environment variables, including variables passed by jenkins continuous integration server
-    env = System.getenv()
+// Inherited props
+val dirNatives: String by rootProject.extra
+val distsDirectory: DirectoryProperty by project;
+
+// Read environment variables, including variables passed by jenkins continuous integration server
+val env: MutableMap<String, String> = System.getenv()!!
+
+// Version related
+val startDateTimeString = dateTimeFormat.format(Date())!!
+val versionFileName = "VERSION"
+val versionBase by lazy { File(templatesDir, "version.txt").readText().trim() }
+val displayVersion = versionBase

-    // Version related
-    startDateTimeString = dateTimeFormat.format(new Date())
-    versionFileName = 'VERSION'
-    versionBase = new File(templatesDir, "version.txt").text.trim()
-    displayVersion = versionBase
-}


This isn't strictly line-for-line compatible because I only kept some of these as gradle "extra" properties. Most of them I left as local variables.

I don't believe it's a problem for the normal build environment, since there's nothing else in the build that interacts with them, but maybe it could be a concern if something like Jenkins has an alternate configuration that sets or uses them differently?

keturn · 2020-10-02T16:47:34Z

Reviewing by file on GitHub tells you the .gradle.kts is an entirely new file, but if you review by commit it'll actually show a diff.

keturn · 2020-10-03T17:11:22Z

marking as Draft while I fix a couple things with the distribution tasks

convert syntax and methods

keturn · 2020-10-03T18:11:53Z

I confirmed that the server task writes a config.cfg with the values from -PextraModules and such

and that the dist tasks make their zip files including module jars.

there might be some things involving version files or various BUILD_* environment variables that didn't get exercised in my development environment here.

skaldarnar

Looked over the changes and can confirm this is more or less a direct translation from Groovy to Kotlin DSL. Left a few comments, so please have a look.

I did not test the changes locally, though. Still setting up my system after hardware upgrade...

skaldarnar · 2020-10-03T18:09:36Z

facades/PC/build.gradle.kts

-    implementation project(':engine')
-    implementation group: 'org.reflections', name: 'reflections', version: '0.9.10'
+    implementation(project(":engine"))
+    implementation(group = "org.reflections", name = "reflections", version = "0.9.10")


On a side note: https://github.com/ronmamo/reflections is available in 0.9.12 now.

It has been for a while, but there are some widely reported issues with 0.9.12 (and some for 0.9.11) so I've been afraid to mess with it.

facades/PC/build.gradle.kts

skaldarnar · 2020-10-03T18:12:57Z

facades/PC/build.gradle.kts


-    dependsOn rootProject.moduleClasses
+    dependsOn(":moduleClasses")


Hm, so everything else seems to become more (type-)safe, while you reference the dependency via a plain string there? Is there another way to do this in the Kotlin DSL?

There's some tension going on in gradle-config-land. In one direction, you have Kotlin making things more type-safe. In the other direction, Gradle has this configuration-avoidance initiative to do lazy evaluation for certain types of things so it doesn't have to figure out the entire multi-project configuration tree before it's able to run a single task. That's discouraged direct links between sub-projects in many ways.

on second thought, I'm not sure how much the configuration-avoidance API has to do with this specific example.

from Kotlin's perspective, these are instances of Project. Project, as a Class, doesn't have a property named moduleClasses. build.gradle.kts files configure projects but they do not define new classes for each of them. Things like rootProject.moduleClasses only work with dynamic run-time attribute-lookup trickery.

We certainly could say something along the lines of rootProject.tasks.get("moduleClasses") but that's not any different than what we have here in terms of providing assurance that "moduleClasses" is a safe reference.

...and also the Gradle API is really big and provides a bunch of ways to get references to things and I certainly haven't learned everything about what's supposed to be the current best practice.

Long-term, the “safe” way involves not knowing about the tasks of other projects at all. You declare a dependency like "I depend on things from "org.terasology.engine:engine" and need assets of type <Jar> that conform to bytecode_level=JAVA_1_8" or something like that, the dependency-resolver does its thing and checks to see if that project exports anything that matches those criteria.

skaldarnar · 2020-10-03T18:23:37Z

facades/PC/build.gradle.kts

-    args "-homedir"
-    jvmArgs ["-Xmx1536m"]
+    args = listOf("-homedir")
+    jvmArgs = listOf("-Xmx1536m")


Would be nice to make this adjustable in the future via some kind of command line argument...

skaldarnar · 2020-10-03T18:29:51Z

facades/PC/build.gradle.kts

+configure<IdeaModel> {
    module {
        // Change around the output a bit
        inheritOutputDirs = false
-        outputDir = file('build/classes')
-        testOutputDir = file('build/testClasses')
+        outputDir = file("build/classes")
+        testOutputDir = file("build/testClasses")
    }
 }


Do we still need this with the new idea setup introduced recently?

no but no matter how many times people have asked me that none of them have ever removed it 😉

It can affect gestalt source module loading when you compile and run it via idea.
(And it compability with gradle build, it use same output dir, instead standart build//classes)

facades/PC/ doesn't have modules in it, so for the stuff that applies only to this sub-project we don't have to worry about the gestalt loader.

facades/PC/server.build.gradle

skaldarnar

I cannot start the server, but starting the game and building the distribution works fine! 👍

facades/PC/server.build.gradle

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

keturn commented Oct 2, 2020

View reviewed changes

keturn added 2 commits October 3, 2020 10:06

chore[build]: update for application.mainClass property name change

6fe680b

chore[build]: convert facades/PC/build.gradle to kotlin [WIP: move file]

0fc9b95

keturn force-pushed the chore/facade-gradle-kts branch from fd4e5ec to b264d9f Compare October 3, 2020 17:06

keturn marked this pull request as draft October 3, 2020 17:10

chore[build]: convert facades/PC/build.gradle to kotlin

0591882

convert syntax and methods

keturn force-pushed the chore/facade-gradle-kts branch from b264d9f to 0591882 Compare October 3, 2020 17:43

keturn marked this pull request as ready for review October 3, 2020 17:44

skaldarnar reviewed Oct 3, 2020

View reviewed changes

keturn requested a review from skaldarnar October 20, 2020 16:12

skaldarnar approved these changes Oct 20, 2020

View reviewed changes

facades/PC/server.build.gradle Outdated Show resolved Hide resolved

fix (gradle): remove obsolete comment

7340fc0

Co-authored-by: Tobias Nett <skaldarnar@googlemail.com>

keturn merged commit 52a0a52 into develop Oct 20, 2020

keturn added this to the v4.1.0 milestone Oct 20, 2020

DarkWeird mentioned this pull request Oct 21, 2020

fix(gradle-server): restore missing suffix for jvmarg -Xmx #4212

Merged

Cervator deleted the chore/facade-gradle-kts branch October 25, 2020 01:04

Cervator added the Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. label Oct 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore[build]: convert facades/PC to gradle to kts #4175

chore[build]: convert facades/PC to gradle to kts #4175

keturn commented Oct 2, 2020 •

edited

keturn Oct 2, 2020

keturn commented Oct 2, 2020

keturn commented Oct 3, 2020

keturn commented Oct 3, 2020

skaldarnar left a comment

skaldarnar Oct 3, 2020

keturn Oct 3, 2020

skaldarnar Oct 3, 2020

keturn Oct 3, 2020

keturn Oct 3, 2020

skaldarnar Oct 3, 2020

skaldarnar Oct 3, 2020

keturn Oct 3, 2020

DarkWeird Oct 20, 2020

keturn Oct 20, 2020

skaldarnar left a comment


		dependsOn rootProject.moduleClasses
		dependsOn(":moduleClasses")

chore[build]: convert facades/PC to gradle to kts #4175

chore[build]: convert facades/PC to gradle to kts #4175

Conversation

keturn commented Oct 2, 2020 • edited

How to test

Choose a reason for hiding this comment

keturn commented Oct 2, 2020

keturn commented Oct 3, 2020

keturn commented Oct 3, 2020

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaldarnar left a comment

Choose a reason for hiding this comment

keturn commented Oct 2, 2020 •

edited