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

Remove converting to negative fd #3516

Merged
merged 5 commits into from Oct 16, 2015
Merged

Remove converting to negative fd #3516

merged 5 commits into from Oct 16, 2015

Conversation

tak1n
Copy link
Member

@tak1n tak1n commented Oct 10, 2015

Opening a PR for it because I'm not entirely sure if it should be removed.
Removing it means following behaviour:

Another way to merge multiple file descriptors is [:child, fd]. [:child, fd] means the file descriptor in the child process. This is different from fd. For example, :err=>:out means redirecting child stderr to parent stdout. But :err=>[:child, :out] means redirecting child stderr to child stdout. They differ if stdout is redirected in the child process as follows.

# stdout and stderr is redirected to log file.
# The file "log" is opened just once.
pid = spawn(command, :out=>["log", "w"], :err=>[:child, :out])

Convert a fd to -(fd + 1) if its a Fixnum seems wrong.
Negative fds's are mostly invalid.
@tak1n
Copy link
Member Author

tak1n commented Oct 10, 2015

Note:
Travis still doesn't recognize our 2.2 branch.
We should as soon as possible open a pr from 2.2 to master to get 2.2 specs run on travis.

@chuckremes
Copy link
Member

chuckremes commented Oct 10, 2015

You should create a spec that shows the failure in Process.spawn. It should go somewhere near here:
https://github.com/rubinius/rubinius/blob/2.2/spec/ruby/shared/process/spawn.rb#L335

Then make your code change and verify the behavior is correct. I don't think this PR will be merged without a spec.

@tak1n
Copy link
Member Author

tak1n commented Oct 10, 2015

Thx for reviewing accidentally forgot to commit it.

@chuckremes
Copy link
Member

chuckremes commented Oct 10, 2015

Looks good to me. Let's leave it for a few days to see if @YorickPeterse or @brixen have comments before merging it.

@@ -376,6 +376,14 @@
@name.should have_data("glark")
end

it "redirects STDERR to child STDOUT if :err => [:child, :out]" do
Copy link
Contributor

@YorickPeterse YorickPeterse Oct 10, 2015

Choose a reason for hiding this comment

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

I would use regular English for this instead, so if :err is [:child, :out], but that's a minor detail.

@YorickPeterse
Copy link
Contributor

YorickPeterse commented Oct 10, 2015

Regarding Travis, I can't seem to figure out why it's not building the 2.2 branch. Does running ./bin/mspec ci pass all specs?

@tak1n
Copy link
Member Author

tak1n commented Oct 10, 2015

All the specs which were marked as failing (IO popen) and my Process.spawn spec pass.
There are these nil/true/false specs failing, but they were already failing.

@YorickPeterse YorickPeterse self-assigned this Oct 10, 2015
* 2.2:
  allow nil,true,false to be modified when frozen
@YorickPeterse YorickPeterse merged commit 3a4c3fd into 2.2 Oct 16, 2015
@YorickPeterse YorickPeterse deleted the fix-process-spawn branch Oct 16, 2015
@YorickPeterse
Copy link
Contributor

YorickPeterse commented Oct 16, 2015

👍 Thanks

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.

None yet

3 participants