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.

Sign up for GitHub

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

Jump to bottom

Support ruby 1.9.3 #23

Merged
merged 4 commits into from
Aug 15, 2017
Merged

Support ruby 1.9.3 #23

merged 4 commits into from
Aug 15, 2017

Conversation

jabley
Copy link
Contributor

@jabley jabley commented Jul 29, 2017

Despite 1.9 being EOL, some people still need it. This changes adds
support for that. The main issue was dir is a 2.x feature.

@jabley jabley mentioned this pull request Jul 29, 2017
Merged
@jabley
Copy link
Contributor Author

jabley commented Jul 29, 2017

OK, so it's not as straightforward as I'd hoped. Would you be interested in this change if I can get it working?

@jabley jabley force-pushed the feature/support-1.9 branch from 59857cc to d5ca7d2 Compare July 29, 2017 10:00
@jabley
Copy link
Contributor Author

jabley commented Jul 29, 2017

I have the tests passing, but I'm not sure what I've done is correct. The ruby issue that introduced rb_thread_call_without_gvl talks about it being in internal.h and attempts to use it cause a compiler warning.

miyucy
miyucy reviewed Jul 29, 2017
View reviewed changes
ext/brotli/extconf.rb Outdated
# Gem should support wide range of ruby versions
def _dir
File.dirname(__FILE__)
end
Copy link
Owner

Choose a reason for hiding this comment

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

I think this func does not need. just write File.dirname(__FILE__) as like spec file.

@miyucy
Copy link
Owner

miyucy commented Jul 29, 2017

I have the tests passing, but I'm not sure what I've done is correct. The ruby issue that introduced rb_thread_call_without_gvl talks about it being in internal.h and attempts to use it cause a compiler warning.

Oh, I'm don't know too. It scary...
I want do not give without_gvl feature to Ruby v1.9. Because, 1.9 is EOL (as you know) so if strange behavier (or uncaught bug) was found anyone can not fix it.
So, I want to safty. What do you think?

@jabley
Support ruby 1.9.3
9489553 
Despite 1.9 being EOL, some people still need it. This changes adds
support for that. The main issue was __dir__ is a 2.x feature.
@jabley jabley force-pushed the feature/support-1.9 branch from d5ca7d2 to 9489553 Compare July 30, 2017 09:56
@jabley
Copy link
Contributor Author

jabley commented Jul 30, 2017

I think not supporting 1.9 is a reasonable position, since it's been EOL for a number of years. People that still have to use 1.9 should be encouraged to upgrade.

@miyucy
Copy link
Owner

miyucy commented Jul 30, 2017

Ah, sorry for Misleading response.
I want to support 1.9 as possible.
But, I wantn't to introduce without_gvl function in 1.9.
How about this? jabley#1

jabley added 2 commits July 31, 2017 12:39
@jabley
Merge pull request #1 from miyucy/support-1.9
931813f 
Invoke directly
@jabley
Invoke directly without rb_thread_call_without_gvl
03041a1 
rb_thread_call_without_gvl is an internal API in Ruby 1.9, and was only
made part of ruby/thread.h in 2.x.

To support 1.9, we instead call the function directly. This is deemed
less risky than relying on an unexported, internal API from the EOL Ruby
1.9.x code base.
@miyucy miyucy merged commit 43d23bf into miyucy:master Aug 15, 2017
@jabley jabley deleted the feature/support-1.9 branch August 16, 2017 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miyucy miyucy miyucy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jabley @miyucy