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

Replace nix dependency with in-house yanix crate #640

Closed
wants to merge 4 commits into from

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Nov 26, 2019

This commit replaces nix dependency with an in-house solution,
which borrows heavily from the nix crate, called yanix.
yanix stands for Yet Another Nix crate and is exactly what the name
suggests: an adaptation of the nix crate featuring certain
adjusments and additions tailored specfically to its main application
area which are implementation of WASI syscalls.

Since this commit belongs to the category of relatively big refactoring
efforts, here's a summary breakdown of major changes:

  • all uses of nix crate were replaced with yanix, and where
    possible, uses of raw libc calls was wrapped in a safe Rust fn and
    moved to the yanix crate
  • a couple of emscripten cfgs was added here and there, with the
    ultimate goal being able to cross-compile wasi-common to the
    emscripten target
  • fast-forwards snapshot_0 to the latest host-specific implementation
    so that this commit can be fully testable

Now, onto what this commit doesn't:

  • perform full-fledged refactoring of the *nix implementation - on the
    contrary, it barely scratches the surface; I welcome subsequent PRs
    which brings us closer to this goal though! :-)
  • make it possible to cross-compile wasi-common to emscripten target
    yet, but hopefully, brings us one step closer

From more detailed shortcomings of this commit, the implementation of
Dir (i.e., wrapper around readdir) is still left inside the
wasi-common proper for now, but if the time permits, I'll take a look
at moving it into the yanix crate where it seems to belong.

This PR relates to #520

@kubkon
Copy link
Member Author

kubkon commented Nov 26, 2019

cc @marmistrz

This commit replaces `nix` dependency with an in-house solution,
which borrows heavily from the `nix` crate, called `yanix`.
`yanix` stands for Yet Another Nix crate and is exactly what the name
suggests: an adaptation of the `nix` crate featuring certain
adjusments and additions tailored specfically to its main application
area which are implementation of WASI syscalls.

Since this commit belongs to the category of relatively big refactoring
efforts, here's a summary breakdown of major changes:
* all uses of `nix` crate were replaced with `yanix`, and where
  possible, uses of raw `libc` calls was wrapped in a safe Rust fn and
  moved to the `yanix` crate
* a couple of `emscripten` cfgs was added here and there, with the
  ultimate goal being able to cross-compile `wasi-common` to the
  `emscripten` target
* fast-forwards `snapshot_0` to the latest host-specific implementation
  so that this commit can be fully testable

Now, onto what this commit doesn't:
* perform full-fledged refactoring of the *nix implementation - on the
  contrary, it barely scratches the surface; I welcome subsequent PRs
  which brings us closer to this goal though! :-)
* make it possible to cross-compile `wasi-common` to `emscripten` target
  yet, but hopefully, brings us one step closer

From more detailed shortcomings of this commit, the implementation of
`Dir` (i.e., wrapper around `readdir`) is still left inside the
`wasi-common` proper for now, but if the time permits, I'll take a look
at moving it into the `yanix` crate where it seems to belong.
Copy link
Contributor

@marmistrz marmistrz left a comment

Choose a reason for hiding this comment

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

The change is so large that it's basically unreviewable. For the future, please try to split such changes into commits that could be reviewed separately. In particular, it would be good to have a separate commit for the functions that were just kanged from nix and a separate one for our wrappers.

Shouldn't you attribute the nix developers as the crate authors? I guess that most code was taken from nix.

@@ -0,0 +1,270 @@
//! Errno-specific for different Unix platforms
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider using std::io::Error instead of our own homebrew error type? Then you could also reuse cvt for syscall error handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, given that we are lower-level than potentially Rust itself, I'd prefer we've had our wrapper around the libc errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

And for that reason, I'm not able to reuse the cvt crate.

@kubkon
Copy link
Member Author

kubkon commented Nov 26, 2019

The change is so large that it's basically unreviewable. For the future, please try to split such changes into commits that could be reviewed separately. In particular, it would be good to have a separate commit for the functions that were just kanged from nix and a separate one for our wrappers.

Excellent point, and my apologies for the size of this PR, however, given the non-trivial nature of this task (replacing one core dependency with another), I've tried my best to keep the changes to minimum. Anyhow, this PR seems so large mainly due to the presence of changes in both the upstream and snapshot_0 which are essentially exactly the same bits of code, so you can safely ignore reviewing the changes in snapshot_0.

Shouldn't you attribute the nix developers as the crate authors? I guess that most code was taken from nix.

Absolutely! I haven't adjusted the authors bits in Cargo.toml just yet.

@kubkon
Copy link
Member Author

kubkon commented Nov 26, 2019

Actually, if it makes sense, I believe I can still chop this PR into a couple smaller ones if it aids the reviewing process. Now that I've got everything in place, I should be able to submit a series of PR instead of one large change. My main thinking behind one large change was that we'll have a setup tested from end to end. Lemme know what everyone thinks!

@kubkon
Copy link
Member Author

kubkon commented Nov 26, 2019

I'm gonna close this for now, as @sunfishcode and I have decided it would be better to avoid copying big chunks of code from nix. Instead, we believe we can easily rewrite those bits ourselves which will save us from having to deal with multi-licensing, etc. After I make the necessary adjustments to yanix, I will open a new PR for review.

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.

2 participants