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

Import JRuby's strscan #25

Merged
merged 5 commits into from
Apr 15, 2022
Merged

Import JRuby's strscan #25

merged 5 commits into from
Apr 15, 2022

Conversation

headius
Copy link
Contributor

@headius headius commented Sep 3, 2021 •
edited
Loading

Finally getting around to addressing #15. Initial import has gone well but there are a few failures to look into, probably just feature/fix drift in our implementation.

I will fix the issues and make sure this packages properly as a gem, so we can start using it in JRuby 9.4 (equivalent to CRuby 3.0).

TODO:

  • Import and build JRuby strscan impl
  • Add license disclaimer or get sign-off from contributors to relicense
  • Pass tests
  • Add a CI run
  • Package java gem

This was referenced Sep 3, 2021
Open
Closed
@headius headius marked this pull request as draft September 3, 2021 05:18
@headius headius mentioned this pull request Sep 3, 2021
Merged
5 tasks
@kou
Copy link
Member

kou commented Sep 3, 2021

The current CI failures are fixed by 3ceafa6.
Could you rebase on master?

And do you have a plan to add jobs for JRuby to the current CI?

@headius
Copy link
Contributor Author

headius commented Sep 7, 2021

Could you rebase on master?

Thank you, yes I will rebase!

And do you have a plan to add jobs for JRuby to the current CI?

That is the primary reason this is a draft. We need to get all the tests green and then I will push a CI job for JRuby.

@headius
Copy link
Contributor Author

headius commented Sep 7, 2021

Also needs some tweaks to get a -java gem released. I will add a checklist to the PR description.

@headius
Copy link
Contributor Author

headius commented Sep 7, 2021 •
edited
Loading

I have reviewed the current code and we would need confirmation from the following individuals to relicense.

Contributors listed below: please respond in a comment with the following, assuming you consent:

"I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license."

For me:

I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license.

@headius
Copy link
Contributor Author

headius commented Sep 7, 2021

Silly me... I forgot that @knawrocke contributed the original code over email 15 years ago, so I added him to the list.

@knawrocke
Copy link

I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license.

@nicksieger
Copy link

Ohai 👋 !

I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license.

@headius
Copy link
Contributor Author

headius commented Sep 7, 2021

Added @olabini / @olabiniV2 just to cover everyone who touched this code.

@headius headius linked an issue Sep 7, 2021 that may be closed by this pull request
java port #15
Closed
@enebo
Copy link

enebo commented Sep 7, 2021

I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license.

@olabiniV2
Copy link

Hi everyone. I am Ola Bini - the owner of the @olabini account - which I'm currently locked out of. If any legal issues arise, I'm willing to provide offline government identification if necessary, to support this agreement.

I consent to my contributions being relicensed under the Ruby license and the BSD 2-clause license.

@lopex
Copy link

lopex commented Sep 8, 2021

I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license.

@headius headius marked this pull request as ready for review September 24, 2021 04:52
@headius headius marked this pull request as draft September 24, 2021 04:52
@headius headius mentioned this pull request Jan 27, 2022
Closed
@headius headius mentioned this pull request Feb 7, 2022
Merged
@headius headius marked this pull request as ready for review February 7, 2022 23:55
@headius
Copy link
Contributor Author

headius commented Feb 7, 2022

This should now be green on all tests and I am just tweaking build and CI.

Still looking for consent to relicense from @vvs @nahi @kares.

@kares
Copy link

kares commented Feb 9, 2022

👍 for the effort and

I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license.

@tagliala tagliala mentioned this pull request Mar 5, 2022
Merged
11 tasks
@headius
Copy link
Contributor Author

headius commented Apr 13, 2022

I have pinged @vvs and @nahi through other channels to try to get their consent to relicense. If anyone knows them personally, let them know I'm looking for them! 😀

@headius headius mentioned this pull request Apr 13, 2022
Closed
4 tasks
@nahi
Copy link
Member

nahi commented Apr 13, 2022

I didn't know I did something for the file. Long history...
I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license.

@vvs
Copy link

vvs commented Apr 14, 2022

Hi there, fellas, long time no see!

"I consent to having my contributions relicensed under the Ruby license and the BSD 2-clause license."

@headius
Copy link
Contributor Author

headius commented Apr 14, 2022

Thank you @nahi and @vvs (and good to hear from you both!

With that, all contributors to this code have given consent to relicense!

@kou
Copy link
Member

kou commented Apr 15, 2022

Great!!!

@headius Could you confirm the test failure only on Windows + JRuby?
https://github.com/ruby/strscan/runs/5120568110?check_suite_focus=true

================================================================================
Failure: test_AREF(TestStringScanner):
  Expected Exception(IndexError) was raised, but the message doesn't match.
  Expected /\u{30c6 30b9 30c8}/ to match "undefined name <\u00E3\u0192\u2020\u00E3\u201A\u00B9\u00E3\u0192\u02C6> reference".
  <nil> is not true.
D:/a/strscan/strscan/test/lib/core_assertions.rb:489:in `assert'
D:/a/strscan/strscan/test/lib/core_assertions.rb:460:in `assert_raise_with_message'
D:/a/strscan/strscan/test/strscan/test_stringscanner.rb:468:in `test_AREF'
     465:     assert_equal 'foo', s['a']
     466:     assert_equal 'bar', s['b']
     467:     assert_raise(IndexError) { s['c'] }
  => 468:     assert_raise_with_message(IndexError, /\u{30c6 30b9 30c8}/) { s["\u{30c6 30b9 30c8}"] }
     469:   end
     470: 
     471:   def test_pre_match
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'
================================================================================

Note: "\u{30c6 30b9 30c8}" is "テスト" (Japanese) ("test" in English)

Is it a test problem? Or is it a JRuby on Windows problem?

@headius
Copy link
Contributor Author

headius commented Apr 15, 2022

Is it a test problem? Or is it a JRuby on Windows problem?

@kou I believe this is a problem specific to Windows, and only affects the error message here (the error is still raised, but due to Windows + Java exception logic and character encoding, the message is encoded incorrectly).

I can open a JRuby issue to fix that but it will not be in a release for a little while. We could skip that test for now, or simplify it for JRuby.

@headius
Copy link
Contributor Author

headius commented Apr 15, 2022

We could skip that test for now, or simplify it for JRuby.

I should also point out that this is the last test in test_AREF, and we pass everything else there. Perhaps the error message is not critical enough to block JRuby?

@headius headius mentioned this pull request Apr 15, 2022
Open
@headius
Copy link
Contributor Author

headius commented Apr 15, 2022

jruby/jruby#7177 filed for the Windows error message encoding failure.

@kou
Copy link
Member

kou commented Apr 15, 2022

OK.
I'll merge this. Thanks!

@kou kou merged commit 0687ebd into ruby:master Apr 15, 2022
@headius headius deleted the jruby branch April 18, 2022 15:28
@headius
Copy link
Contributor Author

headius commented Apr 18, 2022

@kou Can we get a preview gem for this one too please?

@kou
Copy link
Member

kou commented Apr 19, 2022

Sure!
Done.

headius added a commit to headius/jruby that referenced this pull request Apr 19, 2022
@headius
Switch strscan to gem v3.0.2.pre1
57c6ea6 
See ruby/strscan#25
@headius
Copy link
Contributor Author

headius commented Apr 19, 2022

I have updated JRuby master to use the strscan gem! That change will get picked up in the next nightly snapshot.

@kou
Copy link
Member

kou commented Apr 19, 2022

Thanks!
I'll re-run CI tomorrow.

@kou
Copy link
Member

kou commented Apr 21, 2022

CI sill works except jruby/jruby#7177 : https://github.com/ruby/strscan/actions/runs/2186790823

eregon
eregon reviewed Apr 21, 2022
View reviewed changes
lib/strscan.rb
Comment on lines +1 to +6
if RUBY_ENGINE == 'jruby'
require 'strscan.jar'
JRuby::Util.load_ext("org.jruby.ext.strscan.StringScannerLibrary")
else
require 'strscan.so'
end
Copy link
Member

Choose a reason for hiding this comment

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

This is actually unfortunate, because it would just work on TruffleRuby otherwise, since the strscan.rb in stdlib would win over strscan.so.
Yet another reason for JRuby to support require 'strscan' to load the strscan.jar + the necessary setup.

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, this causes:

bundle exec rake test
/home/eregon/.rubies/truffleruby-dev/bin/truffleruby run-test.rb
<internal:core> core/kernel.rb:236:in `gem_original_require': cannot load such file -- strscan.so (LoadError)
Did you mean?  strscan
	from /home/eregon/code/strscan/lib/strscan.rb:5:in `<top (required)>'
	from <internal:core> core/kernel.rb:234:in `gem_original_require'
	from /home/eregon/code/strscan/test/strscan/test_stringscanner.rb:7:in `<top (required)>'
	from <internal:core> core/kernel.rb:234:in `gem_original_require'
	from run-test.rb:8:in `block in <main>'
	from <internal:core> core/dir.rb:261:in `each'
	from <internal:core> core/dir.rb:261:in `glob'
	from run-test.rb:7:in `<main>'
rake aborted!

while tests ran fine before.

Copy link
Member

Choose a reason for hiding this comment

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

#35 to fix it

eregon added a commit to eregon/strscan that referenced this pull request Apr 21, 2022
@eregon
Add support for TruffleRuby in the gem
1925362 
* This is necessary since lib/strscan.rb was added (ruby#25), as the
  TruffleRuby stdlib strscan.rb is no longer found first by `require "strscan"`.
* Write conditions in a way that any other Ruby implementation would
  simply use its stdlib until it is added explicit support in this gem.
eregon added a commit to eregon/strscan that referenced this pull request Apr 21, 2022
@eregon
Add support for TruffleRuby in the gem
3efd592 
* This is necessary since lib/strscan.rb was added (ruby#25), as the
  TruffleRuby stdlib strscan.rb is no longer found first by `require "strscan"`.
* Write conditions in a way that any other Ruby implementation would
  simply use its stdlib until it is added explicit support in this gem.
@eregon eregon mentioned this pull request Apr 21, 2022
Merged
eregon
eregon reviewed Apr 30, 2022
View reviewed changes
strscan.gemspec
s.files = %w{ext/strscan/extconf.rb ext/strscan/strscan.c}
s.extensions = %w{ext/strscan/extconf.rb}

jruby = true if Gem::Platform.new('java') =~ s.platform or RUBY_ENGINE == 'jruby'
Copy link
Member

@eregon eregon Apr 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is this necessary or just RUBY_ENGINE == 'jruby' would be enough?
(I noticed this line when modifying this file and wondering whether it can be simplified)

Copy link
Member

Choose a reason for hiding this comment

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

@headius What do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think RUBY_ENGINE should be enough. It has been available for a long time.

Copy link
Member

Choose a reason for hiding this comment

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

#36

kou added a commit that referenced this pull request May 1, 2022
@eregon @kou
Add support for TruffleRuby in the gem (#35)
4cdb3ee 
* This is necessary since lib/strscan.rb was added (#25), as the
  TruffleRuby stdlib strscan.rb is no longer found first by `require "strscan"`.
* Write conditions in a way that any other Ruby implementation would
  simply use its stdlib until it is added explicit support in this gem.

Fixes oracle/truffleruby#2420

Co-authored-by: Sutou Kouhei <kou@clear-code.com>
@eregon eregon mentioned this pull request May 3, 2022
Merged
@eregon eregon mentioned this pull request Mar 6, 2023
Open
@kou kou mentioned this pull request Oct 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon left review comments

@kou kou kou left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

java port