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

Use flag registry #26

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Use flag registry #26

merged 1 commit into from
Dec 8, 2022

Conversation

headius
Copy link
Contributor

@headius headius commented Apr 18, 2022

Now that there's a pre gem out there are a few additional fixes needed to get things working.

@headius
Copy link
Contributor Author

headius commented Apr 18, 2022

@kou I have merged my PR for JRuby to switch to the gem, and a snapshot build is being generated right now.

The additional patch here is needed to remove some object flags from JRuby core that are specific to StringIO. I have patched around the issue in JRuby master but this will allow me to remove the flags completely.

Once setup-ruby picks up the latest JRuby nightly then we should be able to restart the build and see what fails. I will fix any additional issues in this PR.

@kou
Copy link
Member

kou commented Apr 19, 2022

OK!

@headius
Copy link
Contributor Author

headius commented Apr 19, 2022

Well I am confused. It appears to have picked up the nightly build but it still does not pass the tests.

When I run them locally, they pass:

$ rake test
Ignoring bindex-0.8.1 because its extensions are not built. Try: gem pristine bindex --version 0.8.1
/Users/headius/projects/jruby/bin/jruby run-test.rb
/Users/headius/projects/jruby/lib/ruby/gems/shared/gems/bundler-2.2.19/lib/bundler/vendor/thor/lib/thor/error.rb:105: warning: constant DidYouMean::SPELL_CHECKERS is deprecated
Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead.
/Users/headius/projects/jruby/lib/ruby/gems/shared/gems/power_assert-2.0.1/lib/power_assert.rb:13: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
/Users/headius/projects/jruby/lib/ruby/gems/shared/gems/power_assert-2.0.1/lib/power_assert.rb:13: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
Loaded suite run-test
Started
..........................................................P
=======================================================================================================================================================================================
Pending: test_ractor(TestStringScannerRactor): pended.
/Users/headius/projects/strscan/test/strscan/test_ractor.rb:6:in `setup'
org/jruby/RubyKernel.java:1238:in `catch'
org/jruby/RubyKernel.java:1233:in `catch'
org/jruby/RubyKernel.java:1238:in `catch'
org/jruby/RubyKernel.java:1233:in `catch'
=======================================================================================================================================================================================

Finished in 0.650269 seconds.
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
59 tests, 321 assertions, 0 failures, 0 errors, 1 pendings, 0 omissions, 0 notifications
98.3051% passed
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
90.73 tests/s, 493.64 assertions/s

Are you able to try running them yourself and let me know if they fail?

@headius
Copy link
Contributor Author

headius commented Apr 19, 2022

Ok I think I was wrong.

The JRuby commit that would have strscan as a gem is jruby/jruby@dd44c46. The strscan CI build last used 5ce97a58dc, a previous commit.

I will make sure there's a fresh snapshot build now so that it will be picked up by tomorrow, and then we will re-run tests and see how it looks.

@kou
Copy link
Member

kou commented Dec 8, 2022

@headius I've released 3.0.3 now for CRuby 3.2.0. Should we include this change for the latest stringio release? If so, we should merge this and release 3.0.4 with this change.

@headius
Copy link
Contributor Author

headius commented Dec 8, 2022

Yes I believe we can make this change now. Perhaps we can merge and push a .pre gem so I can test it for JRuby 9.4.1?

Flag registry works based on class, but core cannot reference the
StringIO class now, so we move the flags out to the gem.
@headius headius force-pushed the more_stringio_jruby branch from 377ec22 to ebbc292 Compare December 8, 2022 08:38
@headius
Copy link
Contributor Author

headius commented Dec 8, 2022

I have rebased on master so we can get an updated CI run.

@headius
Copy link
Contributor Author

headius commented Dec 8, 2022

Looks good to me!

@kou kou changed the title Additional tweaks for the JRuby gem Use flag registry Dec 8, 2022
@kou kou merged commit bb69bd4 into ruby:master Dec 8, 2022
@kou
Copy link
Member

kou commented Dec 8, 2022

Thanks!

@kou
Copy link
Member

kou commented Dec 8, 2022

Released.

@headius headius deleted the more_stringio_jruby branch December 9, 2022 00:20
headius added a commit to jruby/jruby that referenced this pull request Dec 9, 2022
This eliminates the StringIO object flags in JRuby core, which
which have moved into the gem.

See ruby/stringio#26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants