korlibs-archive / korge-next

Moved to https://github.com/korlibs/korge
75 stars 37 forks source link

Deprecate korge-fleks to use Fleks directly from Quillraven's repo. Updated fleks-ecs sample. #718

Closed Kietyo closed 2 years ago

Kietyo commented 2 years ago

The benefit of doing this is that we won't have 2 copies of Fleks MPP to maintain.

Kietyo commented 2 years ago

@jobe-m fyi

jobe-m commented 2 years ago

Awesome! That was the missing piece to use Fleks from upstream in the example. Many thanks :)

LGTM

soywiz commented 2 years ago

Cool!

Is this working for other targets than the JVM? like JS, and all the Kotlin/Native targets KorGE supports? I have checked central: https://search.maven.org/artifact/io.github.quillraven.fleks/Fleks/1.0-KMP-RC1/jar

and the associated module: https://repo1.maven.org/maven2/io/github/quillraven/fleks/Fleks/1.0-KMP-RC1/Fleks-1.0-KMP-RC1.module

And in the module I can only see JVM references. Are the other targets published? Am I missing something?

Maybe we can keep a module that executes tests on all the targets includen Fleks as a dependency and at least calling very basic functionality to be sure the integration works properly, but without publishing that artifact.

Another concern we should keep in mind: What's the plan if we decide to add more targets in KorGE (ie. wasm or linuxArm32Hfp/raspberry pi zero), should we propose quillraven to publish the new targets? Or there are plans to have pure Kotlin Common libraries supported in Kotlin in the short term so we can add new targets freely? My concern is mainly because right now KorGE was only depending on kotlinx.coroutines, and we cannot even support officially raspberry pi zero, because of this: https://github.com/Kotlin/kotlinx.coroutines/issues/855 ; there are unofficial repos with those artifacts but we are only supporting mavenCentral and google here that are likely less prone to security issues once published when using fixed versions.

Another possible approach, would be using korge bundles, that is using source-code dependencies supported by korge, that can download a github repo and load a path inside it and for common code is compatible with all existing and new targets:

Like for example:

korge {
    bundle("https://github.com/korlibs/korge-bundles.git::korge-admob::4ac7fcee689e1b541849cedd1e017016128624b9##2ca2bf24ab19e4618077f57092abfc8c5c8fba50b2797a9c6d0e139cd24d8b35")
    config("ADMOB_APP_ID", "...")
}

It internally downloads https://github.com/korlibs/korge-bundles.git, then checkouts the git commit 4ac7fcee689e1b541849cedd1e017016128624b9 and loads the folder korge-admob. It also hashes all the files with SHA256 2ca2bf24ab19e4618077f57092abfc8c5c8fba50b2797a9c6d0e139cd24d8b35 and checks everything is downloaded as expected. So it is pretty safe that we are using the expected verified version even when using an external git repo in the case a bad party managed to create a git commit collision.

We could even add fleks here in the catalog of sourcecode bundles: https://awesome.korge.org/

And we can also support it with supportFleks. We are already doing it for several modules: https://github.com/korlibs/korge-next/blob/61d5d25b2a486338f913d9c8847850587c599f76/buildSrc/src/main/kotlin/com/soywiz/korge/gradle/KorgeExtension.kt#L449-L460 https://github.com/korlibs/korge-next/blob/61d5d25b2a486338f913d9c8847850587c599f76/buildSrc/src/main/kotlin/com/soywiz/korge/gradle/KorgeExtension.kt#L431-L433

I believe what we are missing is to try to use that external source code in e2e tests to verify they keep working between versions

Thoughts on all these points?

Kietyo commented 2 years ago

What's the plan if we decide to add more targets in KorGE (ie. wasm or linuxArm32Hfp/raspberry pi zero), should we propose quillraven to publish the new targets?

I think Quillraven might appreciate your help in that regard, I'm not that familiar with KMP and neither is he I think.

I did notice now that some native targets broke when attempting to build but the sample runs fine on JVM/JS.

For now I will just import Quillraven's version of Fleks and work on/use that since it's being actively updated. I do not want to take the responsibility of keeping both versions in sync and I'm fine with whatever you / Quillraven / @jobe-m decide.

jobe-m commented 2 years ago

Sorry, for not answering earlier. I am currently on the way into vacation with the caravan and don't have much time and good Internet connection on the way to our final camping site. I will look into your answers later.

jobe-m commented 2 years ago

I like the idea of using the source code bundles for integrating Fleks into Korge. Currently the korge-Fleks module is a copy of the "next" branch in Fleks repo. So the main work of keeping Korge-Fleks in sync with the original JVM version of Fleks is to sync the master and next branches in Fleks repo. This is anyway done in the Fleks repo.

I currently don't know of any code in Korge that depends directly on Fleks. Only the Fleks-ecs example. So I guess it won't be a dependency problem for the core of korge, would it?

I also already noticed that the native target of Fleks does not work. It crashes the game app on start. I guess there is a problem with the inject object and threadlocal or native GC. But this is anyway reworked currently also because of ideas of @Kietyo.

We could keep the korge-fleks module for e2e tests and integration checks.

I converted this PR into a draft and plan to check here the integration of Quillravens latest additions to the next branch in Fleks -> https://github.com/Quillraven/Fleks/pull/50

soywiz commented 2 years ago

BTW: It should be super cool travelling in caravan.

I can try to help Quillraven to get it publishing all the artifacts if he wants the help.

Also I created sometime ago this gradle plugin to use sourcecode dependencies outside korge too: https://github.com/korlibs/kotlin-source-dependency-gradle-plugin