-
Notifications
You must be signed in to change notification settings - Fork 21
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
Reconsider MUST on "." and ".." entries in directory listings #52
Comments
Rust also filters out . and .., and in general I encourage you to consider a level of patience. We have finite resources, and filesystems have an immense surface area of innumerable details, so it's inevitable that wasi-testsuite is going to be the place where some questions like this are going to arise. Filtering out |
I think that saying users are depending on this behavior maybe applies to wasmtime, but not others who filter it out. It feels like lifting behaviors of wasmtime immutable into this repo isn't the worst choice, but also not the best. So, basically I'll ask again if we can change these tests to MAY if it is required for wasmtime to continue that behavior. Would that be acceptable? |
WebAssembly/wasi-filesystem#90 for forward, and hoping after reflection you can reconsider the opinion here. End users and runtime owners also have time pressures made worse both by ambiguity and the arbitrary. |
@johanbrandhorst I know you are helping on the WASI proposal in Go, which is starting with snapshot-01 for pragmatic reasons. Since you are tighter in Go than me, copying you on this. I'm curious your opinion and my current thought is to fork the tests and edit them. If this repo allows test behavior contributions from other projects, we can raise that as a PR once things get more open. the long part In wazero, you know we use Go as a runtime, which is unlike most, what's also unlike most is we don't use LLVM. We tend to find implicit bias in implementation. This repo is an attempt to specify what wasi snapshot-01 was supposed to be, though there's no effort to document it. It seems mainly a tool to bridge to the next, but the point remains that this is literally a w3c repo named testsuite. We'd like to pass things here, but also not ignore reality on conflict. Effectively nothing is documented in specification, yet tests are being written. They seem to favor wasmtime as for example the tests were lifted from there with so far no interest in modification based on reality of other languages/runtimes. For example, in Go directory entries of "." and ".." are filtered out before we can read them in os.Dir, yet the main test here for reading a directory requires them to be present. This is literally the code in os/dir_darwin.go. We can't know if they existed or not. // Check for useless names before allocating a string.
if string(name) == "." || string(name) == ".." {
continue
} I think that possibly if there were multiple spec owners someone would agree with me that it is dubious to require this at the WASI abstraction, and in any case the role of a spec steward includes making those specs relevant, even specs that are inconvenient when one wishes to rally folks towards a new incomplete alternative. I think these pressures will result in issues like this, IOTW this may not be the only time this pops up. To think sustainably about it, give wazero's first priority is its users, not trying to reverse engineer directories to match this, we have a few options.
WDYT? |
As I'm working on a test filter meanwhile, I wanted to cite that I can't find a place in POSIX that is at odds with my suggestion to make this a MAY vs a MUST. If we are saying POSIX documents this in lieu of WASI documenting it, I think the spec agrees they may be returned, but aren't required to.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html I'm happy to modify the tests here accordingly if given the OK from @sunfishcode it would be accepted |
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 <[email protected]>
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 <[email protected]>
I think that wording refers to the fact that POSIX technically permits implementations to avoid assigning any special meaning to As I said above, I agree with you about changing this going forward. And as I said above, you're not the only one here implementing these features in a language whose standard library filters out And as I said above, the main consideration that I see for Preview1 is compatibility. I said POSIX, but it appears strictly speaking I should have said all popular POSIX-like platforms for probably decades at this point, and also Windows, appear to include the I've now submitted a meeting agenda item and will leave it to the Subgroup as a whole to decide. |
As discussed in the the 02-09 meeting, explicitly document that Preview1's `fd_readdir` includes the `.` and `..`. This resolves WebAssembly/wasi-testsuite#52.
closing this issue, here's a summary |
…`. (#516) As discussed in the the 02-09 meeting, explicitly document that Preview1's `fd_readdir` includes the `.` and `..`. This resolves WebAssembly/wasi-testsuite#52.
This manually adds dot and dot-dot directory entries discarded by compilers like Go. Doing so complies with the latest interpretation of wasi preview1 without forcing us to do the same in preview2 or in GOOS=js. There's a cost penalty of one stat per directory list, which will be only measurable when using real file I/O. This should result in us being able to remove the exclusion here https://github.com/WebAssembly/wasi-testsuite/pull/55/files#diff-8a3ffd323d75a12f8deb01b053f062876d83dda0b89c1fa24b293cee4195bcfd See WebAssembly/wasi-testsuite#52 Signed-off-by: Adrian Cole <[email protected]>
This manually adds dot and dot-dot directory entries discarded by compilers like Go. Doing so complies with the latest interpretation of wasi preview1 without forcing us to do the same in preview2 or in GOOS=js. There's a cost penalty of one stat per directory list, which will be only measurable when using real file I/O. This should result in us being able to remove the exclusion here https://github.com/WebAssembly/wasi-testsuite/pull/55/files#diff-8a3ffd323d75a12f8deb01b053f062876d83dda0b89c1fa24b293cee4195bcfd See WebAssembly/wasi-testsuite#52 Signed-off-by: Adrian Cole <[email protected]>
The recently added rust tests include tests that summarize as directory listings MUST return "." and ".." entires https://github.com/WebAssembly/wasi-testsuite/blob/main/tests/rust/src/bin/fd_readdir.rs#L150-L153
I think this should be backed by specification about why and also edge cases. For example, should a pre-open return ".." at any time? Should it return ".." if its pre-open name is "/"? What about if the pre-open name is ".."?
This is particularly of interest with virtual filesystems, which may not have a ".." notion at all. Meanwhile at least zig and go guests filter out "." and ".." from listings, but to synthetically produce ".." based on the above cases is not only extra state but also futile when they are discarded.
My opinion is that the testsuite should not introduce new rules that are not documented in the spec itself, and so if this should be a MUST, it should be specified first, including edge cases. In other words, this test should be reverted or switched to a MAY. If folks believe this should be a MUST then we need to move this issue to the proper forum to debate it and the implications diversely, inclusive of people who represent Go and Ziglang communities.
The text was updated successfully, but these errors were encountered: