Fix/transient deps

[keturn]

This is meant to address the problem described in #4014: unresolved module dependencies when they're not all present as source.

How to Test

See #4014 for notes on what kind of brokenness we were seeing. The basic test plan is:

• Start with nothing in modules/ (neither sources nor jars)
• groovyw module get JoshariasSurvival without using recurse
• start a game and make a new JoshariasSurvival world
  • from gradlew game
  • from the IDE TerasologyPC run config

You might also test other modules or game modes with deep dependency trees.

Outstanding before merging

• this branch is based on build: save build time by not checking jcenter for terasology dependencies #4342; merge it or rebase this branch. (The fix doesn't depend on that, but it does speed up testing a bit.)
• does it work? (right now I have it working with JoshariasSurvival and CoreSampleGameplay in modules/, does it work with JS alone and no CSG?)
• why do I have both Workstation-1.1.0.jar and Workstation-1.1.0-SNAPSHOT.jar?
• does it mess up the distribution tasks?

to do later (out of scope for bugfix)

• should we do something similar with the :moduleJars task?
• can we get rid of RemoteModuleGatherer???

[keturn]

I'm submitting this a promising proposal, but as a Draft because I don't think I've figured out all the ramifications yet.

That requirements document on the build process that I was taking notes for over in #4157 would come in handy here.

[keturn]

ok, it works if I only have JoshariasSurvival checked out without CoreSampleGameplay.

It seems to work with gradlew and intellij, though intellij doesn't trigger RemoteModuleGatherer. That suggests we could get rid of RemoteModuleGatherer, though I still have to see whether its involved in building the distributions.

and I only have the one version of Workstation now, so I guess whatever that was went away with last night's module rebuilds?

[keturn]

I see several distribution tasks under :facades:PC

• distApp: says it does not include modules
• distModules: depends on distApp as well as :moduleJars. "only grabs Core in Jenkins but locally will grab any present. Distros now handle Jenkins packs"
• distPCZip: depends on distModules
• distForLauncher: includes distPCZip (I think)

Questions I have (probably for @Cervator) are:

• which of these tasks are the ones we use for releases?
• do we actually distribute modules this way, or is that done elsewhere? ("Distros"?)

[keturn]

there are also gradle's default distribution tasks, distZip and distTar.

[keturn]

I'm very tempted to go on a tangent changing some of that, but the scope of this ticket is just making sure the current PR doesn't break our releases.

It's okay if that leaves things in sort of a half-and-half state, we can revisit distribution tasks in a separate PR.

[Cervator]

So, lots of potentially good stuff 👍

moduleClasses - if we can get rid of this one (and the jar variant) then awesome! Those two were ancient hacks, and I think once upon a time they helped make sure modules were built along with everything else, especially in cases where you were trying to dist stuff. They at least partially broke a while ago and I have no idea if they're still needed. Cool to see a possible replacement strategy.

RemoteModuleGatherer

I don't know or remember anymore what that even does. After we extracted the final module from the engine repo I think the need for a lot of stuff went away (and some comments went out of date, Jenkins doesn't build any modules as part of the engine build anymore). So long as modules can build locally and you can potentially make a local game zip with modules then cool.

I say "potentially" because Jenkins doesn't even use that approach anymore - since no module activity happens during the engine build at all. Module bundling happens 100% in the Omega job instead, and even that should perhaps be replaced with something more simple. It literally just grabs a list of modules at this point and it seems senseless to even have Gradle try to dependency resolve that since we literally put the full list of modules into the property, no dependencies should remain (unless we missed a spot)

Should the engine workspace even be able to build a zip with modules, or start a server from source with modules? I'm not even sure. Especially if we refactor the module packager, make it a lib, then enable it to be embedded in a workspace so its utility becomes available there without module clutter in the engine Gradle.

Main thing engine really needs to be able to do anymore is make sure you've got module jar files for dependencies you don't have in source form. And fetching module source but that's in groovyw now

and I only have the one version of Workstation now, so I guess whatever that was went away with last night's module rebuilds?

Probably yes. Module binaries should be cleaner now.

Dist tasks

Only distForLauncher really matters anymore for Jenkins. The others are in that twilight zone of "do we even need to do this locally, if Jenkins makes game zips using a completely different approach anyway?"

Although of course they do indeed chain together... I could live without distModules personally.

Long ago we went from using distZip to our own custom task but I don't remember why anymore. I've been curious about some of the newer things like full on application or platform definitions.

[Cervator]

Haven't tested, but it looks good and I like where this is going 👍

The build file under modules/ makes me curious about any unexpected impact, but that's more a mild spidey sense than any real concern.

[keturn]

RemoteModuleGatherer is the thing that spies on gradle's dependency downloads and puts jars in /modules/.

but with those questions answered, it sounds like the stuff this PR has done with module dependencies won't break releases, because it's not part of that process. In that case I think I can take this out of Draft.

[Cervator]

With #4367 submitted I'm more comfortable that we have more clues to rediscover if any quirks come up, particularly around the in-game module downloader. No objection to merging this right now, or in a few hours, just leaving some minor comments about naming/docs :-)

[Cervator]

Would it be worth adding a quick comment here? I'm not even sure what "platform" even means here, although I can figure what the overall meaning is

[keturn]

can do

[Cervator]

Similarly here I fully expect that this works, but is allowing dependencies not a default state of some sort? That seems odd.

[keturn]

That is the case! By default, gradle's java-platform is a thing that specifies version constraints, but consumers can depend on the platform without depending on everything in it.

that scenario probably looks more like:

dependencies {
    implementation(platform("com.example.foo:foo-framework:3"))
    implementation("com.example.foo:foo-core")
    implementation("com.example.foo:foo-http-server")
}

to say "I need these things from Foo Framework v3, make sure all versions are consistent with what they've published as v3 of that platform."

In order to say this platform depends on things we have to explicitly enable set this flag.

[Cervator]

I figure this builds the runtime classpath so all the module jars are available to the engine. this can be used to refer to a whole project level in Gradle, so to say?

This reminds me of that old idea to make the PC facade have a compile dependency on all the modules, to have them for sure get built when you run the game. Although at the cost of not being able to launch without every single module working, and maybe at a risk to get weird dependencies written into the binaries published to Artifactory.

[keturn]

this is given by the closest enclosing scope; it's the project that subprojects is iterating through. (the hints added by IntelliJ make this clearer when reading it in the IDE)

make the PC facade have a compile dependency on all the modules

Yes, that's exactly what is happening when the facade depends on the platform.

at the cost of not being able to launch without every single module working

I think we already sacrificed that ability a while ago?

Test plan:

• groovyw recurse JoshariasSurvival
• add some compilation error to EdibileFlora's FillingModifierSystem
• gradlew game --continue
  • :modules:EdibleFlora:compileJava fails

I think because the game task depends on the moduleClasses task.

I haven't yet come across a gradle way to always attempt to rebuild dependency subprojects if their sources changed but also soft-fail and try to run subsequent steps despite that. You can --continue, but I think that only makes it keep going on independent parts of the dependency tree, not excuse a missing dependency.

[keturn]

get weird dependencies written into the binaries published to Artifactory.

yes, if you publish a jar of the PC facade while you have subprojects present in /modules/, that would include things we probably don't want included.

Is that a thing that happens? Does anything publish facade that's not in a clean workspace made explicitly for that purpose?

If so, then yeah, we'll have to add more separation so this local workspace state isn't confused with what we want to publish to the world. I hadn't imagined it was used that way.

[Cervator]

Gradle will always hit the issues, yep, IntelliJ has had a tendency in the past to skip errors if they're in something the run config doesn't care about. I'm not sure about that now since we're having some other weird issues there from time to time :-)

[Cervator]

if you publish a jar of the PC facade ...

I didn't see this comment earlier while posting my own for some reason. No, that isn't done anywhere at the moment. And any sort of test-publishing we might do locally surely should target something like the Nanoware test repos in Artifactory.

It is good to know though, in case we get creative and weird in the future. One idea that floated around was to build modules in an actual full engine workspace, rather than the minified single-module workspace we run in Jenkins right now. Even that in theory shouldn't hit it - since that should just publish the module, not the facade.

So - not worried about it. May be worth highlighting with a comment perhaps :-)

[Cervator]

Again understanding what this does, but cleanPlatform throws me off naming-wise. Is it almost like "cleanPlatform" would really be the task name? But since you want :modules:clean to work then the task name just remains "clean"

Would something like cleanAllModules and cleanAllModules.dependsOn(... do the same thing but be clearer in naming?

[keturn]

cleanPlatform here is a local variable so I can use it below after this has been shadowed by the subproject. It's not renaming any tasks.

[Cervator]

Oh yes, I got that part. Thus the "almost like" 😁

Like in the abstract this class could as well have been called "cleanPlatform" or "cleanAllModules" - plain "cleanPlatform" just throws me off as a variable name the way it gets used, but that's very minor and probably just me 👍