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

Refactor Process in preparation for Windows support #6744

Merged
merged 5 commits into from Oct 13, 2018

Conversation

@RX14
Member

RX14 commented Sep 18, 2018

Commits are best read in order, with their descriptions.

@RX14 RX14 force-pushed the RX14:feature/process-refactor branch from b8dceed to 3aebf51 Sep 18, 2018

Show resolved Hide resolved src/process.cr

@RX14 RX14 requested a review from bcardiff Sep 19, 2018

@ysbaddaden

Good changes overall. Just a few details I'd like to discuss.

Show resolved Hide resolved src/process.cr
Show resolved Hide resolved src/kernel.cr Outdated

@RX14 RX14 force-pushed the RX14:feature/process-refactor branch from 3aebf51 to bb6b770 Sep 19, 2018

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Oct 1, 2018

Let's merge.

@RX14 RX14 added this to the 0.27.0 milestone Oct 1, 2018

@RX14

This comment has been minimized.

Member

RX14 commented Oct 1, 2018

I was planning on rebasing the windows part of the process refactor on top of this PR to make 100% sure that this PR was complete, but I'm moving back to university so havent had time :(

I'm fine to merge this, but there's a very small but nonzero chance that there might be more amendments coming.

@bcardiff

I liked the refactors. Rebase & merge when needed.

@@ -147,6 +147,7 @@ class IO::FileDescriptor < IO
end
def reopen(other : IO::FileDescriptor)
return if self.fd == other.fd

This comment has been minimized.

@wontruefree

wontruefree Oct 9, 2018

Contributor

should this return self?
will this introduce a Nil into the types that can be returned?

This comment has been minimized.

@RX14

RX14 Oct 9, 2018

Member

Thanks for spotting! (this method should probably return nil but thats for another PR.

RX14 added some commits Sep 18, 2018

Refactor fork to remove fork_internal with block
Having a single fork_internal method allows all callers to replace it with
Crystal::System::Process.fork more easily.

Also a breaking change as Process.fork no longer uses `with self yield self`.
Convert args to argv in Process.exec_internal
This is useful when porting to Windows, as Crystal::System::Process.spawn will
take args as an array. This is because Windows's CreateProcess doesn't take an
argument vector.
Catch all errno from exec_internal
Previously only errors from LibC.execvp were caught.
Convert Stdio to file descriptors before Process.exec_internal
This means that Process.exec_internal is always passed an IO::FileDescriptor,
moving non platform-specific code outside of exec_internal and into it's
callers. Also fixes Process.exec crashing with a non-descriptive error when
passed an IO that's not an IO::FileDescriptor. Redirect::Pipe is also disallowed
for exec.

@RX14 RX14 force-pushed the RX14:feature/process-refactor branch from bb6b770 to 7a95390 Oct 13, 2018

@RX14 RX14 merged commit aad11d4 into crystal-lang:master Oct 13, 2018

2 of 4 checks passed

ci/circleci: test_linux32 CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment