-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
std::process for fuchsia: updated to latest liblaunchpad #40139
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
@@ -51,7 +51,7 @@ impl Command { | |||
} | |||
|
|||
unsafe fn do_exec(&mut self, stdio: ChildPipes) | |||
-> io::Result<(*mut launchpad_t, mx_handle_t)> { | |||
-> io::Result< mx_handle_t> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the catch
This looks good to me, on reading the code. I'll build locally as well, just to make sure. |
let mut process_handle: mx_handle_t = 0; | ||
let mut err_msg: *const libc::c_char = ptr::null(); | ||
mx_cvt(launchpad_go(launchpad, &mut process_handle, &mut err_msg))?; | ||
// TODO: See if we want to do something with that err_msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "tidy" linter didn't like this. It feels like a TODO to me, but if it wants FIXME, I suppose we should make it happy.
Looks great, thanks! r=me with the tidy failure fixed |
Confirm that Rust is functional again, xi editor builds and runs. I haven't deeply exercised process creation though. |
e994eaf
to
a198b41
Compare
a198b41
to
2123d6a
Compare
@bors r=alexcrichton tidy fixed @raphlinus yeah I plan to write a bunch of unit tests for sys/fuchsia to give us some peace of mind. |
@bors: r+ |
📌 Commit 2123d6a has been approved by |
Looks like some tests failed because of S3 outage yesterday. Is there a way I can trigger a rerun? |
@tedsta oh this PR is in the queue which is the one that really matters, the Travis status on the PR itself is mostly just advisory currently |
…lexcrichton std::process for fuchsia: updated to latest liblaunchpad Our liblaunchpad changed a bit and so fuchsia's std::process impl needs to change a bit. @raphlinus
…lexcrichton std::process for fuchsia: updated to latest liblaunchpad Our liblaunchpad changed a bit and so fuchsia's std::process impl needs to change a bit. @raphlinus
Our liblaunchpad changed a bit and so fuchsia's std::process impl needs to change a bit.
@raphlinus