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.

Sign up for GitHub

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

Jump to bottom

Add yanix crate and replace nix with yanix in wasi-common #649

Merged
merged 7 commits into from
Dec 9, 2019
Merged

Add yanix crate and replace nix with yanix in wasi-common #649

merged 7 commits into from
Dec 9, 2019

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Nov 28, 2019 •
edited
Loading

This PR adds yanix crate as a Unix dependency for wasi-common. yanix stands for Yet Another Nix crate and is exactly what the name suggests: a crate in the spirit of the nix crate featuring certain adjusments and additions tailored specfically to its main application area which are implementation of WASI syscalls.

This PR also makes the necessary changes to wasi-common to depend only on yanix crate.

I apologise for the largeness of the PR! I've kept it as two orthogonal commits for easier reviewing, so I hope that helps somewhat.

This PR is a successor and hopefully an improvement of #640 and is the first of addressing #520.

@kubkon kubkon added the wasi:impl Issues pertaining to WASI implementation in Wasmtime label Nov 28, 2019
@kubkon kubkon mentioned this pull request Nov 28, 2019
Closed
@kubkon
Copy link
Member Author

kubkon commented Nov 29, 2019

cc @marmistrz

@alexcrichton
Copy link
Member

In terms of style and how to organize this, my personal preference is to do what the standard library does. There's a crate that has a cross-platform API, and then internally it's got a sys module that's entirely different per platform, but all platforms provide the same API to use in this platform-specific implementation module.

I don't get the feeling this PR is quite like that, but I don't think that's necessarily a bad thing. It's a reflection of reality as it is today and I think that's fine, incremental improvement is always good!

In that sense I mostly just want to comment about how I don't think this is the best organization we can have long-term, but I also don't want to block this in any way!

@kubkon
Copy link
Member Author

kubkon commented Dec 2, 2019

In terms of style and how to organize this, my personal preference is to do what the standard library does. There's a crate that has a cross-platform API, and then internally it's got a sys module that's entirely different per platform, but all platforms provide the same API to use in this platform-specific implementation module.

I don't get the feeling this PR is quite like that, but I don't think that's necessarily a bad thing. It's a reflection of reality as it is today and I think that's fine, incremental improvement is always good!

In that sense I mostly just want to comment about how I don't think this is the best organization we can have long-term, but I also don't want to block this in any way!

Thanks for the feedback! Yep, personally, I'm also a fan of the libstd's style. However, initially I wanted to keep yanix closer to nix organisation-wise in order to be able to compare which functionality is needed and which one is not, I assumed their style. Having said that, I'm all for rewriting it using the libstd style, however, I'd do it incrementally just as you suggested, and roll out the changes gradually in the subsequent PRs. Does that sound fair?

@alexcrichton
Copy link
Member

👍

definitely sounds good to me!

@kubkon
Copy link
Member Author

kubkon commented Dec 3, 2019

👍

definitely sounds good to me!

In the end, I've decided to the reorg in this PR since there were some other outstanding things to sort out. This, however, meant that it was easier to do the reorg by pulling in #650 into this PR which is less than ideal, but hopefully not a complete deal breaker.

sunfishcode
sunfishcode reviewed Dec 6, 2019
View reviewed changes
crates/wasi-common/yanix/src/clock.rs Show resolved Hide resolved
crates/wasi-common/yanix/src/errno.rs Outdated Show resolved Hide resolved
crates/wasi-common/yanix/src/fcntl.rs Outdated Show resolved Hide resolved
crates/wasi-common/yanix/src/fcntl.rs Outdated Show resolved Hide resolved
crates/wasi-common/yanix/src/fcntl.rs Outdated Show resolved Hide resolved
Jakub Konka added 7 commits December 8, 2019 14:18
Add yanix crate
ea6a995 
This commit adds `yanix` crate as a Unix dependency for `wasi-common`.
`yanix` stands for Yet Another Nix crate and is exactly what the name
suggests: a crate in the spirit of the `nix` crate featuring certain
adjusments and additions tailored specfically to its main application
area which are implementation of WASI syscalls.
Replace nix with yanix crate
07ed69e 
Having introduced `yanix` crate as an in-house replacement for the
`nix` crate, this commit makes the necessary changes to `wasi-common`
to depend _only_ on `yanix` crate.
Address review comments
ab3d875 
* make `fd_dup` unsafe
* rename `get_fd` to `get_fd_flags`, etc.
* reuse `io::Error::last_os_error()` to get the last errno value
Address more comments
171d2e5 
* make all `fcntl` fns unsafe
* adjust `wasi-common` impl appropriately
Make all fns operating on RawFd unsafe
fecc4b8
Fix linux build
6b7ed8f
Address more comments
9147a28
sunfishcode
sunfishcode approved these changes Dec 9, 2019
View reviewed changes
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your work putting this together!

@sunfishcode sunfishcode merged commit 51f880f into master Dec 9, 2019
@sunfishcode sunfishcode deleted the add-yanix-crate branch December 9, 2019 00:40
@kubkon kubkon mentioned this pull request Dec 11, 2019
Closed
@kubkon kubkon mentioned this pull request Dec 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunfishcode sunfishcode sunfishcode approved these changes

@peterhuene peterhuene Awaiting requested review from peterhuene

@alexcrichton alexcrichton Awaiting requested review from alexcrichton

Assignees
No one assigned
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kubkon @alexcrichton @sunfishcode