WebAssembly / WASI

WebAssembly System Interface
Other
4.72k stars 240 forks source link

Explicitly document that Preview1's `fd_readdir` includes `.` and `..`. #516

Closed sunfishcode closed 1 year ago

sunfishcode commented 1 year ago

As discussed in the the 02-09 meeting, explicitly document that Preview1's fd_readdir includes the . and ...

This resolves WebAssembly/wasi-testsuite#52.

codefromthecrypt commented 1 year ago

Since you mention preview1 can you link or just comment here the position on preview2? Also, can you link to the notes, e.g. who decided this and why?

codefromthecrypt commented 1 year ago

Also, please add a test that covers the impact of the decision here. For example, naive things will add ".." for root, and I also mentioned other edge cases. Adding spec should not just have work for downstream implementors like us, but also a shared burden here. Otherwise, it is very easy to create work for other people that still results in nonsense data.

sunfishcode commented 1 year ago

The PR establishing for Preview2 making it exclude . and .. is https://github.com/WebAssembly/wasi-filesystem/pull/93. It's merged, though comments or new issues are still welcome. That said, I'm not presently aware of anyone with objections.

The notes for the meeting aren't posted yet, but briefly, there were no objections to the plan to document that Preview1 includes the . and ... A major goal for WASI is to promote inteoperability between implementations, and that would have us reduce implementation choices, rather than add new ones. WASI has other goals too, such as implementation feasibility, however we have examples of fd_readdir implementations written on top of standard-library APIs which exclude . and .. using extra code to re-add entries for . and .., so unless someone wants to make the case that that's a prohibitive burden for them, this doesn't seem to override the goal of interoperability.

It's my understanding that we already have a test for the basic behavior here, as that seemed to be what prompted https://github.com/WebAssembly/wasi-testsuite/issues/52 and this discussion. More tests for more details would be good, though this issue doesn't need to block on that. The Subgroup isn't obligated to do work just to make people feel better about having to do work themselves.

All Unix-like platforms I have access to have a .. in the root directory.

codefromthecrypt commented 1 year ago

The Subgroup isn't obligated to do work just to make people feel better about having to do work themselves.

What I was trying to get at, is similar to how any open source project would work, there's a good balance in having to dog food or deal with end users outside the inner circle in some way. Having some responsibility in general is the goal, it isn't about making people feel better about doing work, it is about being more vested in the greater ecosystem. For example, there's a large and deserved perception that wasi-libc is basically the anchor of all decisions, and that doesn't really feel like a spec/impl separation. things like this have come up, and one way to try to get in front of it is being more outward/outreach focused. This is a part of community I'm suggesting, but indeed your subgroup can decide to dismiss this the way you just did.

codefromthecrypt commented 1 year ago

All Unix-like platforms I have access to have a .. in the root directory.

I'll reply for those who might be interested why I asked about ".."

The current WASI test suite dodges some property tests on "..", which is the edge case I mentioned. For example, it doesn't check the inode or name length. I'm not sure why this was done, which is what triggered this. https://github.com/WebAssembly/wasi-testsuite/blob/main/tests/rust/src/bin/fd_readdir.rs#L108-L111

The other case is that "../.." mounts (pre-opens) are allowed by wasmtime which is where the spec tests were taken from. However, there are no tests about this sort of thing.

The main issue for implementors here, is that it isn't possible to know what will be enforced next because nothing is written down and questions about them are not valid. Basically whatever wasi-libc or wasmtime do must be the spec, and that's very hard to track.

linclark commented 1 year ago

Also, can you link to the notes, e.g. who decided this and why?

Apologies—as the WASI chair, this is on me for not having gotten the notes posted on the meetings repo yet. You can find the meeting notes here. They are also available right after the meeting to all subgroup members in the Google Doc where we take the notes, so you can join the subgroup (free to all) if you want to have immediate access to them.

This was decided by the subgroup as a whole. Dan had added it as a discussion to the meeting agenda, which he informed you of here. Anyone who wants to represent their opinion in a discussion is allowed to take part in the discussion. The instructions for joining the subgroup are available on the agendas.

In this case, the general consensus of the group (not just @sunfishcode) was that WASI should not make the suggested change for the reasons indicated in the notes. If you would like to make a case to the group and have them reassess, then you could add an agenda item to re-discuss it. However, I would ask that you not imply that individual contributors aren't vested in the greater ecosystem or that they are arbitrarily "dismissing" things when they express disagreement. WASI's users have a broad range of needs and priorities, and these are sometimes in tension with each other. In those cases, we have to weigh the pros and cons and choose the least bad option. I believe that is what happened here, and if you believe relevant information wasn't discussed, then you can bring that to the discussion table.

codefromthecrypt commented 1 year ago

@linclark thanks for your response. I think phrasing this as a "least bad option" is appropriate.

I do believe there's an implicit prioritization here, both that wasi-libc and wasmtime behaviors are inherently the correct impl (e.g. literally lifting tests from wasmtime.. this bias should be thought deeply about), and also prioritizing input from those who attend meetings.

These things create a pressure system that doesn't lean into external ecosystem, but does makes change more convenient for the central one. Particularly, it would be really nice if I had the ability to just simply add a behavior by lifting my tests into a spec no questions asked. The other side isn't as nice. I think this impedance mismatch between normal OSS and specs is common, not just WASI iotw. Similar things happen in webassembly core as well, and even outside W3C. Anyway, it is outside the scope of this issue to correct, but I really do hope someone in the subgroup can start thinking hard about for example why does tinygo not even implement readdir in wasi? What role does the community have in the success of the community at large? What signals would be used to show not tell that the community is vested outwardly and towards newcomers as much as several year familiars? These are things I'm really asking you to think about as a chair, as I know you want the best here.

sunfishcode commented 1 year ago

My understanding is that Marcin chose to import some Wasmtime tests into wasi-testsuite because they wanted some tests in the testsuite. I imagine they'd accept PRs adding tests from other engines too.

The nature of the space is that every time we add new tests to wasi-testsuite, and every time a new engine runs the testsuite, we'll likely find interpretations that differ. When there isn't a clear consensus on what the right fix is, the Subgroup is the tool we have to make decisions. The Subgroup has many people, participating from many organizations, engaged in many different areas of the community, and I encourage you to participate as well.

As a historical note, fd_readdir's API is the way it is because it came from CloudABI. We learned a lot of valuable things from CloudABI, and we also learned about some areas where WASI's needs differ from CloudABI's. Consequently, Preview2 has a directory iteration API that's much friendlier to non-C languages. And with your suggestion to remove "." and "..", it's even friendlier to use.

codefromthecrypt commented 1 year ago

@sunfishcode thanks for your feedback and footnote. I think all of you heard my feedback, and I feel heard, and that is much appreciated. It is even more appreciated that the next version is starting to make things a little bit easier, not just the dot dot-dot, but also the other recent perf regression about inodes, handled differently in preview2 as well.

I think the wasi-testsuite when run upstream on diverse runtimes and also windows will remove a lot of need for me to personally raise issues on behalf of people who tell me about these issues.

If I get one wish granted, take at least windows as a P1 design concern and fail the testsuite repo if a design change invalidates windows. This should apply to other repos as well, including any for new proposals. In other words, even if only bytecodealliance runtimes are used, require them to pass and keep passing. Right now, they are not required to pass. We require your tests to pass in wazero, so it doesn't make sense that upstream doesn't.

Anyway, as you mentioned, it is up to the subgroup (presumably via discussion and not issues like this) to make judgement calls about your own runtimes, and decide the path forward.

linclark commented 1 year ago

I'm glad to see folks seeing each other's perspectives here. I think we've reached a place where this PR can be merged. If there's further discussion to be had around the other content in this thread, do feel free to open up new issues or discussions to focus on those points.

codefromthecrypt commented 1 year ago

FYI: I was doing research on this as it is very problematic, and found the earliest time someone complained about this to the WASI project was in late 2019 https://github.com/WebAssembly/wasi-filesystem/issues/3. Specifically, they noted dot entries were not portable. I'm linking this as it wasn't mentioned before in this thread.