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

feat: protoc-gen-scala support #121

Merged
merged 2 commits into from
Jan 3, 2023
Merged

feat: protoc-gen-scala support #121

merged 2 commits into from
Jan 3, 2023

Conversation

strophy
Copy link
Collaborator

@strophy strophy commented Nov 30, 2022

Initial implementation of protoc-gen-scala. Still need to figure out what Java dependencies are required to run this in the target stage, particularly on arm64.

@strophy strophy mentioned this pull request Nov 30, 2022
@rvolosatovs rvolosatovs added this to the 4.1.0 milestone Nov 30, 2022
Copy link
Owner

@rvolosatovs rvolosatovs left a comment

Choose a reason for hiding this comment

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

What's the status here? Is the arm64 not supported yet? If so, how about we file an issue about it and fix up in a follow-up?

@strophy
Copy link
Collaborator Author

strophy commented Dec 12, 2022

Not ready yet, I need to block out some time for more testing and to get the version strings compatible with Renovate here. I'll test arm64 build at the same time and see if we can support it immediately or later.

@strophy
Copy link
Collaborator Author

strophy commented Dec 21, 2022

I've made progress on this and understand the build process (and the errors I was seeing) much better now. I expect we will be able to complete this PR using a different build method when this fix is merged, it doesn't look like it will take too long.

@strophy strophy marked this pull request as ready for review January 3, 2023 00:14
@strophy
Copy link
Collaborator Author

strophy commented Jan 3, 2023

@rvolosatovs this is finally ready. The builder will not be versioned by Renovate since Scala sbt downloads different compilers as necessary. I ended up getting help from the ScalaPB maintainers and using the original build approach, will ask for help sooner next time 😅

Copy link
Owner

@rvolosatovs rvolosatovs left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but could you drop the merge commit?
Simply squashing all the commits into one based on top of latest main would be fine as well IMO (although as a nitpick it would probably be nicer to have a separate commit for reordering of the README list first)

Dockerfile Outdated Show resolved Hide resolved
@strophy
Copy link
Collaborator Author

strophy commented Jan 3, 2023

Changes look good to me, but could you drop the merge commit? Simply squashing all the commits into one based on top of latest main would be fine as well IMO (although as a nitpick it would probably be nicer to have a separate commit for reordering of the README list first)

Thanks for the clear instructions, I installed GitLens and finally think I did it correctly! 🎉

@rvolosatovs
Copy link
Owner

Great job, thanks!

@rvolosatovs rvolosatovs enabled auto-merge (rebase) January 3, 2023 02:54
@rvolosatovs rvolosatovs merged commit 8aef971 into main Jan 3, 2023
@rvolosatovs rvolosatovs deleted the feat/scala branch January 3, 2023 03:25
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