Skip to content
This repository has been archived by the owner on Nov 9, 2019. It is now read-only.

Implement fd_filestat_set_times using the filetime crate. #54

Merged
merged 4 commits into from
Aug 13, 2019

Conversation

marmistrz
Copy link
Member

In the future we could consider using filetime to replace systemtime_to_timestamp from #42.

@marmistrz
Copy link
Member Author

I rebased the change upon the new revision of #46.

@sunfishcode
Copy link
Member

This looks good to me. In particular, the interpretation of MTIM vs MTIM_NOW and so on looks reasonable. It looks compatible with WASI libc, and in general rejecting unimportant variations in usage will make things more consistent.

Would you mind adding a test for this to wasi-misc-tests?

@marmistrz
Copy link
Member Author

I'll add a test.

Can we change the spec to be explicit about the MTIM vs MTIM_NOW interpretation and close #53 (I probably should've raised the issue against wasmtime, not wasi-common)

@sunfishcode
Copy link
Member

Yeah. Actually, the best place to file #53 is the WASI issue tracker, tagged with wasi-filesystem.

@sunfishcode
Copy link
Member

Sounds good. And this just needs a few conflicts resolved before we can merge it.

@kubkon
Copy link
Member

kubkon commented Aug 9, 2019

@marmistrz could you fix conflicts please?

@marmistrz
Copy link
Member Author

marmistrz commented Aug 10, 2019

This still needs some work to pass the new test.

@kubkon
Copy link
Member

kubkon commented Aug 10, 2019

It's really interesting that it fails on *nixes and not Windows :-)

@marmistrz
Copy link
Member Author

Because I haven't enabled them yet on Windows :P

@kubkon
Copy link
Member

kubkon commented Aug 10, 2019

Haha, touche! Totally forgot about that! :-D Lemme know if you need any help with debugging or whatnot!

@marmistrz
Copy link
Member Author

The reason why CI on Mac fails is that on Mac filetime uses utimes and not utimensat. The former has microsecond precision, the latter - nanosecond. https://github.com/alexcrichton/filetime/blob/master/src/unix/mod.rs#L6-L23

@alexcrichton is there any reason why we can't use utimensat on Mac? The previous implementation in wasi-common used utimensat and passed the CI. utimensat should be available starting from MacOS 10.13.

@kubkon
Copy link
Member

kubkon commented Aug 12, 2019

@marmistrz do you want me try filetime out on macos with utimensat used instead of utimes? If it passes, we could submit a PR with a fix to @alexcrichton's repo.

@kubkon
Copy link
Member

kubkon commented Aug 12, 2019

@alexcrichton, together with @marmistrz we've worked out that @marmistrz's changes build and test out fine with alexcrichton/filetime#43 patch applied. Could you have a look and let me know what changes should be made to that PR so that we could merge it in, etc.?

@kubkon kubkon force-pushed the fd_filestat_set_times branch 2 times, most recently from f35e465 to f76bed5 Compare August 13, 2019 17:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants