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

misc/wasm: go_wasip1_wasm_exec script is broken by latest wasmtime #63718

Closed
johanbrandhorst opened this issue Oct 24, 2023 · 14 comments
Closed
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@johanbrandhorst
Copy link
Member

Does this issue reproduce with the latest release?

Yes

What did you do?

Upgraded to wasmtime 14.0.1 and ran

$ PATH=$PATH:$(go env GOROOT)/misc/wasm GOOS=wasip1 GOARCH=wasm go run main.go

on

//go:build wasm && wasip1

package main

import "fmt"

func main() {
	fmt.Println("Hello world")
}

What did you expect to see?

Hello world

What did you see instead?

error: unexpected argument '--max-wasm-stack' found

  tip: to pass '--max-wasm-stack' as a value, use '-- --max-wasm-stack'

Usage: wasmtime run --dir <HOST_DIR[::GUEST_DIR]> --env <NAME[=VAL]> <WASM>...

For more information, try '--help'.

Discussion

This is happening because the 14.0.0 wasmtime release introduced a breaking change to their command line flags. The change was introduced in bytecodealliance/wasmtime#6925. There was no transition period between the new and the old flags.

The error in question comes from the following line in misc/wasm/go_wasip1_wasm_exec:

exec wasmtime run --dir=/ --env PWD="$PWD" --env PATH="$PATH" --max-wasm-stack 1048576 ${GOWASIRUNTIMEARGS:-} "$1" -- "${@:2}"

The --max-wasm-stack flag was moved to the -W flag namespace. Anyone using wasmtime v14.0.0 and above will face an error when trying to use go_wasip1_wasm_exec through go test or go run.

There are a few ways we can deal with this.

  1. Introduce a runtime wasmtime version check to the script to switch between the new and the old flags. This would automatically fix users using newer versions of Go, but it wouldn't handle any custom GOWASIRUNTIMEARGS provided by the user.
  2. Update the script to use the new flag and drop support for wasmtime < 14.0.0. Probably something to consider a year or so down the line, but not right now.
  3. Do nothing to the script and educate users about not using wasmtime >= 14.0.0. Probably not sustainable in the long run.

I'm leaning towards option 1 and we're currently talking to the wasmtime team to understand what they recommend using for a runtime version indicator (might just be a matter of parsing the wasmtime --version output).

Other considerations

This breaking change (and how it was made) is not an indication of a stable project, and it might be appropriate for the Go project to change the default runtime in the go_wasip1_wasm_exec script and its test runners to something like wazero, a project with which we have more direct relations and has shown itself to be a responsible member of the ecosystem so far.

Since any fix we introduce to the exec script won't reach users until the Go 1.22 release, we may also want to add a warning to the WASI blog post that newer versions of wasmtime are not currently supported. As it stands, the examples using go test and go run in the blog post will fail to run with wasmtime >= v14.0.0.

@johanbrandhorst johanbrandhorst added the arch-wasm WebAssembly issues label Oct 24, 2023
@dmitshur dmitshur added this to the Go1.22 milestone Oct 24, 2023
@tschneidereit
Copy link

I want to echo here what Alex Crichton wrote in bytecodealliance/wasmtime#7336 (comment): we're very sorry for the disruption this change to wasmtime's CLI caused. As Alex laid our in that comment, we actually put a lot of thought into how best to do this change, because we understood beforehand that it'd cause disruption.

What we didn't do, and should have done, is actively reach out to key users of the CLI, such as this community. Another thing we didn't do, but should have, is write an RFC for this change. While as Alex says this is explicitly meant to be the (first and) last revamping of Wasmtime's CLI, we'll put heightened focus on ensuring that we do RFCs for the kinds of changes that might affect users of the CLI.

Generally, for however long you decide to keep using Wasmtime, I would recommend subscribing to the RFCs repository, as that's meant to be a low-volume way of keeping up with important changes. And if anything comes up there (or you learn about it via other means that you think isn't going the right way, we'd love to hear about it—be it via GitHub issues or on the Bytecode Alliance's Zulip.

@dr2chase
Copy link
Contributor

Tagging @golang/wasm.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 25, 2023
@cherrymui
Copy link
Member

Thanks for the detailed information. I agree that option 1 seems best. In the long run maybe we can retire the old flag, but probably not now. Switching the default runtime sounds also reasonable -- your call.

Since any fix we introduce to the exec script won't reach users until the Go 1.22 release

We could do a backport to the next Go 1.21.x minor release, if that helps. (Backport should be low risk, so we probably don't want to switch the default there, but a version check seems reasonable.)

Thanks.

@tschneidereit
Copy link

As a heads-up, we'll do a patch release of the Wasmtime CLI restoring support for the old interface, which should hopefully resolve the immediate issue here as fully as possible. More details here.

@johanbrandhorst
Copy link
Member Author

That's great to hear, thank you for moving quickly on this. We're happy to work with the wasmtime project on a migration plan to move to the new interface over time. For now, maybe we can just add a warning to the blog post stating that v14.0.0 and v14.0.1 of wasmtime are incompatible with the latest release of Go and encourage users to downgrade to earlier releases or the latest patch release. I went and looked at the latest release (https://github.com/bytecodealliance/wasmtime/releases/tag/v14.0.2) but couldn't find anything useful in the release notes about this, where can I expect to find the release notes about the releases?

@tschneidereit
Copy link

The release including this change is currently in the works, so it'll be v14.0.3.

A changelog can always be found here. We'll make sure that that is linked from the gh releases soon.

@tschneidereit
Copy link

We have now released Wasmtime 14.0.3, which restores the old CLI as described on Thursday. Please let us know if this isn't fully working as expected!

@johanbrandhorst
Copy link
Member Author

We're planning to introduce version detecting code supporting all recent versions of wasmtime in 1.22 and remove support for the older CLI flags in 1.23, which should give users ~1 year to upgrade their wasmtime versions.

@tschneidereit
Copy link

@johanbrandhorst that sounds great! And just to check my understanding, that means that we should keep support for the old CLI around until some time after 1.22 is released in February, but we don't have to wait until after 1.23?

@johanbrandhorst
Copy link
Member Author

Technically in Go we support the last 2 versions so ideally the switch wouldn't happen until 1.21 is no longer supported, which would be with the release of 1.23 in ~August 2024, but I understand if your project is OK with just supporting the latest version of Go.

@tschneidereit
Copy link

Thank you, that's helpful. We'll see were exactly we land, and will explicitly ask for your feedback once we have a proposed plan for disabling the old CLI.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/538699 mentions this issue: misc/wasm: silence Wasmtime 14 CLI warning

gopherbot pushed a commit that referenced this issue Nov 2, 2023
The latest version of Wasmtime, 14.0.4 as of writing this, offers a new
CLI while also supporting the old CLI. Since this is known and tracked
in issue #63718, silence the warning that otherwise causes many tests
to fail.

Since Wasmtime 13 and older don't pay attention to WASMTIME_NEW_CLI,
this change increases compatibility of the script, letting it work
with Wasmtime 9.0.1 as currently tested by the old cmd/coordinator, and
with Wasmtime 14.0.4 as currently tested in the new LUCI infrastructure.

The rest of the transition is left as future work.

For #63718.
For #61116.

Change-Id: I77d4f74cc1d34a657e48dcaaceb6fbda7d1e9428
Cq-Include-Trybots: luci.golang.try:gotip-wasip1-wasm_wasmtime
Reviewed-on: https://go-review.googlesource.com/c/go/+/538699
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/541219 mentions this issue: misc/wasm: support new wasmtime CLI

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 10, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/577135 mentions this issue: misc/wasm: drop wasmtime < 14 support

gopherbot pushed a commit that referenced this issue Apr 11, 2024
For Go 1.23, we decided to no longer support the old CLI interface
exposed by wasmtime. This removes the extra logic included to support
both the new and the old CLI interface. Now only versions of wasmtime
14 and newer are supported.

Fixes #63718

Change-Id: Iea31388dc41bc8d73caa923c7e4acae2228bf515
Reviewed-on: https://go-review.googlesource.com/c/go/+/577135
Reviewed-by: Randy Reddig <randy.reddig@fastly.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants