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

Add native kotlin support #337

Merged
merged 11 commits into from
Feb 3, 2022
Merged

Conversation

vincentfree
Copy link
Contributor

Kotlin support has been added to protobuf since version v3.17.0.
The generated kotlin files have kotlin dal support as an added bonus over direct usage of the java versions of the protobuf .java files

Kotlin support has been added to protobuf since version v3.17.0. 
The generated kotlin files have kotlin dal support as an added bonus over direct usage of the java versions of the protobuf .java files
@vincentfree vincentfree requested a review from a team October 4, 2021 15:13
Makefile Outdated Show resolved Hide resolved
@vincentfree
Copy link
Contributor Author

I have also updated the otel/build-protobuf image from version 0.4.0 to 0.7.0

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

The new github action failed

@vincentfree
Copy link
Contributor Author

The new github action failed

I will have to update the otel/build-protobuf image before this will pass since the current version of protobuf is quite old and does not support kotlin. I had a PR open for this so I'll fix those issues and hope It will make version 0.9.0. Afterwards this will also pass.

So this PR is dependend on: open-telemetry/build-tools#76

@vincentfree
Copy link
Contributor Author

vincentfree commented Jan 7, 2022

I have fixed the issue with the otel image locally. The builds are running and I hope this new functionality can be published soon so this PR can be finished and closed.

I've tested with the local build and got the compiled kotlin files, snippet:

kotlin/io/opentelemetry/proto/trace/v1:
total 100K
-rw-r--r-- 1 user root 2.0K Jan  8 09:13 ConstantSamplerKt.kt
-rw-r--r-- 1 user root 7.4K Jan  8 09:13 InstrumentationLibrarySpansKt.kt
-rw-r--r-- 1 user root 2.0K Jan  8 09:13 RateLimitingSamplerKt.kt
-rw-r--r-- 1 user root 8.1K Jan  8 09:13 ResourceSpansKt.kt
-rw-r--r-- 1 user root  45K Jan  8 09:13 SpanKt.kt
-rw-r--r-- 1 user root 2.4K Jan  8 09:13 StatusKt.kt
-rw-r--r-- 1 user root 7.6K Jan  8 09:13 TraceConfigKt.kt
-rw-r--r-- 1 user root    0 Jan  8 09:13 TraceConfigProtoKt.kt
-rw-r--r-- 1 user root 2.1K Jan  8 09:13 TraceIdRatioBasedKt.kt
-rw-r--r-- 1 user root    0 Jan  8 09:13 TraceProtoKt.kt
-rw-r--r-- 1 user root 7.1K Jan  8 09:13 TracesDataKt.kt

@bogdandrutu
Copy link
Member

@vincentfree dependency merged, what else can I help with?

@vincentfree
Copy link
Contributor Author

@bogdandrutu
Thanks, when the image is published then kotlin support should work and this PR can be updated with the new base image.

I've tested it on my laptop so I'm quite sure that this PR is almost done.

Makefile Outdated
gen-kotlin:
rm -rf ./$(PROTO_GEN_KOTLIN_DIR)
mkdir -p ./$(PROTO_GEN_KOTLIN_DIR)
$(foreach file,$(PROTO_FILES),$(call exec-command, $(PROTOC) --java_out=./$(PROTO_GEN_JAVA_DIR) --kotlin_out=./$(PROTO_GEN_KOTLIN_DIR) $(file)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this depend on gen-java instead of re-generating the java files in their dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could but there is a relation between the java and kotlin generated classes. The kotlin file 's refer to those from Java so it might work but I've never seen kotlin generation without the java_out option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why the build failed is because it has a dependency on the newer otel image with my other PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincentfree I understand the build failure, but still not sure about the regenerating of the java files in this rule. I would give a try without to see what happens :D

Copy link
Contributor Author

@vincentfree vincentfree Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't hurt to try though the kotlin generation step would then have a dependency on the gen-java. Still it wouldn't have the overlap of files generated

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest update you need to change gen-openapi as well, no more swagger.

@vincentfree
Copy link
Contributor Author

@bogdandrutu thanks for the new release of otel/build-protobuf:0.9.0
Updated the codebase with the new version, I'm now referencing gen-java in gen-kotlin so proper reuse ;)
And updated the swagger_out to openapiv2_out everything should be fine now.

I've tested gen-kotlin and gen-openapi locally. Both work.

Makefile Outdated Show resolved Hide resolved
@vincentfree
Copy link
Contributor Author

@arminru your review is required to pass this PR. It's fully ready to merge back to main

CHANGELOG.md Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@bogdandrutu bogdandrutu merged commit 270488e into open-telemetry:main Feb 3, 2022
@vincentfree vincentfree deleted the patch-1 branch February 5, 2022 08:12
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.

6 participants