-
Notifications
You must be signed in to change notification settings - Fork 605
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
Conversation
Regarding LLVM, what problems are you having? Is it complaining |
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 |
I believe you need to install llvm via Homebrew, see rbenv/ruby-build@4aeeac2. |
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). |
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 :) |
@sodabrew @yorickpeterse is there a value exposed to Ruby in MRI that would require us to use bit-fields for compatibility? |
@sodabrew I'm not opposed to your approach but could you please reapply on master, force push, so we can clearly evaluate the changes? |
840095d
to
2bcec5b
Compare
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 Thanks for the quick work on the Process::Status initializer! |
@sodabrew thanks for working on this and reporting the issues. |
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.