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

Pass wasi-testsuite #1036

Closed
4 of 5 tasks
codefromthecrypt opened this issue Jan 15, 2023 · 20 comments · Fixed by #1133
Closed
4 of 5 tasks

Pass wasi-testsuite #1036

codefromthecrypt opened this issue Jan 15, 2023 · 20 comments · Fixed by #1133

Comments

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Jan 15, 2023

We should pass wasi-testsuite prior to 1.0. This includes the following:

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Jan 15, 2023

Made a shared fork including @achille-roussel's start and a small glitch fix. The wrapper syntax will change again once I get how mounts are expected to work working.

However, the more important thing is the failing tests:

$ python3 test-runner/wasi_test_runner.py  -t ./tests/assemblyscript/testsuite/  ./tests/c/testsuite/ -r adapters/wazero.sh 
  • lseek
  • stat-dev-ino
  • fdopendir-with-access
  • pwrite-with-access

Note: we were earlier told that the "rights" system was deprecated and not to be implemented as it was removed from wasi. This may or may not be related to the failures. I'll look into each of the above and tick off or explain.

@codefromthecrypt
Copy link
Contributor Author

see WebAssembly/wasi-testsuite#43 on proc_exit-failure

@codefromthecrypt
Copy link
Contributor Author

proc_exit-failure wasn't a real failure. it was about drift so I reworded the upstream issue

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Jan 15, 2023

To move forward we need to implement at least fd_tell and fd_pwrite as well add inodes to the stat results

@codefromthecrypt
Copy link
Contributor Author

next step is to implement io.WriterAt for pwrite. This is supported in windows and unix, so we should be ok. The lack of support in embed:fs is moot as embed:fs is read-only anyway. once this is weaved in, we should be able to pass the pwrite-with-access test

@codefromthecrypt
Copy link
Contributor Author

I will next Writer/ReaderAt stuff from WASI so that it is defined at the syscallfs layer (re-used for gojs this way, but also saner place to test it). after that, backfill integration tests for embed:fs, and finally implement pwrite in WASI and equiv in gojs

@codefromthecrypt
Copy link
Contributor Author

update: #1037 extracts ReaderAt stuff from WASI

next step: implement WriterAt as described in the last comment.

@codefromthecrypt
Copy link
Contributor Author

#1038 passes the pwrite-with-access test

@codefromthecrypt
Copy link
Contributor Author

#1041 passes all but one

@evacchi
Copy link
Contributor

evacchi commented Jan 25, 2023

there are some issues with passing on Windows.

  1. the test runner is assuming a UNIX env (e.g. running shell scripts with ./script.sh instead of, e.g. bash ./script.shhttps://github.com/tetratelabs/wasi-testsuite/pull/1
  2. some paths are being resolved in a weird way on Windows:
+ exec wazero run -hostlogging=filesystem -mount=/d/a/wazero/wazero/tests/c/testsuite/fs-tests.dir:/fs-tests.dir 'D:\a\wazero\wazero\tests\c\testsuite\stat-dev-ino.wasm'
invalid mount: path "D:\\a\\wazero\\wazero\\tests\\c\\testsuite\\fs-tests.dir;C" error: CreateFile D:\a\wazero\wazero\tests\c\testsuite\fs-tests.dir;C: The system cannot find the file specified.

notice the trailing ;C

(see https://github.com/tetratelabs/wazero/actions/runs/4008102301/jobs/6881833821)

in order to solve the first issue, I have set up with https://github.com/tetratelabs/wasi-testsuite/pull/1 an env variable you can export to override the default path to bash (which defaults to bash):

e.g., on Windows:

TEST_SHELL_RUNNER='C:\Program Files\Git\bin\bash.exe'

@evacchi
Copy link
Contributor

evacchi commented Jan 26, 2023

I found a better workaround WebAssembly/wasi-testsuite#48

@codefromthecrypt
Copy link
Contributor Author

there are some missing tests, which can be generated like cd cd tests/rust/; ./build.sh. We fail some of these due to what looks like a mount issue, so I'll look into it.

WebAssembly/wasi-testsuite#49

@codefromthecrypt
Copy link
Contributor Author

The way some rust tests are written, they are using underlying WASI calls instead of higher level integration like wasi-libc, so it is hard to tell which of these are impactful vs just trying for coverage of specified functions. In any case, the following remain:

  • fd_advise
  • fd_fdstat_set_flags
  • fd_renumber
  • path_symlink

There are also some other things that need closer looks

@codefromthecrypt
Copy link
Contributor Author

dangling_symlink fails for us due to path_open behavior. It seems wasmtime implicitly propagates O_NOFOLLOW which isn't defined in wasi. It is ok for us to change path_open the same way, but I think it should be a separate PR as it is behavior affecting. https://github.com/WebAssembly/wasi-testsuite/blob/main/tests/rust/src/bin/dangling_symlink.rs#L10 In that PR we should also coerce syscall.LOOP -> wasi ELOOP

@codefromthecrypt
Copy link
Contributor Author

TL;DR; unless a current end user or maintainer of wazero screams, I'm going to put an ignore for fd_readdir as I believe it is invalid and partially specified.

so we had somewhat of a long discussion on slack with @jerbob92 and @achille-roussel about the new tests of "." and ".." being somewhat of suspicious merit, as compilers including even rust filter them out. The current status is that the owner of WASI decided this can't change, but maybe can in snapshot-02. As the issue is complex especially wrt how to handle ".." etc on mounts, and the tests in wasi-testsuite barely cover the nuance, I think the best course of action is to ignore these this test until the specification improves. In other words, I think it is an invalid test and feel it isn't in the best direction for end users that we try to figure out how to reverse engineer a rationale spec, impl, and unit test out of it.

WebAssembly/wasi-testsuite#52

codefromthecrypt pushed a commit that referenced this issue Feb 5, 2023
The current fd_readdir test is invalid as it requires dot and dot-dot
entries, which both aren't required by POSIX nor returned by go. Once
this is addressed upstream, we can remove the skip.

See WebAssembly/wasi-testsuite#52
See #1036

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

#1098 on fd_readdir

codefromthecrypt added a commit that referenced this issue Feb 5, 2023
The current fd_readdir test is invalid as it requires dot and dot-dot
entries, which both aren't required by POSIX nor returned by go. Once
this is addressed upstream, we can remove the skip.

See WebAssembly/wasi-testsuite#52
See #1036

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

https://github.com/fermyon/wagi-ruby I think the next to do should be fd_fdstat_set_flags

==> wasi_snapshot_preview1.path_open(fd=3,dirflags=SYMLINK_FOLLOW,path=usr/local/lib/ruby/3.2.0/rubygems.rb,oflags=,fs_rights_base=,fs_rights_inheriting=,fdflags=NONBLOCK)
<== (opened_fd=4,errno=ESUCCESS)
--> wasi_snapshot_preview1.fd_fdstat_set_flags(fd=4,flags=0)
<-- errno=ENOSYS
==> wasi_snapshot_preview1.fd_close(fd=4)
<== errno=ESUCCESS
==> wasi_snapshot_preview1.fd_fdstat_get(fd=2)
<== (stat={filetype=CHARACTER_DEVICE,fdflags=APPEND,fs_rights_base=,fs_rights_inheriting=},errno=ESUCCESS)
<internal:gem_prelude>:2:in `require': Function not implemented -- /usr/local/lib/ruby/3.2.0/rubygems.rb (LoadError)
	from <internal:gem_prelude>:2:in `<internal:gem_prelude>'

@evacchi
Copy link
Contributor

evacchi commented Feb 15, 2023

current status on Windows. Only a few tests are failing. Last time I checked,

  • one was related to comparing nodes to verify file identity which IIRC may not work on Windows and is being reconsidered for later revisions
  • the other was the error code being returned on dangling symlinks

Here is a test run on Windows using the Python suite with wazero.py as well (just rebased and restarted to see the latest status) evacchi#5
and here is a test reproducing the symlink issue

@mathetake
Copy link
Member

so with #1132, all test passes, so I will raise a PR against the upstream so that it will include our runner.py by default

@mathetake
Copy link
Member

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 a pull request may close this issue.

3 participants