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

Alternative changes to make Process::Status compatible with MRI #3271

Closed
wants to merge 1 commit into from

Conversation

sodabrew
Copy link
Contributor

@sodabrew sodabrew commented Jan 5, 2015

Alternative approach to issue #3268 instead of PR #3270.

Needs specs, still a WIP, please don't merge yet, etc.

I haven't compiled this yet on OS X due to LLVM issues (worked with ruby-build, doesn't work on the command line, I'm confused). Sorry about using Travis to do the heavy lifting.

@sodabrew sodabrew changed the title Alternative changed to make Process::Status compatible with MRI Alternative changes to make Process::Status compatible with MRI Jan 5, 2015
@yorickpeterse
Copy link
Contributor

Regarding LLVM, what problems are you having? Is it complaining llvm-config can't be found?

@sodabrew
Copy link
Contributor Author

sodabrew commented Jan 5, 2015

I'm on OS X Yosemite, so there's no gcc-apple-4.2, yet vendor/*Makefiles are still showing up with CC=gcc-apple-4.2 even after I configure with ./configure --with-vendor-yaml --llvm-config llvm-config-mp-3.5 --cc /usr/bin/clang --cxx /usr/bin/clang++

@yorickpeterse
Copy link
Contributor

I believe you need to install llvm via Homebrew, see rbenv/ruby-build@4aeeac2.

@sodabrew
Copy link
Contributor Author

sodabrew commented Jan 5, 2015

I'm using LLVM via MacPorts, and it works fine to compile Rubinius itself, but the vendor libs are picking up the old compiler. I'll tinker with it later today.

About the approach in this PR: while it better matches the MRI behavior, it totally misses the point that the status code interpretation is provided by a set of MACROS that should be implementation defined. I have no idea why MRI has a copy of those macro definitions in its own codebase.

I'm not sure how much we can or should care about Process::Status.status and Process::Status.to_i matching MRI exactly (that is, having all of the status bits not just the exit code).

@yorickpeterse
Copy link
Contributor

In this case my question would be: is there any code that breaks because Rubinius does not provide the same macros/bitwise stuff/etc? If the answer is yes then we should match that behaviour where possible, otherwise I don't think it makes that much sense.

Either way, this PR will have to wait until the other one is merged as otherwise we'll probably end up in Git merge hell :)

@brixen
Copy link
Member

brixen commented Jan 7, 2015

@sodabrew @yorickpeterse is there a value exposed to Ruby in MRI that would require us to use bit-fields for compatibility?

@brixen
Copy link
Member

brixen commented Jan 7, 2015

@sodabrew I'm not opposed to your approach but could you please reapply on master, force push, so we can clearly evaluate the changes?

@sodabrew
Copy link
Contributor Author

sodabrew commented Jan 7, 2015

I rebased, but that's really just academic. I no longer believe it is a good idea to hardcode the bitwise relationships in Ruby - they are defined to be C macros in the Unix spec, so they have to be in the C++ code in vm/builtin/system.cpp in order to enable platform portability.

Thanks for the quick work on the Process::Status initializer!

@sodabrew sodabrew closed this Jan 7, 2015
@brixen
Copy link
Member

brixen commented Jan 7, 2015

@sodabrew thanks for working on this and reporting the issues.

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