-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
build: wasip1 wasmtime builder is broken #59583
Comments
Just checking: is wasmtime supposed to implement wasi preview 1? We already confused ourselves quite a bit before coming up with |
Yes, wasmtime does support preview 1, we did test it extensively during the implementation phase, but ran into some tests that were hard or impossible to satisfy on both runtimes (mostly because the Go tests tend to assume POSIX like behavior from the OS and wasmtime seems to have a slightly different philosophy to implementing certain things). We'll document each of the failings here once we get the runner up and running, so we can discuss how to proceed. |
Thanks. |
CC @golang/wasm (I realize this is redundant for the bug submitter). |
@johanbrandhorst would you mind listing the failing tests here (or notable ones)? That would be helpful for both of wazero and wasmtime to recognize the differences. |
Change https://go.dev/cl/485615 mentions this issue: |
Change https://go.dev/cl/485556 mentions this issue: |
The new GOWASIRUNTIME variable will be used to select the WASI runtime in the executable script. For golang/go#59583 Change-Id: Ie2d5f9373393b4c40ba592f956e5ad1e4b1fd207 Reviewed-on: https://go-review.googlesource.com/c/build/+/485556 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Heschi Kreinick <[email protected]> Auto-Submit: Johan Brandhorst-Satzkorn <[email protected]> Run-TryBot: Johan Brandhorst-Satzkorn <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
Change https://go.dev/cl/487015 mentions this issue: |
Today Current may fail if the binary is not built with cgo and USER and/or HOME is not set in the environment. That should not cause the test to fail. After this change, GOCACHE=$(go env GOCACHE) CGO_ENABLED=0 USER= HOME= go test os/user now passes on linux/amd64. For #59583. Change-Id: Id290cd1088051e930d73f0dd554177124796e8f2 Reviewed-on: https://go-review.googlesource.com/c/go/+/487015 Reviewed-by: Michael Pratt <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Reviewed-by: Johan Brandhorst-Satzkorn <[email protected]> Auto-Submit: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
As @bcmills noted in https://go-review.googlesource.com/c/go/+/485615/comments/7b08ced3_cf6d1646, the first real error on wasmtime was due to a symlink permission error. He suggested we add a runtime probe on wasip1 to I've also added a table to track test failures to the original topic. |
Allow switching to wasmtime through the GOWASIRUNTIME variable. This will allow builders to run the wasip1 standard library tests against the wasmtime WASI runtime. For #59583 Change-Id: I4d5200df7bb27b66e041f00e89d4c2e585f5da7c Reviewed-on: https://go-review.googlesource.com/c/go/+/485615 Reviewed-by: Bryan Mills <[email protected]> TryBot-Bypass: Johan Brandhorst-Satzkorn <[email protected]> Run-TryBot: Johan Brandhorst-Satzkorn <[email protected]> Reviewed-by: Cherry Mui <[email protected]>
Change https://go.dev/cl/490115 mentions this issue: |
The workaround for the symlink problem implemented in https://go.dev/cl/490115 seems to have worked and has given us another set of test failures:
I'll continue to investigate these errors, and I've updated the table at the top. |
I've been trying to get the new wasip1 target working in the browser for a few days (to get esbuild in the browser with a VFS) and have experienced many of the same "is a directory" / "not capable" errors in effectively every (non-wazero) runtime or shim I've tried. (e.g. the npm package There seems to be a somewhat universal problem among runtimes where they don't seem to support I didn't report something on this tracker since I think these are all issues with the runtimes, but here's a quick example if it's helpful getting a minimal repro: https://github.com/jakebailey/go-wasip1-directory-test |
Thanks for reporting @jakebailey, your code example was very useful to track down the issue! I submitted https://go.dev/cl/490755 which should address the issue (Go was requesting too many rights when opening directories). |
Excellent! I'll give the CL a try later. |
That patch does make (I know this isn't the thread for either; I only commented since the builder had the same directory error I was experiencing. Sorry for the other off-topic stuff!) |
The CL doesn't appear to be quite perfect; in the I recognized after writing the below that this issue is specifically about wasmer returns
It might be enough to just treat For now, I'm just cheating by changing |
Thanks for the thorough investigating @jakebailey, I do think we'll want a separate issue for the wasmer errors. I've created #59907 to track the progress on supporting the wasmer runtime. |
With https://go.dev/cl/490755 merged we have an exciting new error:
I've updated the table at the top. Full test log here. |
This isn't entirely true. wasi has an extended flag O_DIRECTORY to clarify if it should fail if the file is a directory. We have tests to show we return ENOTDIR on that flag when it is a file not a directory. |
p.s. I don't think anyone should assume a heuristic to use rights to hack semantics that layer on other flags. As "secret handshakes" come up, we can evaluate them pragmatically. One example of wandering this line is @achille-roussel recently in tetratelabs/wazero#1421 Just remember there are other compilers out there who may not have the same heuristics. Doing things like this without supporting tests in wasi-testsuite may lead to surprises for more people later, and possibly backtracking. |
Sure, but Regardless, I finally got a working WASI in the browser via wasi-js, so I'm already very happy with the state of tip! Thank you all for your hard work! |
I think that wasmtime should use O_DIRECTORY and not assume the opposite heuristically regardless of fdRights. |
soapbox warning: At least wasip1 should stick to POSIX in other words, and not go further than that when interpreting flags. It shouldn't be the case that the entire ecosystem should do something weird vs say out loud it is weird. It is effectively POSIX that at least gives us a way out of subjectivity in calls like this.. we can refer to https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html and see if it is wrong in general to allow open to succeed on a file or a directory. It is exactly things like this that scare me as WASI as an org distance themselves from POSIX, perhaps with a goal to not be like POSIX. We get more subjectivity than most OS the farther we go from things like this. WASI did cite some faults in the p1 design, but didn't at the same time say it is a goal for the successor to align closer to POSIX. This worries me. |
Certain WASI runtimes do not support generic symlinks, and instead return permission errors when they are attempted. Perform a runtime probe of symlink support in hasSymlink on wasip1 to determine whether the runtime supports generic symlinks. Also perform the same probe on android. For #59583 Change-Id: Iae5b704e670650d38ee350a5a98f99dcce8b5b28 Reviewed-on: https://go-review.googlesource.com/c/go/+/490115 Auto-Submit: Johan Brandhorst-Satzkorn <[email protected]> Reviewed-by: Achille Roussel <[email protected]> TryBot-Bypass: Johan Brandhorst-Satzkorn <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Run-TryBot: Johan Brandhorst-Satzkorn <[email protected]> Reviewed-by: Bryan Mills <[email protected]>
I'm unable to reproduce the |
With some help from the wasmtime team I added some runtime debug logging to the most recent test run. We can now see the following logs preceding the failure:
Not sure it helps us that much, the error still seems to be |
a lot of wasm runtimes share implementation code, but wazero has no deps, everything is from scratch. so we will fail in different ways than others ;) |
It appears this failure may have exposed an issue in wasmtime, I've been told the team is looking into it. |
The next wasmtime release (in ~two weeks) that includes bytecodealliance/wasmtime#6265 will fix the |
I'll look out for the next release, thanks! |
https://build.golang.org/log/a1da7514866e849d7af09fd7ff1af7fc8d11030f looks like the first build using a builder image rebuilt with wasmtime v9.0.1 (CL 496920). It made it past
|
Change https://go.dev/cl/498616 mentions this issue: |
https://go-review.googlesource.com/c/go/+/498616 unexpectedly fixed all the remaining test failures, so this issue is fixed 🎉. |
The PATH variable is required to run the testenv tests. Set it for all the runtime invocations where we don't already set it by inheriting from the environment. For #59583 For #59907 For #60097 Change-Id: If582dd8f086e3f40bc58d555f6034dcffe6f8e5f Reviewed-on: https://go-review.googlesource.com/c/go/+/498616 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Bryan Mills <[email protected]> Run-TryBot: Johan Brandhorst-Satzkorn <[email protected]>
Change https://go.dev/cl/498655 mentions this issue: |
The wasmtime WASI runtime now passes the standard library test suite. For golang/go#59583. Change-Id: I8e87cb46f3b7799996d4f1312478885e22fca1f8 Reviewed-on: https://go-review.googlesource.com/c/build/+/498655 Reviewed-by: Dmitri Shuralyov <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Auto-Submit: Johan Brandhorst-Satzkorn <[email protected]> Reviewed-by: Achille Roussel <[email protected]> Reviewed-by: Michael Knyszek <[email protected]> Run-TryBot: Johan Brandhorst-Satzkorn <[email protected]>
This is a tracking issue for problems relating to the wasip1-wasmtime builder. The wasmtime runner fails a number of tests, because of differences in how wasmtime and wazero (the primary wasip1 runner) have implemented the wasi_snapshot_preview1 syscall API.
This issue will track efforts to make the wasip1 port and wasmtime runner pass the standard library tests, or document which tests are impossible to fix because of runtime issues.
List of known failing tests on wasmtime:
The text was updated successfully, but these errors were encountered: