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

Vendor ocaml-protoc-plugin as a git subtree #4

Open
wokalski opened this issue Mar 27, 2022 · 8 comments
Open

Vendor ocaml-protoc-plugin as a git subtree #4

wokalski opened this issue Mar 27, 2022 · 8 comments
Labels
good first issue Good for newcomers

Comments

@wokalski
Copy link
Contributor

wokalski commented Mar 27, 2022

Why?

The main repo sees very little activity and maintenance. There are some unanswered issues and unmerged pull requests there.

Solution

I believe we should vendor ocaml-protoc-plugin in a git subtree (+ possibly publish it as ocaml-protoc-plugin-grpc).

The reason for a subtree is to make it easy to either split it into a separate repo or upstream the changes to issuu/ocaml-protoc-plugin in the future.

An alternative

We could obtain maintain access to https://github.com/issuu/ocaml-protoc-plugin. If we are given access I would like to enforce our open source policies there though; the main one is: every contributor that gets their PR accepted gets write access to the repo[1]. I've been quite successful in the past at keeping my open source projects healthy even after I lost interest in them. I attribute it to this policy. Do you think it's possible @andersfugmann?

1: the exception being "typo" fixes that might be automated or spammy.

Which solution should we pursue?

I believe in doing things fast - I think we should indeed create a subtree and vendor the repo and then when/if we are given a green light from issuu to maintain the upstream, we can remove the vendored version.

@quernd
Copy link
Collaborator

quernd commented Mar 28, 2022

Note that it's only an optional dependency of ocaml-grpc (you could use other protobuf libraries or no protobuf at all).

That being said, I agree that the repo has seen very little activity recently, and some forks offer enhancements that make it possible to add an opinionated way of making typesafe gRPC calls with minimum boilerplate. E.g. this unmerged PR parses service/method name.decoders/encoders and packages it up in a module together with the decoders/encoders. I also have a some more quality of life enhancements here and here.

@wokalski wokalski added the good first issue Good for newcomers label Mar 28, 2022
@wokalski
Copy link
Contributor Author

I added a "good first issue" label to this. It's not a good first issue for an OCaml newbie but for someone moderately familiar with OCaml it should be pretty straightforward: add ocaml-protoc upstream as a subtree here, and then create PR(s) with the quality of life improvements @quernd mentioned.

@bobot
Copy link

bobot commented Apr 14, 2022

FWIW I used a submodule instead of a subtree in https://github.com/ryjones/fabric-chaincode-ocaml

mbacarella pushed a commit to mbacarella/ocaml-grpc that referenced this issue May 7, 2022
@tmcgilchrist
Copy link
Collaborator

tmcgilchrist commented Mar 18, 2023

issuu/ocaml-protoc-plugin#28 parses service/method name.decoders/encoders and packages it up in a module together with the decoders/encoders

That PR has been merged since and released with ocaml-protoc-plugin.4.4.0 on opam. Do the other linked quality of life improvements still make sense @quernd ? Are there other improvements that could be packaged up as PRs to ocaml-protoc-plugin?

@quernd
Copy link
Collaborator

quernd commented Mar 18, 2023

Thanks for bringing this up! I'm going to rebase our fork and see how much is left and whether it makes sense to propose upstream.

@tmcgilchrist
Copy link
Collaborator

This is perhaps not required as vendored if we get the required changes into the next version of ocaml-protoc-plugin. Pending a decision on naming generated fields on issuu/ocaml-protoc-plugin#49 this should be closable soon.

@andersfugmann
Copy link

andersfugmann commented Jun 15, 2023

I've merged issuu/ocaml-protoc-plugin#50 which adds extra fields to the generated service struct which should allow you to implement generic functions as discussed in issuu/ocaml-protoc-plugin#49.

The changes are included in release 4.5.0, which is on its way into in opam repository (ocaml/opam-repository#23936).

@tmcgilchrist
Copy link
Collaborator

I'm working on updating the examples to use the new API. Thanks @andersfugmann

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants