chore[facade]: use picocli for processing command line options

[keturn]

This PR switches PC facade's handling of command-line options to use picocli.

The hope is that it improves input validation, provides better feedback to users, and makes it easier to implement new command-line options.

How to test

test out all the command line options!

Outstanding before merging

• What do we do about Launcher? Support POSIX-style command line options TerasologyLauncher#589
• re-add extended help message with examples
• update https://github.com/MovingBlocks/Terasology/blob/develop/docs/Playing.md
• make sure --homedir --no-crash-report isn't a problem (it was using '--no-crash-report` as the value of homedir!)
  • filed upstream as required parameter does not exclude negated options remkop/picocli#1197
  • local workaround in place: fail if homedir starts with -

[keturn]

I ended up moving everything currently in Terasology (the class with the static main method) to a new class (TerasologyCommand), but it looks like I didn't have to do that. I could I have left that main as static but switched everything else to non-static.

Those aren't two useful classes, it just moved stuff around and left a one-line static method in the other class, so I think I'll shift it back so it's a smaller change overall.

[that's a recap from discord, adding it here as a comment as I haven't got to actually do it yet.]

[skaldarnar]

This is then added via the picocli library, isn't it? I don't think we need to copy word-by-word, but just make sure that users can figure out what options/flags are available.

[skaldarnar]

Does the Callabale move this to a different thread? Will this cause even more issues with porting to LWJGL 3 and MacOS (see #3969 )?

[keturn]

I don't think it runs it in another thread. I think picocli just uses Callable as an interface that's easy to define for use with its .execute(it) method, so it can run it, handle exceptions, and get a process return code at the end. But it'll be a good thing to double-check.

[keturn]

I confirmed this. currentThread() returns the same value inside the call method as it does in the static main.

[keturn]

It's working in IntelliJ IDEA but the build-with-command-line-gradle fails. 😞

:facades:PC:game FAILED
Error: Unable to initialize main class org.terasology.engine.Terasology
Caused by: java.lang.NoClassDefFoundError: picocli/CommandLine$Model$ArgSpec

changing the picocli dependency type to implementation or compileOnly doesn't help.

Not sure what the deal is. Is it related to the picolci-codegen or annotation processing or something?

[keturn]

It's something in the difference between the game and run tasks in :facades:PC. run is working better.

[keturn]

Oh heck. I just realized that if we change the way command-line options are spelled, Launcher needs to know.

We could add aliases for the old spellings. It wouldn't be difficult.

Thing is, I like the idea of following the POSIX conventions for these, and in that convention prefixing options with a single dash means you're using the short-form single-letter options. That's a potential source of confusion I'd like to avoid.

I'd prefer to make this explicitly-breaking-change instead, get it over with.

But then what does Launcher do?

Is it willing to break compatibility with previous releases just as quickly, or do we need to teach it to use old-style options if the version number is below a cutoff point?

[pollend]

At the moment the whole engine is going through a large split and change so I wouldn't be too worried about breaking API changes if it makes things work better. I also don't want to have stuff be bagged down by older changes for the sake of consistency since I don't think we have any obligations to keep things consistent.

for backwards compatibility just use a version check so the launcher can run older copies of the game.

[keturn]

The blocker to getting this merged is the game task: its classpath is missing something, something the run task isn't missing.

And if we're working on the game task and its variants, it seems like a good time to address the code duplication going on between them.

For that, we could use some documentation on our requirements for those tasks.

• Run Game (start a process that brings up the game window etc etc) is the obvious one.
• Build all classes and assets for engine and facade should be a dependency, I don't think there's any disagreement about that.

I think the part where questions come up is around modules. Currently game adds these dependencies:

    // Dependencies: natives + all modules & the PC facade itself (which will trigger the engine)
    dependsOn(":extractNatives")
    dependsOn(":moduleClasses")
    dependsOn("classes")

Where :moduleClasses is a task of the root project that depends on the classes tasks of everything under /modules/*.

And game has this to say about its classpath:

    // Classpath: PC itself, engine classes, engine dependencies.
    //     Not modules or natives since the engine finds those.
    classpath(sourceSets["main"].output.classesDirs)
    classpath(sourceSets["main"].output.resourcesDir)
    classpath(project(":engine").sourceSets["main"].output.classesDirs)
    classpath(project(":engine").sourceSets["main"].output.resourcesDir)
    classpath(project(":engine").configurations.runtimeClasspath)

from reading that, I expect:

• game depends on compiling the .class files of all modules/*
  • but not their .jars
  • a compilation failure of any module will be treated as a failure to provide a dependency of the task, and game will not launch
• the classpath does not include anything from /modules (neither /modules itself nor /modules/**/build/**)
• the classpath does include *.class files and resources from from /engine/build

[continued…]

[keturn]

Follow-up questions to earlier:

We habitually tell people to run gradlew jar game. The game task specifically depends on building the .class files, not the .jars. Does the fact that we tell people to build the jars mean:

• that the game task should really have a jar dependency?
• or that the jar was left out on purpose because gestalt module loader should be able to work with the classes alone, but we've broken module loading somehow so it doesn't?

There's a category of module development workflow requirements that I'd like to have spelled out, not only for this but for any time we make changes to the gradle + IntelliJ build process.

Things like:

• after you make changes to the source, does it rebuild everything you expect it to? (and what are those things?)
• when is the reflections cache built and used, and when is it not?
• which things can be reloaded at runtime with gestalt? a module's assets? its classes? new classes?
• do the debugger's breakpoints work?
• does the IDE + debugger's class-reloading work?

[DarkWeird]

Gradle jar game - it is old behaviour (before my changes to build.gradle) it is seems was related with build dir or reflections.cache. dont remember

Gestalt-module works fine with classes and jars(idk which priorities)

If you change source - then rebuilding only changef module and it's dependents.
If we make api module for apis then recompile only implement module(gradle 5+ used in android builds mostly)

Reflections - builds at compile time and used by gestalt-module. If reflections.cache not found - then it gathering at runtime.
Current behavior: reflection gather before jar and cleanup after jar.. then reflections on source modules gathering at runtime.

Better: gather reflection when classes(or compileJava) task done and uptodate it when classes uptodate.

Reloading:
Only few assets type possible to reload(see code) (we have special method invoke on tick somewhere) (gestaltv7 have special manager with reloading)
You cannot reload classes during debugging only method's body changes (see jvm debug reloading limitation) (but you can use some thing like DCEVM or JRebel)

Debugging with gradle works fine.

[keturn]

classes

Do we keep the the modules/**/build/** directories off the runtime classpath only because it makes things tidier (it would make the classpath a lot longer), or also because including them there would break module loading or the security model or something?

reloading

On asset reloading: There's this gestalt demo video showing changing textures. Is it sufficient to just write to their source locations, without having jar or syncAssets tasks rerun?

I went looking for docs on asset reloading. I found a brief note about it here: https://github.com/MovingBlocks/gestalt/wiki/Gestalt-Asset-Core-Quick-Start#reload-assets-changed-on-disk
though I'm having trouble finding a class with that name used by Terasology. I think that might be its gestalt v7 name.

the invocation to reload on tick is in TerasologyEngine.tick

reflections

Reflections - builds at compile time and used by gestalt-module. If reflections.cache not found - then it gathering at runtime.
Current behavior: reflection gather before jar and cleanup after jar.. then reflections on source modules gathering at runtime.
Better: gather reflection when classes(or compileJava) task done and uptodate it when classes uptodate.

That “better” option is one of those things where I expect we could do it for just gradle, but I'm worried about getting consistent behavior when mixed with IntelliJ IDEA's compilation process.

Scope

A lot of these notes are only very loosely related to this picocli PR, but they are related in that we're specifying the things that need to keep working when we tinker with these JavaExec tasks and their classpaths.

I'll want to organize this information and put it someplace. A page on MovingBlocks/Terasology/wiki I guess?

[DarkWeird]

Classes

We cannot use gradles build dir definition (build/classes/main) and not copy resources in classes dir because gestalt-module/gestalt-assets use only one dir from source sets.

Reflections

I hope we can drop reflections and use Annotation Processing, when gestalt DI will done. Then idea and gradle processes will be consistent

Scope

Terasology - everytime code research :D

Yeah, plz do it!

[keturn]

We cannot use gradles build dir definition (build/classes/main) and not copy resources in classes dir because gestalt-module/gestalt-assets use only one dir from source sets.

then how did that asset-reloading demo work? was the editor saving to the classes directory? or was there a auto-refreshing copy-resources-to-classes task running in the background? maybe it's worth pinging @immortius for info about that demo.

[DarkWeird]

Copying resources in classes build directory.

Gestalt-module can load only one directory as source module.

Normal gradle build directories include separate direcories for resources and source sets.

[Cervator]

I still haven't had time to dig into this, although I did just bump into the run vs game tasks, wondering why we still had two - thought one was renamed into the other years ago. Sooo ... I took it out (#4216). Looked like game was more complete anyway. Are we still missing something or is that unique to this rework?

gradlew jar game is a mix of habit and speed (just three characters woo!). I don't know if it has ever been required (although reflections being involved in the past rings a bell - but only coincidentally). It really should just be gradlew game but ... gremlins and superstitions

[keturn]

still haven't had time to dig into this, although I did just bump into the run vs game tasks, wondering why we still had two - thought one was renamed into the other years ago.

Ah, that might well be true about the renaming. I (re)introduced the run task while I was doing the big gradle upgrade (for 6.4 & .idea, probably?). That was one on the areas where I started down that path of these changes to facades/PC/build.gradle and then realized it was biting off more than I could chew in that PR.

Looked like game was more complete anyway.

In what sense? It might have been overlooked among all the comments posted to this issue, but note here that the question that started all that discussion is that the run tasks works after I add the picocli dependency and the game task doesn't.

If you're able to describe what game provides that is lacking from run, that would make the specification I was looking for to be able to work on these tasks without breaking someone's workflow.

It really should just be gradlew game but ... gremlins and superstitions

I am against keeping gremlins in source code. You can make a module for gremlins in-game if you want. 😉

As for superstitions, please document them here or in their own issues as appropriate. Then we can do some mythbusters on 'em. 🚫 👻

[keturn]

Yes, we won't be able to launch releases containing these changes from the launcher […] Doing this change should be fairly simple, I can prepare that to have it ready +1

@skaldarnar, have you started a Launcher branch for this somewhere?

[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]

That was a potentially messy merge for PC/build.gradle.kts but I think it ended up okay 🤞

[skaldarnar]

I did not forget about this ... I swear! Made a note to update the launcher sometime in the not-so-far future.

[keturn]

There's been a new picocli release in the meantime.

One thing that might be of interest is picocli using Optional.

[keturn]

picocli / POSIX-style command line

[skaldarnar]

Why is RegisterSystem not needed (anymore)? Is the class considered used in any case?

[keturn]

I was re-checking these at some point and removing the class-level annotation didn't result in any inspection warning popping up. 🤷

[keturn]

Launcher supports this now, but I added a new bug in the process. So this is waiting on MovingBlocks/TerasologyLauncher#651 in order for Launcher to be releasable.