Skip to content

Commit

Permalink
Ruby 2.4 fix.
Browse files Browse the repository at this point in the history 
Making conditional a platform check because it relies on Ruby classes
that don't exist in newer versions of Ruby.
  • Loading branch information
@enkessler
enkessler committed Jan 22, 2017
1 parent ae1c9f0 commit 2a884de
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions lib/childprocess.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,14 @@ def arch
host_cpu = RbConfig::CONFIG['host_cpu'].downcase
case host_cpu
when /i[3456]86/
# Darwin always reports i686, even when running in 64bit mod
if os == :macosx && 0xfee1deadbeef.is_a?(Fixnum)

# Ruby 2.4 unifies Bignum and Fixnum into Integer. Also, I've heard that Darwin no
# longer has this issue, so on newer Ruby/Darwin combos this check shouldn't be
# needed anyway. Leaving it here for older combinations, however.

# Check for Ruby older version of Ruby
if (RUBY_VERSION =~ /^[123]\./) && (os == :macosx) && (0xfee1deadbeef.is_a?(Fixnum))
# Darwin always reports i686, even when running in 64bit mod
"x86_64"
else
"i386"
Expand Down

4 comments on commit 2a884de

@ms-ati
Copy link
Contributor

@ms-ati ms-ati commented on 2a884de Feb 6, 2017

Choose a reason for hiding this comment

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

I notice there's no test coverage here. Does the RUBY_VERSION check say "it starts with either 1, 2, or 3, followed by a dot"?

If so, are you sure that's what you meant to check? Or is there an OS X Ruby version in existence whose version starts with 4?

@enkessler
Copy link
Owner Author

Choose a reason for hiding this comment

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

Whoops! My headspace must have been over in Gherkin land (which released 4.0 some months back). That should be a check for Ruby <2.4 (/^1|2.[123]/ or something like that).

There's no testing for it due to not having anything with OSX to test on. I develop on a Windows/Unix dual boot and the CI build on TravisCI is Unix (I think).

Good catch! Feel free to patch and PR. Otherwise I'll get on it in a day or so.

@ms-ati
Copy link
Contributor

@ms-ati ms-ati commented on 2a884de Feb 6, 2017

Choose a reason for hiding this comment

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

Rather than just send a narrow patch, for a somewhat vital infrastructure library like this, it might be better for the community to add Travis matrix coverage on Mac.

I could take a stab at that -- first getting a test which fails in a Travis build, then the fix -- if you agree with the value and don't get to it before me?

@enkessler
Copy link
Owner Author

Choose a reason for hiding this comment

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

For some reason, I was thinking that Travis didn't offer an OSX environment but, now that you mention it, it looks like it does (now if only they'd offer a Windows environment as well...). That being the case, yes, I agree that adding OSX to the CI build is the thing to do.

Please sign in to comment.