Add FFI::Platform::IS_MAC constant #2096

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants

MRI FFI contains a number of constants missing from RBX. This
commit adds IS_MAC to the case statement defining Platform const.

TODO: add IS_GNU, IS_LINUX, IS_FREEBSD, IS_OPENBSD, IS_BSD

this should resolve #2095

Owner

jc00ke commented Dec 11, 2012

Are there specs for these constants?

Owner

dbussink commented Dec 11, 2012

If we're adding these constants, we should also include specs. I'm also not a fan of implementing this half. We should also add the other constants with specs and not leave that as a TODO.

Owner

brixen commented Dec 11, 2012

@sempervictus you could look at this commit for examples on the specs.

RageLtMan added some commits Dec 11, 2012

RageLtMan Add FFI::Platform::IS_MAC constant
MRI FFI contains a number of constants missing from RBX. This
commit adds IS_MAC to the case statement defining Platform const.

TODO: add IS_GNU, IS_LINUX, IS_FREEBSD, IS_OPENBSD, IS_BSD
04c3b6e
RageLtMan add spec for FFI::Platform::IS_MAC 6c27466

If this spec fits the bill lets commit IS_MAC in this PR and i'll add appropriate requests for Linux and other BSD based platforms in a separate PR.

@brixen brixen commented on an outdated diff Dec 16, 2012

spec/ruby/optional/ffi/platform_spec.rb
@@ -40,6 +40,26 @@
end
end
+describe "FFI::Platform::IS_MAC" do
+ platform_is :linux do
+ it "returns false" do
+ FFI::Platform::IS_MAC.should == false
+ end
+ end
+
+ platform_is :windows do
+ it "returns true" do
+ FFI::Platform::IS_MAC.should == false
@brixen

brixen Dec 16, 2012

Owner

The spec description ("returns true") and the expectation disagree.

@brixen brixen commented on an outdated diff Dec 16, 2012

spec/ruby/optional/ffi/platform_spec.rb
+describe "FFI::Platform::IS_MAC" do
+ platform_is :linux do
+ it "returns false" do
+ FFI::Platform::IS_MAC.should == false
+ end
+ end
+
+ platform_is :windows do
+ it "returns true" do
+ FFI::Platform::IS_MAC.should == false
+ end
+ end
+
+ platform_is :darwin do
+ it "returns false" do
+ FFI::Platform::IS_MAC.should == true
@brixen

brixen Dec 16, 2012

Owner

Same problem here.

RageLtMan Add platform constants for Linux and BSD
This commit creates constants and specs for the remaining
FFI::Platform::IS_LINUX constant.

Additional constants have been added for compatibility with MRI's
FFI gem: IS_GNU, NAME, IS_FREEBSD, IS_OPENBSD, IS_BSD.

BSD constants are pointless since RBX does not currently check for
BSD platforms other than OSX. IS_BSD is the only constant which can
be true at the moment since IS_MAC can be true as well.
IS_GNU should be reviewed and potentially spec'ed as it currently
checks definition of FFI::Library::LIBC for its boolean value which
may not be the correct approach. MRI implementation checked for
the definition of "GNU_LIBC" constant which appears analogous to
our FFI::Library::LIBC.
0f12495

Thank you sir - nice to know i am actually capable of screwing that up :).

Fixed and added the remaining constants to this PR since aside from IS_LINUX the rest seem pointless at present. Does my take on IS_GNU work, or should i rewire that and spec it somehow?

@subwindow subwindow commented on the diff Dec 16, 2012

kernel/platform/ffi.rb
OS = 'linux'
end
ARCH = Rubinius::CPU
+ NAME = "#{ARCH}-#{OS}"
+ IS_GNU = defined?(FFI::Library::LIBC)
+ IS_FREEBSD = OS == "freebsd"
@subwindow

subwindow Dec 16, 2012

Member

I don't think this is right, because IS_LINUX and IS_.*BSD can be true at the same time.

I would suggest adding Rubinius.bsd? to the case statement and doing the distinguishing between free/open inside that block.

Thi The only logic change i made was to define IS_GNU via FFI::Library::LIBC, the logic for the BSD definitions comes from MRI's FFI gem (though the validations are a bit different). Not sure about the semantics of BSD being Linux, though i'm pretty sure that they are mutually exclusive definitions.

This may all be a moot point if Brixen gets FFI to work in tandem with Rubinius's low level implementation as we'll just have access to their constants.

Owner

brixen commented Jan 9, 2013

We now bundle a 1.2.0 version of the FFI gem, however, there is a bug in RVM that omits the pre-installed gems. So if using RVM, use the recently released 1.3.0 FFI gem which builds correctly on Rubinius.

brixen closed this Jan 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment