pb-and-k icon indicating copy to clipboard operation
pb-and-k copied to clipboard

pure kotlin dependency-free Marshaller/Unmarshaller and native

Open phcoder opened this issue 5 years ago • 7 comments

This provides native variants for all the components.

Code is adapted from java protobuf code.

phcoder avatar Jan 27 '19 07:01 phcoder

That's a pretty significant level of effort, thanks. Notes:

  1. I think at a higher level saying you referenced the Google protobuf lib Java reference impl is reasonable over embedding the copyright headers.
  2. I don't think we should remove the common Marshaller, Unmarshaller, and Sizer. I don't want the "pure" impl to take over the other ones, just be an option instead of them. Also, I would like backwards compatibility (at least mostly).
  3. Same with Util...some platforms have faster ways of doing this, I don't want to force the pure Kotlin one on those platforms.
  4. Do you mind if instead of accepting this PR wholesale, if I work some of it into a new PR that takes a lighter approach? I will gladly give you credit in the README for the initial impl. I would essentially make a runtime/runtime-pure cross platform project, a runtime/runtime-native project that uses it, then conformance and protoc-gen-kotlin set of native versions. After that, maybe even runtime/runtime-pure-js and runtime-pure-jvm if Kotlin MPP doesn't support using a common-only project directly.

cretz avatar Jan 28 '19 10:01 cretz

  • I think it's reasonable to have Marshaller/Unmarshaller/Sizer/Util to be interfaces with actual implementation determined at runtime (or compile-time).
  • For compatibility: The problem I have experienced is replicating the constructor of (Un)Marshaller from Input/OutputStream but making it a function instead of constructor keeps source compatibility fully without placing restrictions on class structure
  • For Util: we probably need few tests to ensure correct handling of incorrect UTF-8 and UTF-16 sequences to ensure that all implementations behave the same
  • I'm fine with you changing this PR
  • Copyright for files I contribute needs to be marked with "Copyright 2019 Google" and protobuf is "Copyright 2008 Google". Copying headers was the easiest way to achieve this but I'm fine with other approaches like e.g. refrencing license by name instead of by copy

phcoder avatar Jan 28 '19 14:01 phcoder

Thanks for the response. I have to admit I don't have the immediate time to incorporate this but hopefully I can revisit it soon.

cretz avatar Jan 28 '19 15:01 cretz

If you want I can do the changes you requested and update this PR

phcoder avatar Jan 28 '19 17:01 phcoder

I added some flexibility and changed it to use JS/JVM tools on those platforms. By looking through protobufjs it looks like it implements same things as kotlin-pure code and doesn't get advantage of any js functions. So it feels like protobufjs doesn't provide any value. Should we make js and native both use kotlin pure?

phcoder avatar Jan 28 '19 20:01 phcoder

Writing this to show my support for this feature! 🙌

LiewJunTung avatar Mar 13 '19 05:03 LiewJunTung

@LiewJunTung and @phcoder - Sorry I haven't done more here, I haven't been using Kotlin as much lately in my day job. In general I would take this and make a set of "pure" projects to not disturb the existing projects. Can't make any promises when I can revisit though, sorry.

cretz avatar Mar 13 '19 14:03 cretz