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

Remove JRuby ext and use TracePoint #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

headius
Copy link

@headius headius commented Oct 2, 2024

JRuby works fine with the TracePoint logic for many years now, so there's no reason to ship an extension.

JRuby works fine with the TracePoint logic for many years now, so
there's no reason to ship an extension.
@headius
Copy link
Author

headius commented Oct 2, 2024

This does not pass tests because the tests expect that eval "__LINE__", b will use the line number from binding b but that behavior no longer matches Ruby (CRuby 3.0+ fails the same four tests):

$ chruby ruby-3.3
$ rake test
rspec spec -r ./spec/spec_helpers.rb
..F...FF.F

Failures:

  1) Interception should catch the binding on the correct line
     Failure/Error: @exceptions.map{ |e, b| b.eval('__LINE__') }.should == [line]
     
       expected: [48]
            got: [1] (using ==)
     # ./spec/interception_spec.rb:53:in `block (2 levels) in <top (required)>'

  2) Interception should be able to handle NoMethodErrors
     Failure/Error: @exceptions.map{ |e, b| [e] + b.eval('[__LINE__, shoulder, self]') }.should == [[e1, line, :bucket, self]]
     
       expected: [[#<NoMethodError: undefined method `desnrok' for an instance of String>, 116, :bucket, #<RSpec::Exam...eGroups::Interception "should be able to handle NoMethodErrors" (./spec/interception_spec.rb:112)>]]
            got: [[#<NoMethodError: undefined method `desnrok' for an instance of String>, 1, :bucket, #<RSpec::Exampl...eGroups::Interception "should be able to handle NoMethodErrors" (./spec/interception_spec.rb:112)>]] (using ==)
     # ./spec/interception_spec.rb:121:in `block (2 levels) in <top (required)>'

  3) Interception should be able to handle division by 0 errors
     Failure/Error:
           pending "RBX doesn't yet support this",
             :if => defined?(RUBY_ENGINE) && RUBY_ENGINE == 'rbx' do
       
             shoulder = :bucket
       
             begin
               line = __LINE__; 1 / 0
             rescue => e1
               #
             end
     
     ArgumentError:
       wrong number of arguments (given 2, expected 0..1)
     # ./spec/interception_spec.rb:126:in `block (2 levels) in <top (required)>'

  4) Interception should catch exceptions on basic objects
     Failure/Error: @exceptions.map{ |e, b| [e] + b.eval('[self, shoulder, __LINE__]') }.should  == [[e1, foo, :bracket, line]]
     
       expected: [[#<NameError: undefined local variable or method `foops' for an instance of #<Class:0x000000011db7b420>>, #<#<Class:0x000000011db7b420>:0x00000000000b68>, :bracket, 150]]
            got: [[#<NameError: undefined local variable or method `foops' for #<#<Class:0x000000011db7b420>:0x000000011da31bc8>>, #<#<Class:0x000000011db7b420>:0x00000000000b68>, :bracket, 1]] (using ==)
     # ./spec/interception_spec.rb:157:in `block (2 levels) in <top (required)>'

@stdedos
Copy link
Collaborator

stdedos commented Oct 2, 2024

JRuby works fine with the TracePoint logic for many years now, so there's no reason to ship an extension.

At least since https://javadoc.io/static/org.jruby/jruby-complete/1.7.5/org/jruby/ext/tracepoint/TracePoint.html?

So yea, sounds like a safe bet

@stdedos
Copy link
Collaborator

stdedos commented Oct 2, 2024

This does not pass tests because the tests expect that eval "__LINE__", b will use the line number from binding b but that behavior no longer matches Ruby (CRuby 3.0+ fails the same four tests):

expect per-version? 😕

@headius
Copy link
Author

headius commented Oct 10, 2024

expect per-version? 😕

@stdedos Perhaps, or maybe this gem should just ditch support for really old Ruby versions that behave differently?

@stdedos
Copy link
Collaborator

stdedos commented Oct 11, 2024

expect per-version? 😕

@stdedos Perhaps, or maybe this gem should just ditch support for really old Ruby versions that behave differently?

Probably. But "I like not artificially removing support" either.

Also, I'll wait until maintainer actually responds.
It was a "fun" project setting up pipeline - but it's already too much if the maintainer "abandoned" the gem.

@headius
Copy link
Author

headius commented Oct 11, 2024

it's already too much if the maintainer "abandoned" the gem

Yes, it seems a bit quiet. Feel free to ping me if I'm needed again!

@ConradIrwin
Copy link
Owner

Hey both!

Sorry for the slow response here. I've gone ahead and added you both as collaborators on Github. Feel free to merge improvements as you see fit; and if either of you wants to be added to the ruby gem account I can do that too,

Conrad

@stdedos
Copy link
Collaborator

stdedos commented Oct 14, 2024

Hey both!

Sorry for the slow response here. I've gone ahead and added you both as collaborators on Github. Feel free to merge improvements as you see fit; and if either of you wants to be added to the ruby gem account I can do that too,

Conrad

We (I at least) we'd like a little bit your BDFL decision @ConradIrwin wrt old ruby versions and interpreters. I'll @-mention you to the MR (or just stop by yourself).

I've also never released gems (nor do I have an account) - I'd be nice to check / approve them as suitable.

Idk if @headius has tests figured out - if not, I'd like your input wrt them too (or some other Senior Rubist who you know and s/he would like to pass on knowledge).

@ConradIrwin
Copy link
Owner

Happy to drop old versions as people who need them can use an old version of the gem. (Though I'd probably keep support for the last few active versions).

It seems like we may need to just hardcode the line numbers that are correct now (this code is very old, and it's maybe a bit too "clever" to try and get it right).

@stdedos stdedos mentioned this pull request Oct 14, 2024
@headius
Copy link
Author

headius commented Oct 14, 2024

Idk if @headius has tests figured out

I have not looked at the tests other than to confirm that they pass the same with the JRuby extension code removed (i.e. JRuby uses the same TracePoint code as CRuby 2.7+).

@headius
Copy link
Author

headius commented Oct 14, 2024

It seems like we may need to just hardcode the line numbers that are correct now

Agree. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants