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.

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

Update ripper_supported? for truffleruby and ripper specs #395

Merged
merged 1 commit into from Jan 14, 2020

Conversation

@bjfish
Copy link
Contributor

bjfish commented Jan 10, 2020

and allow ripper specs to produce exceptions.

@@ -101,7 +105,7 @@ def supports_taint?
end
ripper_requirements = [ComparableVersion.new(RUBY_VERSION) >= '1.9.2']

ripper_requirements.push(false) if Ruby.rbx?
ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?

This comment has been minimized.

Copy link
@eregon

eregon Jan 10, 2020

This is not ideal, because TruffleRuby might support Ripper at some point.
How about checking Ripper.respond_to?(:lex) which is what https://github.com/k0kubun/hamlit does for instance?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 10, 2020

Member

You could push that onto the stack for if run truffle ruby?

This comment has been minimized.

Copy link
@bjfish

bjfish Jan 10, 2020

Author Contributor

@eregon I think there would be an issue with this because part of the spec asserts Ripper would not be loaded by feature detection:

it 'does not load Ripper' do
expect { RubyFeatures.ripper_supported? }.not_to change { defined?(::Ripper) }
end

This comment has been minimized.

Copy link
@eregon

eregon Jan 10, 2020

I see, that is unfortunate. Probably fine to keep as-is then and change it again (e.g., to a version check) once TruffleRuby supports Ripper.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 12, 2020

Member
Suggested change
ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?
ripper_requirements.push(defined?(::Ripper) && Ripper.respond_to?(:lex)) if Ruby.truffleruby?

This comment has been minimized.

Copy link
@eregon

eregon Jan 14, 2020

We require it if ripper is supported,

Right, so the whole point of this logic is then avoiding require 'ripper' if Ripper is unsupported.
Is that to avoid changing $LOADED_FEATURES or making sure code which uses Ripper still require 'ripper', but only for Rubies with unsupported Ripper?

I would suggest something a lot more straightforward by always requiring Ripper, then we wouldn't need to have so much Ruby-implementation-specific code here.

on recent MRI it's already loaded

I don't see that:

$ ruby -ve 'puts $".grep(/ripper/i)'
ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-linux]

In practise it would mean that you'd have to have require it on truffle ruby if you wanted to use it and it would work.

I wouldn't want that, I'd want the same behavior as MRI.
Since anyway this does not seem a critical feature of RSpec, I propose to adapt the code here once TruffleRuby supports Ripper and merge the PR as it is.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 14, 2020

Member

Right, so the whole point of this logic is then avoiding require 'ripper' if Ripper is unsupported. Is that to avoid changing $LOADED_FEATURES or making sure code which uses Ripper still require 'ripper', but only for Rubies with unsupported Ripper?

It's to avoid requiring ripper if possible.

I would suggest something a lot more straightforward by always requiring Ripper, then we wouldn't need to have so much Ruby-implementation-specific code here.

We do the least requiring we can to avoid false positives due to poisoned environments.

I don't see that:

I stand corrected, irb requires it, ruby does not.

I wouldn't want that, I'd want the same behavior as MRI.

In my experience only MRI behaves like MRI, other Ruby engines always have quirks. 😢

This comment has been minimized.

Copy link
@eregon

eregon Jan 14, 2020

In my experience only MRI behaves like MRI, other Ruby engines always have quirks.

FWIW, TruffleRuby tries hard to behave exactly like MRI. But implementing Ripper is a significant effort, so that's not done yet.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 14, 2020

Member

To be fair I should probably say that every engine has quirks, it's just the MRI ones are the reference quirks? 🤷‍♂

This comment has been minimized.

Copy link
@eregon

eregon Jan 14, 2020

I just wanted to end on a positive note :)
This PR is not the most beautiful code but TruffleRuby otherwise runs RSpec (and Rails nowadays) unmodified with no TruffleRuby-specific code since many years now.
We try hard to avoid any TruffleRuby-specific code in gems.

spec/rspec/support/ruby_features_spec.rb Outdated Show resolved Hide resolved
!sexp.nil?
rescue
false
end

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 10, 2020

Member

I'd rather we didn't attempt to run this if ripper isn't available.

This comment has been minimized.

Copy link
@bjfish

bjfish Jan 10, 2020

Author Contributor
@@ -101,7 +105,7 @@ def supports_taint?
end
ripper_requirements = [ComparableVersion.new(RUBY_VERSION) >= '1.9.2']

ripper_requirements.push(false) if Ruby.rbx?
ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 10, 2020

Member

You could push that onto the stack for if run truffle ruby?

@bjfish bjfish force-pushed the bjfish:update-ripper-helper-and-spec branch from 88aee8b to e15e4a8 Jan 10, 2020
@@ -145,7 +145,7 @@ def ripper_can_parse_source_including_keywordish_symbol?
end

it 'returns whether Ripper is correctly implemented in the current environment' do
expect(RubyFeatures.ripper_supported?).to eq(ripper_is_implemented? && ripper_works_correctly?)
expect(RubyFeatures.ripper_supported?).to eq(RubyFeatures.ripper_supported? && ripper_is_implemented? && ripper_works_correctly?)

This comment has been minimized.

Copy link
@pirj

pirj Jan 11, 2020

Member
expect(RubyFeatures.ripper_supported?).to eq(RubyFeatures.ripper_supported?

That looks odd quite frankly :D

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 12, 2020

Member

This should be reverted, the check here is that ripper_supported? matches the two test definitions, are you saying the test definitions return true here? As that should be impossible if ripper is broken on truffleruby. Whats actually happening?

This comment has been minimized.

Copy link
@bjfish

bjfish Jan 13, 2020

Author Contributor

@JonRowe On truffleruby, a runtime error occurs when running ripper_is_implemented? && ripper_works_correctly? which is why I suggested the begin/rescue change before to assert that running these results in error when RubyFeatures.ripper_supported? is false.

Per your comment: #395 (comment). I updated to not run this spec if RubyFeatures.ripper_supported? is false.

@pirj I agree this looked odd. I've pushed a new version to be more explicit in excluding this spec from running using: , :if => ripper_supported?. Please review.

@@ -101,7 +105,7 @@ def supports_taint?
end
ripper_requirements = [ComparableVersion.new(RUBY_VERSION) >= '1.9.2']

ripper_requirements.push(false) if Ruby.rbx?
ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 12, 2020

Member
Suggested change
ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?
ripper_requirements.push(defined?(::Ripper) && Ripper.respond_to?(:lex)) if Ruby.truffleruby?
@@ -145,7 +145,7 @@ def ripper_can_parse_source_including_keywordish_symbol?
end

it 'returns whether Ripper is correctly implemented in the current environment' do
expect(RubyFeatures.ripper_supported?).to eq(ripper_is_implemented? && ripper_works_correctly?)
expect(RubyFeatures.ripper_supported?).to eq(RubyFeatures.ripper_supported? && ripper_is_implemented? && ripper_works_correctly?)

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 12, 2020

Member

This should be reverted, the check here is that ripper_supported? matches the two test definitions, are you saying the test definitions return true here? As that should be impossible if ripper is broken on truffleruby. Whats actually happening?

@bjfish bjfish force-pushed the bjfish:update-ripper-helper-and-spec branch from e15e4a8 to e183117 Jan 13, 2020
@@ -123,6 +123,10 @@ def ripper_works_correctly?
ripper_can_parse_source_including_keywordish_symbol?
end

def self.ripper_supported?
RubyFeatures.ripper_supported?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jan 13, 2020

Member

This is the subject under test, its not a valid way to see if ripper is supported..

@JonRowe
Copy link
Member

JonRowe commented Jan 13, 2020

This should be reverted, the check here is that ripper_supported? matches the two test definitions, are you saying the test definitions return true here? As that should be impossible if ripper is broken on truffleruby. Whats actually happening?

Github has lost the reply to this somewhere, whats the runtime error from those two detection methods? Is that what the rescue false was for originally? Out of those two solutions I'd prefer you rescue the specific exception with a note about truffle-ruby.

The main thing I wanted you to change was to stop the reliance on RubyFeatures.ripper_supported? to run the test for RubyFeatures.ripper_supported?. Its basically preventing the test from checking for false positives.

@bjfish bjfish force-pushed the bjfish:update-ripper-helper-and-spec branch 2 times, most recently from 41872c7 to efdce64 Jan 13, 2020
@bjfish
Copy link
Contributor Author

bjfish commented Jan 13, 2020

@JonRowe I've updated to add the begin;rescue with a note for truffleruby and removed the dependency on RubyFeatures.ripper_supported? to run the test. Thanks.

Copy link
Member

JonRowe left a comment

I'm sorry for the constant back and forth, I think I've misunderstood your problem a couple of times...

spec/rspec/support/ruby_features_spec.rb Outdated Show resolved Hide resolved
@bjfish bjfish force-pushed the bjfish:update-ripper-helper-and-spec branch from efdce64 to 6ae5540 Jan 13, 2020
@bjfish
Copy link
Contributor Author

bjfish commented Jan 13, 2020

@JonRowe I've updated ripper_is_implemented? with your suggestion, thanks.

Copy link
Member

JonRowe left a comment

Thanks! Apologies again for the lack of clear direction!

@eregon
eregon approved these changes Jan 13, 2020
Copy link

eregon left a comment

Looks good to me too.

Copy link
Member

JonRowe left a comment

Whilst we don't officially support Truffle Ruby, if this makes it possible to run our specs with it I'm ok with it.

@JonRowe JonRowe merged commit 54f601f into rspec:master Jan 14, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
JonRowe added a commit that referenced this pull request Jan 14, 2020
@bjfish bjfish deleted the bjfish:update-ripper-helper-and-spec branch Jan 14, 2020
JonRowe added a commit that referenced this pull request May 2, 2020
Update ripper_supported? for truffleruby and ripper specs
JonRowe added a commit that referenced this pull request May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.