-
Notifications
You must be signed in to change notification settings - Fork 285
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 configurable base plugin support in buf.plugin.yaml #3513
Conversation
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
@@ -440,6 +441,7 @@ type ExternalGoRegistryConfig struct { | |||
Module string `json:"module,omitempty" yaml:"module,omitempty"` | |||
Version string `json:"version,omitempty" yaml:"version,omitempty"` | |||
} `json:"deps,omitempty" yaml:"deps,omitempty"` | |||
BasePluginName string `json:"base_plugin_name,omitempty" yaml:"base_plugin_name,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BasePluginName string `json:"base_plugin_name,omitempty" yaml:"base_plugin_name,omitempty"` | |
BasePlugin string `json:"base_plugin,omitempty" yaml:"base_plugin,omitempty"` |
This might be clearer and more concise. The intended format is remote/owner/plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in a2edfb7 to "base plugin" terminology.
// This affects how imports are resolved - the specified plugin's import path will be used as the | ||
// base path for all generated code, replacing the default protocolbuffers/go import paths. Used | ||
// when depending on non-default BSR plugins. | ||
string base_plugin_name = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srikrsna-buf pointed out this could be a boolean, like use_current_plugin_as_base
. But that'll only solve item 1. in the description (specifically, module dependencies).
You could also argue if a plugin has no plugin deps, then the default (on the BSR) should be to use the plugin itself as the base (for module deps) instead of protocolbuffers/go
. Example of petapis -> paymentaips module dep:
- v1alpha1 "bufbuild.internal/gen/go/acme/paymentapis/protocolbuffers/go/payment/v1alpha1"
+ v1alpha1 "bufbuild.internal/gen/go/acme/paymentapis/custom-plugins/go/payment/v1alpha1"
Either way, I went a step further and made this a configurable string so it satisfies both 1. and 2. Where 2. is module and plugin dependencies.
@@ -440,6 +441,7 @@ type ExternalGoRegistryConfig struct { | |||
Module string `json:"module,omitempty" yaml:"module,omitempty"` | |||
Version string `json:"version,omitempty" yaml:"version,omitempty"` | |||
} `json:"deps,omitempty" yaml:"deps,omitempty"` | |||
BasePlugin string `json:"base_plugin,omitempty" yaml:"base_plugin,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative way to configure this might be to expose an is_base: true
within deps
, e.g.,
deps:
- plugin: remote/org/go-multi:v0.1.0
is_base: true
And allowing exactly one plugin within deps to have this be set to true.
However, I don't like this because it leaks a Generated SDK configuration beyond the registry config. (which at the moment is Go-specific).
// Validate the base plugin is included as one of the plugin dependencies when both are | ||
// specified. This ensures there's exactly one base type and it has a known dependency to | ||
// generate imports correctly and build a correct Go mod file. | ||
if len(pluginDependencies) > 0 { | ||
ok := slices.ContainsFunc(pluginDependencies, func(ref bufremotepluginref.PluginReference) bool { | ||
return ref.IdentityString() == basePlugin.IdentityString() | ||
}) | ||
if !ok { | ||
return nil, fmt.Errorf("base plugin %q not found in plugin dependencies", externalGoRegistryConfig.BasePlugin) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intention that if there's no plugin dependency, assigning base_plugin
has no effect? Is there a use-case for assigning it without a matching dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, we still want to support option 1 in the description.
Suppose you have a multi-plugin that slams ALL the generators into a single multi-plugin. In this scenario, there are no plugin deps (all plugins output to the same package), but we still need to generate the correct module imports.
E.g., think of acme/petapis
, which depends on acme/paymentapis
. Without specifying the base type, we default to protocolbuffers/go
, but what we want is something like this:
package petv1
import (
- v1alpha1 "remote/gen/go/acme/paymentapis/protocolbuffers/go/payment/v1alpha1"
+ v1alpha1 "remote/gen/go/acme/paymentapis/custom-plugins/go/payment/v1alpha1"
Likewise, we need to build a go.mod
file that has the correct Go module:
module remote/gen/go/acme/petapis/custom-plugins/go
go 1.21
require (
- remote/gen/go/acme/paymentapis/protocolbuffers/go v0.2.0-20241204162507-076bca1034ed.1
+ remote/gen/go/acme/paymentapis/custom-plugins/go v0.2.0-20241204162507-076bca1034ed.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see, was not thinking about the multi-plugin. That makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the BSR should support option 1 with no further configuration. If there are no plugin dependencies, use the plugin name itself (instead of protocolbuffers/go
) for module dependencies.
Effectively, the same as manually setting just so protocolbuffers/go
isn't used.
version: v1
name: remote/org/go-multi # --- SAME
plugin_version: v0.1.0
output_languages:
- go
registry:
go:
base_plugin: remote/org/go-multi # --- SAME
min_version: "1.21"
deps:
- module: google.golang.org/protobuf
version: v1.35.2
- module: github.com/planetscale/vtprotobuf
version: v0.6.0
# Add the options to invoke each plugin for the generated SDK.
opts:
- --go_out=.
- --go_opt=paths=source_relative
- --go-json_out=.
- --go-json_opt=paths=source_relative,emit_defaults=true
- --go-vtproto_out=.
- --go-vtproto_opt=paths=source_relative,features=marshal+unmarshal+size
This PR adds an optional
base_plugin
field to the Go registry config, allowing plugins like gRPC and Connect to use a different base plugin instead of the defaultprotocolbuffers/go
. This enables Generated SDK scenarios:protocolbuffers/go
Note: A multi-plugin bundles generators like
protoc-gen-go
,protoc-gen-go-vtproto
, andprotoc-gen-go-json
that output to the same package and are packaged using: https://github.com/bufbuild/tools/tree/main/cmd/protoc-gen-multiExample: