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

Init support for local WASM plugins #3349

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft

Init support for local WASM plugins #3349

wants to merge 24 commits into from

Conversation

emcfarlane
Copy link
Contributor

This PR adds support for running WASM based plugins. Declare a plugin with the suffix .wasm to invoke the plugin using a WASM runtime. For example the plugin buf-plugin-field-option-safe-for-ml can be compiled with GOOS=wasip1 GOARCH=wasm and added to your buf.yaml config:

version: v2
plugins:
  - plugin: buf-plugin-field-option-safe-for-ml.wasm

Copy link
Contributor

github-actions bot commented Sep 27, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedOct 2, 2024, 4:07 AM

@@ -12,8 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build aix || darwin || dragonfly || freebsd || (js && wasm) || linux || netbsd || openbsd || solaris
// +build aix darwin dragonfly freebsd js,wasm linux netbsd openbsd solaris
//go:build unix || wasip1 || js
Copy link
Contributor Author

@emcfarlane emcfarlane Oct 1, 2024

Choose a reason for hiding this comment

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

Use the unix build constraint and drop support for +build as that is required for go versions 1.16 and earlier. Fixes builds for GOOS=wasip1

@@ -135,6 +139,17 @@ func NewCommitProvider(container appext.Container) (bufmodule.CommitProvider, er
)
}

// CreatePluginCacheDir creates the cache directory for plugins.
//
// This is used by the [bufwasm.WithLocalCacheDir] option.
Copy link
Member

Choose a reason for hiding this comment

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

Is the [] something that Godoc picks up? If not, we don't use it anywhere else. If so, good to know!

@@ -103,6 +103,10 @@ var (
//
// Normalized.
v3CacheModuleLockRelDirPath = normalpath.Join("v3", "modulelocks")
// v3CachePluginsRelDirPath is the relative path to the plugins cache directory in its newest iteration.
Copy link
Member

Choose a reason for hiding this comment

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

This documentation isn't really accurate. If you dive down into the code, it appears you are passing this to the bufwasm.Runtime, which then happens to use this as a cache for some WASM compilation that is happening. Given the naming, and the documentation here, I would expect this to be a cache of downloaded plugins from the BSR, which it is not. This is effectively an implementation-specific cache.

This should be v3/wasmruntime or the like, whatever makes sense, and the documentation should reflect what this is specifically used for.

@@ -135,6 +139,17 @@ func NewCommitProvider(container appext.Container) (bufmodule.CommitProvider, er
)
}

// CreatePluginCacheDir creates the cache directory for plugins.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem correct. See above.

@@ -94,6 +95,7 @@ func (c *multiClient) Check(ctx context.Context, request check.Request) ([]*anno
jobs = append(
jobs,
func(ctx context.Context) error {
start := time.Now()
Copy link
Member

Choose a reason for hiding this comment

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

Such timings are the responsibilities of tracing.Tracers

"pluginrpc.com/pluginrpc"
)

// RunnerProvider provides pluginrpc.Runners for a given plugin config.
Copy link
Member

Choose a reason for hiding this comment

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

PluginConfig

}

// wasmRunner is a runner that loads a Wasm plugin.
type wasmRunner struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in this file?

@@ -0,0 +1,81 @@
// Copyright 2020-2024 Buf Technologies, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in bufpkg? It uses nothing buf-specific, and therefore should be in pkg/wasm.

"pluginrpc.com/pluginrpc"
)

// Plugin is a Wasm module.
Copy link
Member

Choose a reason for hiding this comment

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

If Plugin is a WASM module, why is it named Plugin, especially given that plugins have a specific meaning in buf?

}

// NewUnimplementedRuntime returns a new unimplemented Runtime.
func NewUnimplementedRuntime() Runtime {
Copy link
Member

Choose a reason for hiding this comment

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

This can be var UnimplementedRuntime = unimplementedRuntime{}

if r.wasmRuntime == nil {
return nil, syserror.Newf("wasm runtime is required for local wasm plugins")
}
return newWasmRunner(
Copy link
Member

Choose a reason for hiding this comment

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

It's weird that this is implemented in bufpluginrunner while the command.Runner-based Runner is implemented in pluginrpcutil. Why the split? I don't see any reason for one, both Runners should be implemented in pluginrpcutil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can't be implemented in pluginrpcutil as the pkg now depends on bufconfig for the bufconfig.PluginConfig. Could move pluginrpcutil under bufpkg?

Copy link
Member

Choose a reason for hiding this comment

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

the wasmRunner in no way depends on bufconfig.PluginConfig, I'm not sure what you are talking about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Referring to the pluginrpcutil. All good on moving the wasm runner.

@emcfarlane emcfarlane marked this pull request as ready for review October 1, 2024 22:43
@emcfarlane emcfarlane marked this pull request as draft October 2, 2024 00: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