Fix to build bzip2-ruby with Rubinius. #14

Merged
merged 1 commit into from Sep 12, 2012

Projects

None yet

2 participants

@brixen

This code is not supportable on Rubinius and is generally very bad practice. Bzip2 does
not own the Ruby class data structures and should not make any assumptions about them.

In this case, the code appears entirely unnecessary because the value is created as
a String in the first place.

I'm not sure what is the best fix, but this would at least allow compiling on Rubinius.

FWIW, there are numerous bad practices in this C extension, such as relying on MRI
C globals like rb_rs instead of getting the value of the $/ Ruby global and depending
on the internals of RFile. I've added basic RFile support to Rubinius just to support
building this C-extension, but there are limits to what I can do.

@brixen brixen Don't poke inside private MRI data structures.
This code is not supportable on Rubinius and is generally very bad practice. Bzip2 does
not own the Ruby class data structures and should not make any assumptions about them.

In this case, the code appears entirely unnecessary because the value is created as
a String in the first place.

I'm not sure what is the best fix, but this would at least allow compiling on Rubinius.

FWIW, there are numerous bad practices in this C extension, such as relying on MRI
C globals like rb_rs instead of getting the value of the $/ Ruby global and depending
on the internals of RFile. I've added basic RFile support to Rubinius just to support
building this C-extension, but there are limits to what I can do.
6cf4483
@brianmario
@brixen

@brianmario yeah, not saying it's your fault, more documenting it in case someone benefits.

Any chance of releasing a new gem in the near future?

@brianmario
@brixen

@brianmario thank you!

@brianmario
Owner

wow, this fell completely off my radar... sorry @brixen!

@brianmario brianmario merged commit 58a9107 into brianmario:master Sep 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment