iseki0 / kotlinx-serialization-bencoding

A Kotlin serialization codec used to encode/decode bencoding format.
Apache License 2.0
4 stars 0 forks source link

Unresolved class `Bencode` in Android-JVM MPP #2

Closed Him188 closed 4 months ago

Him188 commented 4 months ago

image

Targets:

kotlin {
    androidTarget()
    jvm("desktop")
}

image

iseki0 commented 4 months ago

😥Sorry. The project is currently under development, do you need the decoder only? I will complete it soon. Regarding the unresolved reference, there's an incompatible API change. The old version used InputStream.decodeInBencoding<T>, which doesn't follow the BinaryFormat type in the kotlinx-serialization-core. In the next version I will update it to match the README. @Him188

iseki0 commented 4 months ago

The encoder part is currently under development in another repo, after I complete the unit test it will be merged. Before that if you need the fully tested decoder API, let me know, I will make a release. (The decoder development was completed but not been released yet.) @Him188 Both encoder and decoder will be supported in JS target.

Him188 commented 4 months ago

No worries at all @iseki0, thanks for your work.

I turned to another Bencode decoder written in Java. Since both desktop and Android targets support Java-interop, this is not a problem.

However that library does not support descriptors like in kotlinx-serialization, so I have to do a lot of unsafe casts and the code looks ugly...

Here is the relevant code: https://github.com/open-ani/ani/blob/043a346f06fae777cf846556ff2c15f5157063c2/torrent/impl/qbittorrent/src/QBittorrentTorrentDownloader.kt#L104

I'm happy to move to your library when it gets stable! :) My app is being used by many people so stability is the most important thing in my case.

Him188 commented 4 months ago

I also authorize Kotlin multiplatform libraries like mirai, so I'm a bit familiar with the multiplatform build logic.

I looked through your buildscript. It looks like you are requiring JVM target 17, which sounds too high for a library. Officially the minimum JVM target is 8 and most of the libraries support 8 so that they can work on all projects. Do you need any specific features from JDK 17? If not, I think we should set it to 8. Also we can remove withJava() here?

    jvmToolchain(22)
    jvm {
        @OptIn(ExperimentalKotlinGradlePluginApi::class) compilerOptions {
            jvmTarget = JvmTarget.JVM_17
            freeCompilerArgs.add("-Xjvm-default=all")
        }
        withJava()
    }

Maybe also setting jvmToolchain to lower? Not sure if this gonna affect artifect metadata since Kotlin has got a new metadata layout for MPP.

These changes might help solve the original dependency resolution problem.

My project uses 17 because PreCompose requires 17 however I don't see a reason for using 17. Switching the JDK version in my project which has a lot of modules has been a great pain.

However, I also suspect It might be a Kotlin bug: https://youtrack.jetbrains.com/issue/KT-65362/Cannot-resolve-declarations-from-a-dependency-when-there-are-multiple-JVM-only-project-dependencies-in-a-JVM-Android-MPP

iseki0 commented 4 months ago

@Him188 Thank you for your advice. I set the toolchain to Java 22 just because I want to test it in the GraalVM Native Image environment in the old days. Now I will consider to reduce the toolchain version. (I don't think it's required now.) I configure the target to 17 version just because I want to give supports to JPMS(Jigsaw), even I don't like it. Since the most Android application require the target 8, your advice is very valuable. I will reduce the target version to 8. And the module-info.class could be added by multi-release jar, just like the many libraries chosen.

iseki0 commented 4 months ago

@Him188 I release a new version. https://github.com/iseki0/kotlinx-serialization-bencoding/releases/tag/v0.2.0 😁