build(facades.PC): tidy distribution tasks

[keturn]

This is a follow-up to #4343. I'm not aware of any open issues fixed by this, but it makes some build dependencies more consistent with the way #4343 manages them and addresses some TODOs that have been in the gradle files for a while.

• Removes RemoteModuleGatherer, the code that puts .jar files in modules/ during gradle dependency resolution. It's unnecessary under the modules-on-the-classpath strategy used by Fix/transient deps #4343, figuring out a better way to provide those dependencies without breaking the sandbox is dev workspace: modules on the java classpath breaks the sandbox #4375.
• Removes PC facade targets distApp and distModules, distPCZip, they are unused.
  • the gradle-standard distTar and distZip tasks are supported.
• Keeps distForLauncher so we don't need to change any of Launcher's assumptions about the zip file layout.
• Removes some overrides to gradle/IntelliJ build directories that were counterproductive.

Caveat

If you build a dist while you have module subprojects present, they will get lumped in on the classpath and not separated.

I think that's not a showstopper because our release builds are made without modules and then a separate process adds them to the distribution later.

How to test

• make some zips using :facades:PC:distZip and distForLauncher; make sure they are something you can unzip and start up (outside your development workspace) using the script included in its bin/
• in-game look at Extras / Credits and make sure it displays the credits file.

Outstanding before merging

• merge gradle upgrade upgrade gradle to 6.8.1 #4439
• do the start scripts need to be customized? (the only arg the old start scripts added was -Xmx)

[Cervator]

if it was important for those jar files to be in that directory

So I take it the modules jars from the Gradle cache gets used instead? Two main concerns there:

• Potentially confusing - especially if anybody ever wants to manually move jars files in and out of there, expecting them to be picked up at runtime by the engine (unless of course comparable source modules are present). Likewise with the files in there + source dirs you sort of know exactly what you got, if using stuff out of the Gradle cache you'd have to look elsewhere (Gradle logging, IntelliJ project structure ..)
• Probably a non-issue, but jobs in Jenkins using an engine dependency out of the Gradle cache dir has led to multiple frustrating issues due to execution starting with a working directory there - I dunno if that might happen again in any odd cases (probably mostly from alternative execution starting points like unit tests or weird utility classes)

versionInfo.properties

Used at runtime by the engine to show metadata, but if the file ends up in the same place by some other means that should be fine. It is also used in Jenkins / by the launcher, but likely same story.

Can test by faking some of the Jenkins env vars to trigger the file creation then look at the version label on the main menu

Credits.md

Used to power the in-game credits screen, which we used to update manually in a duplicate file, which was a pain. Now we just copy that file to where the engine can later read it. So probably same story - so long as it ends up in the same place we should be good 👍

Test by doing whatever thing that triggers the copy, then visiting the credits screen in-game. Without doing the thing that triggers the copy you just get a blank credits screen (or only part is missing? I forget) - if that becomes automatic with this then yay 🎉

So I think this PR may well be enough to where we can drop distForLauncher - although in that case we for sure need to remember to update Jenkins :-)

[keturn]

So I take it the modules jars from the Gradle cache gets used instead? […]

Potentially confusing - especially if anybody ever wants to manually move jars files in and out of there, expecting them to be picked up at runtime by the engine.

Well, I didn't remove any of that runtime code. So if you drop files in there it should behave as it does now. I'm not clear on what the use cases are that involve that.

and if we don't want the gradle cache used for module runtime dependencies, then we have to put #4343 on hold and talk about requirements around that. That's where the dependency sources changed. What's being removed in this PR is the now-redundant .jar copying.

[keturn]

What is this code in distForLauncher doing?

Terasology/facades/PC/build.gradle.kts

Lines 265 to 269 in b1ff0bb

	into("../resources/main/org/terasology/version") {
	from("$rootDir/engine/build/classes/org/terasology/version") {
	include("versionInfo.properties")
	}
	}

It looks like it's copying this resource out from engine and putting it in facade's resources, under the same resource path? 🤨

[Cervator]

I'm not clear on what the use cases are that involve that.

One perhaps contrived scenario: using the in-game module downloader will drop the retrieved jar files into modules/ - if you now have a conflict between a hidden file from the Gradle cache vs the visible file in the module dir which one wins?

I don't know why somebody would use the module downloader in a source workspace though. I would worry that there are use cases we'll miss.

have to put #4343 on hold and talk about requirements around that

That makes sense - and is probably partly why it is the way it is. I do remember in the long long ago also messing with the module dependencies and ending up in the Gradle cache as the "easy" option, but that having drawbacks. I can't really dig a lot of good details out of my memory though :-(

Is it not easily possible to capture the transitive dependencies but redirect them into a copy to modules/ ?

What is this code in distForLauncher doing?

I swear there's a reason for this but ... 😬

I tweaked it not too long ago: 00f44d1

But that was just from a Gradle update changing paths slightly. I don't remember why the copy is in both engine and PC. Could it simply be copy paste duplication from years ago?

I kinda wish we had a Nanoware test line in the launcher, so we could merge this there, build some game zips in Jenkins, make sure they list everything fine in the UI, see them show up in the launcher and work fine. That'd be ideal. Then try to take out a thing and see if it still works.

[keturn]

One perhaps contrived scenario: using the in-game module downloader will drop the retrieved jar files into modules/ - if you now have a conflict between a hidden file from the Gradle cache vs the visible file in the module dir which one wins?

Okay, that sort of thing is what I was afraid of when asking why the classpath stuff in the PC facade's build is the way it is: #4157 (comment)

I haven't been planning around how the runtime module downloader provides dependencies because I can't remember having seen it work in recent years.

If there's a way the game can satisfy its dependencies at runtime, there's no need to have gradle pull them at build time. But I've been assuming that's a "later" problem — that we need to be able to get dependencies resolving consistently when we have the advantages of a full dev environment before we worry about how our own package manager works.

Current momentum is to keep going down this "leave the dependencies to Gradle; that's what it's good at" course, but if you've been down this road before and think we are racing to our DOOM then we need to back up to before #4245 and course-correct.

[Cervator]

In-game module downloader is at least semi-broken, yeah. Another similar thing is the server auto-downloader when a client is missing modules that the server is running. Which might also be semi-broken? Or I just keep bumping into quirky situations where the engine tries to download a different version of itself.

It may be less about the game at runtime and more about how running from a source workspace can be quirky. Normal running from a zip will be fine.

Really, we could magic-wand-fix this by telling developers "thou shalt use groovyw module recurse" and put big fat warnings on the get and init variants to say they are not responsible for dealing smoothly with dependencies.

Or we just go this way for source workspaces (Gradle manages all dependencies, don't do any jars in modules/) then let runtime from zip deal with pure jar files there.

I dunno. My head hurts.

[keturn]

I've made some progress on the distribution-related stuff. Just need to check in with the Launcher code and see what it expects to find in distributions. At this point I'm still hoping to not require any Launcher changes.

[keturn]

It looks like launcher doesn't care very much about the structure of the distribution.

GameStarter is what has the info about where to find the main class: https://github.com/MovingBlocks/TerasologyLauncher/blob/59f8a31d552bec05958d26f484bec9a418762b6a/src/main/java/org/terasology/launcher/game/GameStarter.java#L52-L53

It expects libs/Terasology.jar. (It uses -jar rather than a class name.)

It doesn't use the startup scripts, and it doesn't need the distribution to contain a file with version metadata.

I should probably also look at whatever thing it is that grabs the distForLauncher and combines it with a set of modules to make the "omega" package. 🤔

[Cervator]

I should probably also look at whatever thing it is that grabs the distForLauncher and combines it with a set of modules to make the "omega" package

That one is a non-issue. The script lives at https://github.com/Terasology/Index/blob/master/distros/distros.gradle and just reads in a flat list of modules from https://github.com/Terasology/Index/blob/master/distros/omega/gradle.properties. It uses Gradle to fetch the latest versions out of Artifactory ... which isn't ideal and takes a while. There might just be some permalink for "latest" we could use instead, or ideally we'd move on to that book-of-materials approach, and probably avoid Gradle entirely as we aren't really dealing with dependencies at that point.

It even explicitly cuts out transitive dependencies, I dunno what it might do it we forget to add a new dependency to the prop file - would Gradle find it and surprise us later when we realize the prop file doesn't match what actually ships? I'm pretty sure I just did that originally because it was convenient, not correct or particularly fast :-)

We still could run some reports and check, but really cramming some jar files into a zip doesn't seem like a thing that requires Gradle.

The use or non-use of the version metadata file for the launcher is probably just an option (I raised it on chat some time ago) and I guess it isn't used yet and might never be. Even if then I figure we could just deal with a semi-empty version file in logic rather than include / not-include it like some sort of sign. Simple is good and all that 👍

[keturn]

https://github.com/Terasology/Index/blob/7ade39080a194834d0cb97bb3877bf8d1a2d77d1/distros/distros.gradle#L67-L68

I guess this does expect the .zip distribution to be exactly Terasology.zip, not Terasology-4.2.0.zip or any other variant.

It starts with that, and then dumps more stuff in to modules/ and maybe libs/.

Okay, I think those are the details I was checking for.

[Cervator]

I guess this does expect the .zip distribution to be exactly Terasology.zip

It does, but honestly that's just a shortcut that served the need at the time. I think some zip file naming prop snuck into the distro prop files already, but it may not be in use everywhere - we should be able to just start using something like that and not hard code the name anywhere

[keturn]

other significant detail:

gradle's default distribution-builder puts everything inside a containing folder with the name of the project so things are like Terasology-4.2.0/libs/foo.jar, whereas our current dist tasks put libs and everything else at the top level of the archive.

Personally, I like having the containing folder so unzipping an archive doesn't barf out a bunch of files in to your current directory, but I think that's out-of-scope for this ticket because it would require coordination with Index and Launcher.

[keturn]

I've tried to collect the notes we made here about what we expect from build results and classpaths and whatnot on https://github.com/MovingBlocks/Terasology/wiki/Supported-Development-Workflows

[keturn]

Now with tests!

How should we have CI run these tests? Unfortunately they're not nicely packaged tests that will result in a junit.xml file, the tasks just fail if there's a problem.

Make the general test task depend on testDist, or change Jenkinsfile to add a testDist step?

They do take a bit to run, because they involve zipping up everything (including dependencies), which is why I'm a little hesitant to add them to test.

[Cervator]

I left my big review at #4454 (review) which I figure to also include this one - I'd just merge the other one to get it all in one go 👍

[keturn]

Oh, I recommend merging this separately from that.The grade 6.8 upgrade and the reimplementation of distForLauncher with its change to .bat for the Windows distribution is more than sufficient for one PR, and don't need to wait for finishing up the module dependency stuff.

but I'll look at what that review has to say and see how much of it applies to this

[keturn]

From @Cervator's review:

Finally and another functional one - out of curiosity: with the new Terasology.bat being generated, does it include setting default memory stats like the old one did?

Nope, there's an open note about that here. It doesn't set -Xmx and the JVM goes with whatever its heart wants or something.

That is one functional difference, but since the hard-coded 1.5 GB wasn't exactly a one-size-fits-all value, I didn't know if it was worth carrying over to this version.