Skip to content

wit/bindgen: Move wasmimports/wasmexports to a dedicated file#194

Merged
ydnar merged 5 commits intobytecodealliance:mainfrom
lxfontes:lxfontes/extern-file
Oct 10, 2024
Merged

wit/bindgen: Move wasmimports/wasmexports to a dedicated file#194
ydnar merged 5 commits intobytecodealliance:mainfrom
lxfontes:lxfontes/extern-file

Conversation

@lxfontes
Copy link
Copy Markdown
Member

@lxfontes lxfontes commented Oct 4, 2024

Move wasmimport and wasmexport declarations to a dedicated file (named <package>.wasm.go ).

Rationale: The current .wit.go file carries Go and functions intended to be used as wasm bridge ( wasm import / export ). Separating go code from wasm bridge allows readers to identify what is crossing the wasm boundary and gate them if needed. A secondary benefit is paving the way for wit-bindgen-go plugins that utilize the existing ABI and bridge into other implementations. The 2 that come to mind:

  1. test/mocking for component unit testing ( with & without wasmtime )
  2. networked protocols ( ex https://github.com/wrpc/wrpc )

Sample codegen for wasi:logging

logging.wit.go

//go:nosplit
func Log(level Level, context string, message string) {
	level0 := (uint32)(level)
	context0, context1 := cm.LowerString(context)
	message0, message1 := cm.LowerString(message)
	wasmimport_Log((uint32)(level0), (*uint8)(context0), (uint32)(context1), (*uint8)(message0), (uint32)(message1))
	return
}

logging.wasm.go

// Code generated by wit-bindgen-go. DO NOT EDIT.

package logging

// This file contains all wasmimport/wasmexport functions for "wasi:logging/logging".

//go:wasmimport wasi:logging/logging log
//go:noescape
func wasmimport_Log(level0 uint32, context0 *uint8, context1 uint32, message0 *uint8, message1 uint32)

Edit: a future PR will implement build tags for the <package>.wasm.go file, e.g. //go:build wasm || wasm32 || tinygo.wasm.

Copy link
Copy Markdown
Collaborator

@ydnar ydnar left a comment

Choose a reason for hiding this comment

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

This is great, thanks for drafting the PR. I support this direction.

Comment thread cmd/wit-bindgen-go/cmd/generate/generate.go Outdated
Comment thread wit/bindgen/generator.go Outdated
Comment thread wit/bindgen/generator.go Outdated
Comment thread wit/bindgen/generator.go Outdated
Comment thread wit/bindgen/generator.go Outdated
Comment thread wit/bindgen/generator.go Outdated
Comment thread wit/bindgen/generator.go Outdated
@ydnar
Copy link
Copy Markdown
Collaborator

ydnar commented Oct 5, 2024

FYI, this project follows the Go project conventions for PR names and commit messages:

package: present tense verb describing change

More context...

Signed-off-by: Lucas Fontes <lucas@cosmonic.com>
@lxfontes lxfontes force-pushed the lxfontes/extern-file branch from 24f3ab4 to 82bf7a9 Compare October 7, 2024 14:30
@lxfontes lxfontes changed the title feat: [Proposal] Move wasmimports/wasmexports to a dedicated file wit/bindgen: Move wasmimports/wasmexports to a dedicated file Oct 7, 2024
@lxfontes lxfontes marked this pull request as ready for review October 7, 2024 14:42
Copy link
Copy Markdown
Collaborator

@ydnar ydnar left a comment

Choose a reason for hiding this comment

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

One minor change, we could do now or later.

Comment thread wit/bindgen/generator.go Outdated
@ydnar
Copy link
Copy Markdown
Collaborator

ydnar commented Oct 7, 2024

Do you want to update the PR description to reflect current changes?

@ydnar
Copy link
Copy Markdown
Collaborator

ydnar commented Oct 7, 2024

I checked out the branch and ran go test ./wit/bindgen -write, which writes all the generated Go to the filesystem. Afterward, I ran go test ./..., which fails because the wasmimport functions were no longer found.

I think it’s important that go test ./... on Go packages generated by wit/bindgen pass, or at the very least, do not fail.

If the wasm functions are sequestered in a .wasm.go file, how do you think we can accomplish this?

Signed-off-by: Lucas Fontes <lucas@cosmonic.com>
@lxfontes
Copy link
Copy Markdown
Member Author

lxfontes commented Oct 8, 2024

I checked out the branch and ran go test ./wit/bindgen -write, which writes all the generated Go to the filesystem. Afterward, I ran go test ./..., which fails because the wasmimport functions were no longer found.

I think it’s important that go test ./... on Go packages generated by wit/bindgen pass, or at the very least, do not fail.

If the wasm functions are sequestered in a .wasm.go file, how do you think we can accomplish this?

It's the //go:build gate on that file.
That's where the wit-bindgen-go --wasmBuildTag was helpful, but could also cause confusion.

I do agree that failing go test is a blocker and think we have 2 options:

  • don't add the //go:build part to this PR
  • duplicate wasm file, with a build tag negating the wasm file's tags

Duplicating the wasm file doesn't seem to add any value comparing to removing the build tag.
The follow-up PR I was planning is a mock generator for non-wasm builds, performing the lift/lowering into existing structs. That might be a better time to add the //go:build gate to wasm files.

Still have the build tag in the PR and can remove it if that makes sense 👍

@ydnar
Copy link
Copy Markdown
Collaborator

ydnar commented Oct 9, 2024

Makes sense to extract the wasm calls into a separate file, without the build tag. That lets us merge this now and solve the related problem later.

lxfontes and others added 3 commits October 9, 2024 10:01
Signed-off-by: Lucas Fontes <lucas@cosmonic.com>
Signed-off-by: Lucas Fontes <lucas@cosmonic.com>
Comment thread wit/bindgen/testdata_test.go Outdated
@ydnar ydnar merged commit 97b5531 into bytecodealliance:main Oct 10, 2024
ydnar added a commit that referenced this pull request Oct 10, 2024
@ydnar
Copy link
Copy Markdown
Collaborator

ydnar commented Oct 10, 2024

I should have rebased on main before merging, as the type alias PR uncovered a subtle bug in the code that generates Go representation of variant and result types.

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