ruby / strscan

Provides lexical scanning operations on a String.
BSD 2-Clause "Simplified" License
81 stars 32 forks source link

Fix a crash when `named_captures` called on new instance #61

Closed okuramasafumi closed 1 year ago

okuramasafumi commented 1 year ago
StringScanner.new('foo').named_captures

This commit fixes a crash in the case above.

okuramasafumi commented 1 year ago

CI fails since a new test case for crash was added.

okuramasafumi commented 1 year ago

@kou Thank you, I've fixed it. https://github.com/ruby/strscan/blob/master/.github/workflows/ci.yml#L62 This line specifies Ruby version as 3.1, but should we add 3.2 to avoid CI failures?

kou commented 1 year ago

Could you also add a NULL check to the implementation.

Oh, sorry. I wanted to say "Could you also add a NULL check to the JRuby implementation?"

https://github.com/ruby/strscan/blob/master/.github/workflows/ci.yml#L62 This line specifies Ruby version as 3.1, but should we add 3.2 to avoid CI failures?

It's better but it's not related to the CI failures.

okuramasafumi commented 1 year ago

@kou OK, so I changed JRuby version as well and run-test to use locally installed version. I'm not sure if these fixes are ideal but CI now passes. Could you check it, thanks!

okuramasafumi commented 1 year ago

The output from gem install --verbose --backtrace pkg/*.gem.

3.0 https://github.com/ruby/strscan/actions/runs/4171325802/jobs/7221150111

["/opt/hostedtoolcache/Ruby/3.0.5/x64/bin/ruby", "-I", "/opt/hostedtoolcache/Ruby/3.0.5/x64/lib/ruby/site_ruby/3.0.0", "extconf.rb"]

3.1 https://github.com/ruby/strscan/actions/runs/4171325802/jobs/7221150216

["/opt/hostedtoolcache/Ruby/3.1.3/x64/bin/ruby", "-I", "/opt/hostedtoolcache/Ruby/3.1.3/x64/lib/ruby/site_ruby/3.1.0", "extconf.rb"]

3.2 https://github.com/ruby/strscan/actions/runs/4171325802/jobs/7221150323

["/opt/hostedtoolcache/Ruby/3.2.1/x64/bin/ruby", "-I", "/opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/3.2.0", "extconf.rb"]

It looks like 3.2 include path is different from 3.1 and 3.0 that are successful. This is might be related to this problem.

tenderlove commented 1 year ago

What do we need to do to merge this? I ran in to the same bug. 😅

kou commented 1 year ago

We need to debug a problem that Ruby 3.2 and Ruby master doesn't use gem install-ed strscan. But we can work on the problem as a separated pull request and merge this as-is.

hsbt commented 1 year ago

FYI: I fixed installation failure related C-ext default gems https://github.com/rubygems/rubygems/pull/6430.

I'm not sure this fix resolve issue of this PR.

kou commented 1 year ago

Oh, thanks for the info. Can we try the change in this repository?

kou commented 1 year ago

I merge this for now. We can work on the CI failures in a separated pull request.