Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make runtime multiplatform compatible #168

Merged

Conversation

andrewparmet
Copy link
Collaborator

@andrewparmet andrewparmet commented May 4, 2022

To do:

  • Make protokt-core-lite and protokt-core multiplatform. Requires figuring out how to separate the protobuf-gradle-plugin from Kotlin JVM/Java so that we can invoke local protokt on protokt-core-lite and protokt-core while using Kotlin multiplatform. Looks like pb-and-k simply ditched the protobuf-gradle-plugin. All sorts of problems come up right now, e.g. not resolving the api configuration.
    • I got this working with this fork of the protobuf-gradle-plugin. Definitely a hack.
    • There's an open issue for supporting Kotlin multiplatform but it looks like it's not in active development.
    • I got this to work with the vanilla plugin by reflectively adding the multiplatform and JS plugins to the list of prerequisite plugins and doing the rest of the configuration locally using public APIs.
  • Rework publication configuration to account for multiplatform projects.
  • Fix Android compatibility enforcement for multiplatform projects.
  • Which extensions projects can be made multiplatform?
    • Just the API.
  • Make proto-google-common-protos multiplatform (the non-gRPC parts).
  • (Optional): protokt-gradle-plugin complains about language level 1.4 even though we don't specify it anywhere.
  • AutoService can't be used for the well-known types converters
  • JS types can't implement function interfaces. Adding a capitalized function with the same name as the class works. except that mysteriously the compiler sometimes doesn't resolve calls to the function and instead thinks we're calling the private constructor. https://youtrack.jetbrains.com/issue/KT-52492/Inconsistent-overload-resolution-between-function-and-constructo fixed
    • Used lower case letters for now. Even with lower case functions the compiler sometimes can't resolve them or throws NoSuchMethodError - makes me think there's a problem in the Gradle config or something having to do with top level functions. Capital letter functions could possibly work with a strategy like this:
@KtGeneratedMessage("google.protobuf.Duration")
class Duration(dsl: DurationDsl.() -> Unit) : AbstractKtMessage() {
    val seconds: Long
    val nanos: Int
    val unknownFields: UnknownFieldSet

    init {
        val built = DurationDsl().apply(dsl)
        seconds = built.seconds
        nanos = built.nanos
        unknownFields = built.unknownFields
    }
}

Exposing a constructor like this probably breaks easy Jackson integration, but we should probably discourage that anyways.

  • Get rid of Tag and Numbers classes.
  • Get a JS implementation working (conformance tests as well).
  • Get protobuf-gradle-plugin working with Kotlin JS plugin and add a JS integration test project.
  • Extensions configurations are not quite right - figure out why protokt.proto is not propagating through extract-include-protos properly.
  • Remove support for the Kotlin JS plugin and find another way to determine generation of gRPC code for JS projects. Perhaps multiple plugin invocations - once per source set.
  • Upgrade protobuf past 3.24

Unrelated proposed changes for 1.0:

  • Bump Kotlin to 1.8.21 minimum (MPP plugin compatibility for IR compiler seems to vary across Kotlin versions).
  • Change extension wrapper API to work on Sequence instead of cloning ByteArray. Use read-only views of Bytes directly.
  • Default package name convention changes. (Consult grpc-kotlin team for possible fixes to integration with their generated code: Package name alteration grpc/grpc-kotlin#389)

@andrewparmet andrewparmet force-pushed the make-runtime-multiplatform-compatible branch from c780ec7 to ffe7efd Compare October 1, 2022 04:43
.circleci/config.yml Outdated Show resolved Hide resolved
buildSrc/build.gradle.kts Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/DependencyUtils.kt Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/JavaBasedProjectConventions.kt Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/protokt/v1/gradle/ProtoktBuild.kt Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/protokt/v1/gradle/ProtoktBuild.kt Outdated Show resolved Hide resolved
@andrewparmet andrewparmet marked this pull request as ready for review September 14, 2023 01:08
@andrewparmet andrewparmet merged commit 3013531 into open-toast:main Sep 28, 2023
@andrewparmet andrewparmet deleted the make-runtime-multiplatform-compatible branch September 28, 2023 17:55
ogolberg pushed a commit to ogolberg/protokt that referenced this pull request Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants