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

Add bindings to libuv for process spawning #8645

Closed

Conversation

alexcrichton
Copy link
Member

This overhauls std::run to instead run on top of libuv. This is not in a mergeable state, I've been attempting to diagnose failures in the compiletest suite. I've managed to find a fair number of bugs so far, but I still don't seem to be done yet.

Notable changes:

  • This requires upgrading libuv. From the discussion on Upstream libuv patches and upgrade libuv #6567, I took libuv master from a few days ago, applied one patch to fix process spawning with multiple event loops in libuv, and pushed to my own fork
  • The build system for libuv has changed since we last used it. There's some extra checkout from a google build system which apparently does all the magic if you don't want to require autotools, and the google system just requires python. I updated the Makefile to get this build system and build libuv with it instead. This is untested on windows and arm, and both will probably need to see some improvement.
  • This required adding some pipe bindings to libuv as well. Currently the support is pretty simple and probably completely unsafe for pipes, but you at least get read/write methods. This is necessary for capturing output of processes.
  • I didn't redesign std::run at all, I simply tried to reimplement all the existing functionality on top of libuv. Some functions ended up dying, but nothing major. All uses of std::run in the compiler still work just fine.

I'm not quite sure how the rest of the runtime deals with this, but I marked process structures as no_send because the waiting/waking up has to happen in the same event loop right now. If processes start migrating between event loops then very bad things can happen. This may be what threadsafe I/O would fix, and I would be more than willing to rebase on that if it lands first.

Anyway, for now I wanted to put this up for review, I'm still investigating the corruption/deadlock bugs, but this is in an almost workable state. Once I find the bugs I'll also rebase on the current master.

@alexcrichton
Copy link
Member Author

I believe that this is moving into its final form. I'm verifying now, but I believe that with RUST_THREADS=1 this passes make check.

I believe the problem is that when invoking finish_with_output, two tasks are spawned to read all the output from the stderr/stdout pipes created to the child process. Without threadsafe I/O, I think that the event loops are wedging for one reason or another.

So, for now, I think this is blocked on #8631. I'll need to make a lot of changes to rebase on top anyway.

@alexcrichton alexcrichton mentioned this pull request Aug 20, 2013
@alexcrichton
Copy link
Member Author

Now that #8631 has landed (awesome work!) I've rebased on top of that and implemented all pipe I/O, and process manipulation through that interface.

I also refactored everything to mirror more closely what I realized the design was between rtio/uvio, and I think that it actually looks really nice now. The only unfortunate part is that the run module reaches all the way down into the uv::process module to build the configuration for it, but I don't think that it's too egregious.

I'm still running make check locally, but it passed my previously failing test which was smaller, so I think that this is ready to go!

r? @brson

@brson
Copy link
Contributor

brson commented Aug 22, 2013

Great work!

@brson
Copy link
Contributor

brson commented Aug 22, 2013

I do think it's a wart that UvProcessConfig (or whatever it's called) is exposed outside of rt::uv and that will have to change ... someday.

@alexcrichton
Copy link
Member Author

Once this lands, I'll look implementing a rt::io::native::process module which would basically contain all the platform-specific code that was in core::run beforehand.

At the same time, I think I may lift out UvProcessConfig into the rtio module because it'll be a common configuration between the uv/native versions.

@alexcrichton
Copy link
Member Author

@graydon, @brson, could one of you ssh into the mac bots and run xcodebuild -license? I'm hoping that it's the only fix for building libuv, but if it gets hairier I can investigate into setting up a more convenient build system.

@graydon
Copy link
Contributor

graydon commented Aug 23, 2013

Done on mac3, mac4, mac5, mac6. Should be good to try again.

@alexcrichton
Copy link
Member Author

Currently this is blocked on being able to build on windows (the failure is legitimate)

@olsonjeffery
Copy link
Contributor

Can someone point me at the buildbot failure for this? I have a windows vm and I'll try to make time to work through the issue(s) and make a PR to @alexcrichton

@alexcrichton
Copy link
Member Author

I was investigating this last night (sorry but working on windows is painfully slow for me), and the conclusion that I came to is that first we just need to figure out if libuv can build on mingw right now. It advertises itself as being able to do so, but I was unable to even just check out joyent/libuv and build it. If you could get that to work (and get a list of instructions), that's be a good first step!

@lucab
Copy link
Contributor

lucab commented Aug 27, 2013

A couple of notes not directly related to this PR:

  • The submodule will roughly be equivalent to libuv 0.11.12. If it gets tagged before rust 0.8, I'd suggest to synch to the tag commit, to ease tracking.
  • As libuv upstream is not really caring about mingw build, maybe we should setup an autobuilder to check earlier if nothing breaks.

@alexcrichton
Copy link
Member Author

It'd be even more awesome if we could use libuv master directly because then we could only optionally build libuv (assuming a minimum version is available). Right now we'd have to wait for a fix of joyent/libuv#887 to get merged into master to be able to use tip. Beyond that though, there's not really anything extra that we have any more.

It also turns out that their Makefile.mingw is a super-simple bare-bones type makefile. It doesn't look like it'd be too hard to even bundle directly for ourselves. It is worrisome that they're not very interested in mingw, but they also seem amenable to suggestions!

@lucab
Copy link
Contributor

lucab commented Aug 27, 2013

Ah, sorry I was under the impression that the bug-fixing commit was already merged into libuv master.

@alexcrichton
Copy link
Member Author

Yeah sadly it's more of a workaround than an actual fix. It suffices for now, but I think that it would degrade performance as a whole for libuv (even single-threaded even loops) which is why they're hesitant to merge it and would prefer to flesh out a more proper fix first.

@alexcrichton
Copy link
Member Author

Argh, I got so close! It turns out that this wedged the windows bots (hour of no output).

I'll try to investigate tomorrow night.

@alexcrichton
Copy link
Member Author

whoa, it appears you are correct.

@alexcrichton
Copy link
Member Author

well, that makes me happy that it wasn't a runtime-type regression!

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 6, 2013
This is a reopening of the libuv-upgrade part of rust-lang#8645. Hopefully this won't
cause random segfaults all over the place. The windows regression in testing
should also be fixed (it shouldn't build the whole compiler twice).

A notable difference from before is that gyp is now a git submodule instead of
always git-cloned at make time. This allows bundling for releases more easily.

Closes rust-lang#8850
bors added a commit that referenced this pull request Sep 6, 2013
This is a reopening of the libuv-upgrade part of #8645. Hopefully this won't
cause random segfaults all over the place. The windows regression in testing
should also be fixed (it shouldn't build the whole compiler twice).

A notable difference from before is that gyp is now a git submodule instead of
always git-cloned at make time. This allows bundling for releases more easily.

Closes #8850
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 18, 2013
This is a re-landing of rust-lang#8645, except that the bindings are *not* being used to
power std::run just yet. Instead, this adds the bindings as standalone bindings
inside the rt::io::process module.

I made one major change from before, having to do with how pipes are
created/bound. It's much clearer now when you can read/write to a pipe, as
there's an explicit difference (different types) between an unbound and a bound
pipe. The process configuration now takes unbound pipes (and consumes ownership
of them), and will return corresponding pipe structures back if spawning is
successful (otherwise everything is destroyed normally).
bors added a commit that referenced this pull request Sep 19, 2013
This is a re-landing of #8645, except that the bindings are *not* being used to
power std::run just yet. Instead, this adds the bindings as standalone bindings
inside the rt::io::process module.

I made one major change from before, having to do with how pipes are
created/bound. It's much clearer now when you can read/write to a pipe, as
there's an explicit difference (different types) between an unbound and a bound
pipe. The process configuration now takes unbound pipes (and consumes ownership
of them), and will return corresponding pipe structures back if spawning is
successful (otherwise everything is destroyed normally).
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 21, 2022
…archo

Don't lint `manual_non_exhaustive` when the enum variant is used

fixes rust-lang#5714

changelog: Don't lint `manual_non_exhaustive` when the enum variant is used
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.

7 participants