-
Notifications
You must be signed in to change notification settings - Fork 258
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
Explicitly document that Preview1's fd_readdir
includes .
and ..
.
#516
Explicitly document that Preview1's fd_readdir
includes .
and ..
.
#516
Conversation
As discussed in the the 02-09 meeting, explicitly document that Preview1's `fd_readdir` includes the `.` and `..`. This resolves WebAssembly/wasi-testsuite#52.
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? |
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. |
The PR establishing for Preview2 making it exclude The notes for the meeting aren't posted yet, but briefly, there were no objections to the plan to document that Preview1 includes the It's my understanding that we already have a test for the basic behavior here, as that seemed to be what prompted WebAssembly/wasi-testsuite#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 |
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. |
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. 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. |
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. |
@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. |
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. |
@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. |
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. |
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 WebAssembly/wasi-filesystem#3. Specifically, they noted dot entries were not portable. I'm linking this as it wasn't mentioned before in this thread. |
As discussed in the the 02-09 meeting, explicitly document that Preview1's
fd_readdir
includes the.
and..
.This resolves WebAssembly/wasi-testsuite#52.