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

Update ripper_supported? for truffleruby and ripper specs #395

Merged
merged 1 commit into from Jan 14, 2020
Merged

Update ripper_supported? for truffleruby and ripper specs #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/rspec/support/ruby_features.rb
View file
Open in desktop
Expand Up @@ -47,6 +47,10 @@ def non_mri?
def mri?
!defined?(RUBY_ENGINE) || RUBY_ENGINE == 'ruby'
end

def truffleruby?
defined?(RUBY_ENGINE) && RUBY_ENGINE == 'truffleruby'
end
end

# @api private
Expand Down Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

rspec-support/spec/rspec/support/ruby_features_spec.rb

Lines 151 to 153 in b83b4ce

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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. 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


if Ruby.jruby?
ripper_requirements.push(Ruby.jruby_version >= '1.7.5')
Expand Down
2 changes: 1 addition & 1 deletion spec/rspec/support/ruby_features_spec.rb
View file
Open in desktop
Expand Up @@ -111,7 +111,7 @@ def ripper_is_implemented?
in_sub_process_if_possible do
begin
require 'ripper'
!!defined?(::Ripper)
!!defined?(::Ripper) && Ripper.respond_to?(:lex)
rescue LoadError
false
end
Expand Down