WebAssembly / wasi-testsuite

WASI Testsuite
Apache License 2.0
53 stars 21 forks source link

Reconsider MUST on "." and ".." entries in directory listings #52

Closed codefromthecrypt closed 1 year ago

codefromthecrypt commented 1 year ago

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.

sunfishcode commented 1 year ago

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 . and .. makes sense to me. The current filesystem API includes . and .. because it's what POSIX does. However, indeed, it's not especially useful behavior. I suspect it doesn't make sense to change this for preview1 because existing applications may be depending on the existing behavior. But for preview2, we can certainly consider this change; would you be interested in filing an issue in the wasi-filesystem repo, which is the proper forum to file such issues in?

codefromthecrypt commented 1 year ago

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?

codefromthecrypt commented 1 year ago

https://github.com/WebAssembly/wasi-filesystem/issues/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.

codefromthecrypt commented 1 year ago

@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?

codefromthecrypt commented 1 year ago

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.

The readdir() function shall not return directory entries containing empty names. If entries for dot or dot-dot exist, one entry shall be returned for dot and one entry shall be returned for dot-dot; otherwise, they shall not be returned.

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

sunfishcode commented 1 year ago

I think that wording refers to the fact that POSIX technically permits implementations to avoid assigning any special meaning to . and .., but when even POSIX calls something "historical", you know it's truly ancient :wink:.

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 ...

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 . and ... And wasi-libc has assumed that it's WASI's job to provide these for as long as it has existed.

I've now submitted a meeting agenda item and will leave it to the Subgroup as a whole to decide.

codefromthecrypt commented 1 year ago

closing this issue, here's a summary