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

Run vtproto multiple times on different files of the same package #118

Closed
nockty opened this issue Dec 4, 2023 · 3 comments · Fixed by #120
Closed

Run vtproto multiple times on different files of the same package #118

nockty opened this issue Dec 4, 2023 · 3 comments · Fixed by #120

Comments

@nockty
Copy link
Contributor

nockty commented Dec 4, 2023

Context

Currently, it is not possible to run vtproto multiple times for the same package on different files because it would generate multiple times the same helpers, so the code would not compile.

As an example, let's take the pool package of this repository with the following proto files: pool.proto, pool_with_slice_reuse.proto, and pool_with_oneof.proto. As long as we run vtproto once with all those files, it works fine. However, if we run vtproto three times (once for each file), then the generated code doesn't compile because there are three declarations of the same functions / variables:

  • encodeVarint
  • sov
  • soz
  • skip
  • ErrInvalidLength
  • ErrIntOverflow
  • ErrUnexpectedEndOfGroup

I believe this is an issue because this behavior contrasts with the standard protobuf compiler, causing difficulties to onboard automation that assumes the ability to run the compiler for one file at a time. As a user, I'm currently facing this challenge and I would love to fix this issue to improve compatibility with automation and ensure a seamless transition for new users as well. 😃

Possible solutions

I can think of at least three possible solutions to tackle this issue. Before working on an implementation, I would like to gauge interest in the different solutions. If there is a preferred one, I can provide a Pull Request. My recommended one is solution A. There may also be other ways that I haven't think about.

A. Add helpers to vtprotobuf as a package and use them from the generated code (recommended)

We could expose public helpers (e.g. EncodeVarint) as part of the vtprotobuf package, as is currently done for vtprotobuf/types/known/... for supporting well-known types. The generated _vtproto.pb.go files of users would then have a dependency on a vtprotobuf/protohelpers package (for example) to use these helpers.

I think the dependency is fine but, if this is not acceptable for some users, we could add an option to generate today's no-dependency helpers instead. There is an existing --go-vtproto_opt=wkt=false option to avoid depending on vtprotobuf. We could consolidate both options under something like --go-vtproto_opt=vtproto_dep=false, which would disable both support for well-known types and usage of global helpers.

B. Generate a different file containing all helpers for the package

Besides files generated from source proto files, when running vtproto, we could generate an additional file defining the helpers, that we could name vtproto_helpers.go for instance. This way, when running vtproto once for each file, that file would be regenerated and left unchanged.

With this, there is no need for a dependency to vtprotobuf. However, one potential issue that I see is that we would lose the 1:1 mapping between source proto files and generated files, which may not be friendly to automation either. Indeed, the regular proto compiler generates one file for each source file and each generated file corresponds to one source file: there is an exact 1:1 mapping. This would no longer be the case for vtproto with this implementation.

C. Generate different helpers for each file

We could generate the same helpers with different names for different files. For instance (still taking the pool package of this repository), pool.proto would generate encodeVarint_pool, sov_pool, soz_pool, etc. and pool_with_slice_reuse.proto would generate encodeVarint_pool_with_slice_reuse, sov_pool_with_slice_reuse, soz_pool_with_slice_reuse.

I see two issues with this approach, though:

  • the generated code is more verbose and less readable;
  • some helpers are public so they are exposed to users. Not only would a public ErrUnexpectedEndOfGroup_pool_with_slice_reuse be less readable, it would also be confusing to have the same public error defined in several places under different names.
    • note that these public errors are currently generated for each package, so they are already defined in several places (with the same name). Defining them only once in vtprotobuf might be an additional benefit of solution A.
@vmg
Copy link
Member

vmg commented Dec 4, 2023

Option A seems the best by far. The "no dependencies" boat already sailed when we added support for external well-known types so I think it'd be perfectly reasonable to depend on a vtprotobuf package for these encoding helpers and reduce code duplication.

@nockty
Copy link
Contributor Author

nockty commented Dec 4, 2023

Thank you for your lightning fast answer! I'm glad that we agree on the best way forward; I'm going to work on a PR. Do you think we should still provide an option for users to opt out of the dependency?

@vmg
Copy link
Member

vmg commented Dec 4, 2023

I don't think opt-out will be necessary. Implementing the opt-out means we don't get to clean up the old code!

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 a pull request may close this issue.

2 participants