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

Remove fd_filestat_get.rs #59

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Feb 28, 2023

Because it seems testing something very specific to wasmtime.

cf. bytecodealliance/wasmtime#4531

Because it seems testing something very specific to wasmtime.

cf. bytecodealliance/wasmtime#4531
@sunfishcode
Copy link
Member

I propose we keep this test, and document that fd_filestat_get on stdio fds returns a zero size and zero timestamps. This would prevent WASI programs from depending on the entity that stdin and stdout are connected to, making virtualization more robust. And it would making portability to Windows more robust, where stdin and stdout aren't files and don't support GetFileInformationByHandle.

@yamt
Copy link
Contributor Author

yamt commented Feb 28, 2023

I propose we keep this test, and document that fd_filestat_get on stdio fds returns a zero size and zero timestamps. This would prevent WASI programs from depending on the entity that stdin and stdout are connected to, making virtualization more robust. And it would making portability to Windows more robust, where stdin and stdout aren't files and don't support GetFileInformationByHandle.

while it's reasonable to allow the implementations to return 0,
i'm not sure if it's a good idea to require implementations to return 0.

@yamt
Copy link
Contributor Author

yamt commented Apr 7, 2023

I propose we keep this test, and document that fd_filestat_get on stdio fds returns a zero size and zero timestamps. This would prevent WASI programs from depending on the entity that stdin and stdout are connected to, making virtualization more robust. And it would making portability to Windows more robust, where stdin and stdout aren't files and don't support GetFileInformationByHandle.

while it's reasonable to allow the implementations to return 0, i'm not sure if it's a good idea to require implementations to return 0.

WebAssembly/WASI#531

@loganek
Copy link
Collaborator

loganek commented Aug 21, 2023

Actually, I've got some thoughts about the change introduced in WebAssembly/WASI#531 - it says:

This can be 0 if the underlying platform doesn't provide suitable timestamp for this file.

I wonder if the documentation should be updated saying that this must be zero for stdio? Otherwise, there'll be inconsistency across multiple implementations, unless there are good reasons to keep those timestamps non-zero for stdio. Any thoughts?

@yamt
Copy link
Contributor Author

yamt commented Aug 22, 2023

Actually, I've got some thoughts about the change introduced in WebAssembly/WASI#531 - it says:

This can be 0 if the underlying platform doesn't provide suitable timestamp for this file.

I wonder if the documentation should be updated saying that this must be zero for stdio? Otherwise, there'll be inconsistency across multiple implementations, unless there are good reasons to keep those timestamps non-zero for stdio. Any thoughts?

my thinking is that there is little reason to treat stdio that special. for example, there are filesystems which doesn't have timestamps at all. allowing zeros can benefit such filesystems/platforms.
i dunno how useful (or useless) stdio timestamps are. but i prefer to have less special cases.

@loganek
Copy link
Collaborator

loganek commented Aug 22, 2023

I don't have a strong preference here. But if the spec doesn't explicitly say that timestamps for stdio should be zero, I agree the test should be removed. If we want to keep the test, I think the doc must be updated accordingly.

@yamt
Copy link
Contributor Author

yamt commented Aug 22, 2023

I don't have a strong preference here. But if the spec doesn't explicitly say that timestamps for stdio should be zero, I agree the test should be removed. If we want to keep the test, I think the doc must be updated accordingly.

i'm not sure what you are talking about.
the doc has been updated in the PR you mentioned.

@loganek
Copy link
Collaborator

loganek commented Aug 22, 2023

The doc says:

This can be 0

which means it might or might not be zero. But the test expects that the size/time is always zero. The test is more specific than the documentation, which means that for some of the runtimes the test might fail.

@yamt
Copy link
Contributor Author

yamt commented Aug 22, 2023

The doc says:

This can be 0

which means it might or might not be zero. But the test expects that the size/time is always zero. The test is more specific than the documentation, which means that for some of the runtimes the test might fail.

ok. i was misreading your comment. thank you for explanation.
yes, i'm still in favor of removing this test.

@loganek
Copy link
Collaborator

loganek commented Aug 22, 2023

@sunfishcode , unless you have any concerns, I'll merge the PR

@loganek
Copy link
Collaborator

loganek commented Aug 29, 2023

I'm going to merge this PR as there are no concerns raised. Please open an issue for further discussion if needed.

@loganek loganek merged commit ea210d0 into WebAssembly:main Aug 29, 2023
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 this pull request may close these issues.

3 participants