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

Remove options in Managed Mode #645

Open
amckinney opened this issue Oct 8, 2021 · 6 comments · May be fixed by #3624
Open

Remove options in Managed Mode #645

amckinney opened this issue Oct 8, 2021 · 6 comments · May be fixed by #3624
Assignees
Labels
Feature New feature or request

Comments

@amckinney
Copy link
Contributor

We recently had a feature request related to PHP code generation and protoc-gen-validate. In summary, PHP can't generate code for proto2 files, so anything that depends protoc-gen-validate will not compile.

We want users to incorporate protoc-gen-validate as they please, but it's really only relevant when generating the server implementation in the user's language of choice (go in this case).

We've discussed managing .proto options in general (not just file-level options), so this is a good first candidate to explore that space. In this case, we would need to remove all instances of a particular set of options (those provided by validate.rules) and we would need to remove the import validate/validate.proto; declaration so that the PHP stubs don't initialize the initOnce hook.

@amckinney amckinney added Feature New feature or request P2 labels Oct 8, 2021
@bufdev
Copy link
Member

bufdev commented Oct 8, 2021

For future reference, this is roughly what we want:

diff --git a/data/template/buf.go-client.gen.yaml b/data/template/buf.go-client.gen.yaml
index 68b84740..7b522a82 100644
--- a/data/template/buf.go-client.gen.yaml
+++ b/data/template/buf.go-client.gen.yaml
@@ -5,6 +5,12 @@ managed:
   enabled: true
   go_package_prefix:
     default: github.com/bufbuild/buf/private/gen/proto/go
+  options:
+    exclude:
+      - (validate.rules)
+  imports:
+    exclude:
+      - validate/validate.proto
 plugins:
   - name: go-grpc
     out: private/gen/proto/go

@amckinney
Copy link
Contributor Author

amckinney commented Mar 21, 2022

At a high level, the proposal is fairly easy to implement - we can modify the Image just like the rest of the other Managed Mode modifiers, and strip each of the options/imports from the Image. Unfortunately, this means that the user could modify the Image in a way that makes it no longer compile.

For example, suppose that I strip the google/protobuf/descriptor.proto file from the buf.build/bufbuild/buf module. If I use this feature, the protoc-gen-go plugin yields the following (from the root of this repository):

$ buf generate proto --template data/template/buf.go.gen.yaml
protoc-gen-go: invalid FileDescriptorProto "buf/alpha/image/v1/image.proto": proto: message field "buf.alpha.image.v1.ImageFile.message_type" cannot resolve type: "google.protobuf.DescriptorProto" not found

We could consider this a user error and justify that the feature is working as expected, but it's still not an ideal user experience - the plugins could now receive a malformed Image from buf (which was previously a guarantee).

Removing options is also strange - we would need to clear the extension from the Image. This means we need to iterate over and interact with the relevant unknown fields (like we do in protosource, or something else along those lines). We would only affect the plugins that look for them (as intended). However, that still doesn't get us everything we want - the import statement would still remain which ends up affecting specific plugins. For example, protoc-gen-go will execute init hooks for unused imports unless the import is weak (link).

In order to satisfy both of these requirements, we'd need to verify that the Image compiles after we're applied the Managed Mode modifiers and return an error if it doesn't (before any of the plugins are invoked). This suggests a specific modifier ordering - should the options and imports be removed before any of the other standard modifiers are applied, or after? It probably makes sense to apply them before, but it still means we need to compile the image twice: once before it is modified, and again after it's been modified. With that said, we'd only need to re-compile the Image if certain modifiers were enabled (the import statement modifier in this case).


We should consider whether or not the complexity in this feature is worth it. There's already a fair amount of learning required for users to understand Managed Mode, and this adds even more questions into the mix. I'm not so worried about the implementation in this case - I'm only concerned about the UX.

@amckinney
Copy link
Contributor Author

Another thing to keep in mind here -

We're beginning to see similar functionality appear in different parts of buf. Now that we have partial images with the --type flag, there's an argument that this kind of behavior would be there instead of in Managed Mode. The two features act similarly - they both modify an Image in some way. We should be careful with introducing functionality to one and not the other, or introducing this functionality to both places and have multiple ways to do the same thing.

Similarly, this might be a feature that we want to implement on the AST before it's compiled into an Image (kinda similar to a buf format-esque approach). The user might actually want to modify the source itself so it can be published in two different places, where one version contains the options and another doesn't.

TL;DR We'll need to think about this one a bit more to decide where it should go.

@bufdev bufdev removed the P2 label Feb 2, 2023
@rauanmayemir
Copy link

Another case where this feature would shine is docs and api spec generators like gnostic. You wouldn't need them in the language-specific runtime.

@bhollis
Copy link

bhollis commented Apr 26, 2024

I found this issue while looking to file a similar request. Like in the original comment, we too would like to eliminate all protovalidate options and imports when generating code for our clients, while retaining them when generating code for our server.

This is to help reduce the code size of our clients - for some languages like Ruby, the entire descriptor is emitted into the generated code, even though Ruby can't make use of the protovalidate options. In this case, validate_pb.rb is 63KB.

I'd be happy for this to either be a transform on the AST, or on the Image. In the meantime we may end up implementing an AST-level transformer that pre-processes our .proto files.

@bufdev
Copy link
Member

bufdev commented Jun 13, 2024

A few thoughts here:

  • I think we'd actually want to make this a per-plugin option in the new v2 of buf.gen.yaml. Same level as include_imports and include_wkt.
  • We would need to be smart enough to only require the user to specify exclude_options. If the removal of these options results in the imports being unused, we remove the imports as well.
  • This is going to be a pretty difficult one to implement, but I'd like if we do it at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants