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

Import JRuby's stringio impl #21

Merged
merged 10 commits into from
Apr 15, 2022
Merged

Import JRuby's stringio impl #21

merged 10 commits into from
Apr 15, 2022

Conversation

headius
Copy link
Contributor

@headius headius commented Sep 3, 2021

This will eventually import JRuby's stringio implementation. Initial import mirrors how the strscan impl was imported in ruby/strscan#25. I will get the failing tests passing and make sure the gem builds properly for the java platform.

  • Import and build JRuby code
  • Add license disclaimer or get permission from contributors to relicense
  • Pass tests
    • Three test failures remain that may not be supportable today on JRuby
  • Add CI job
  • Package java gem

@headius
Copy link
Contributor Author

headius commented Sep 7, 2021

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.

@cheald
Copy link

cheald commented Sep 7, 2021

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

1 similar comment
@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.

@eregon
Copy link
Member

eregon commented Sep 7, 2021

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

@dragonsinth
Copy link

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

Also.. wtf.. LOL

@headius
Copy link
Contributor Author

headius commented Sep 8, 2021

@dragonsinth Thank you for your contribution! 😀

Copy link

@kares kares left a comment

Choose a reason for hiding this comment

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

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

The fields in JRuby must be deleted since the StringIO class will
move out of the codebase. Do not depend on them.
Enumerator methods like #take will bail out before returning
normally from the yield, avoiding the position increment.
Most of the fixes here are of the following types:

* Encoding handling in StringIO construction
* Proper checking for open/closed/read/write
* Style fixes to arity-split more methods

The three remaining excludes are as follows:

* Two that test overflow conditions that are different or not
  relevant on JRuby's JVM-based String
* One where we fail to reject trancoding UTF-8 to Windows-31J,
  somewhere in mixed-encoding negotiation logic
@headius
Copy link
Contributor Author

headius commented Feb 9, 2022

I have pushed an update to stringio that fixes all but three failures, adds JRuby to CI, rebases on current master, and tweaks Rakefile and Gemfile for the JRuby extension.

The remaining three failures may need to be masked out.

  • Two are testing overflow conditions near LONG_MAX that do not work properly on JRuby due to the JVM's INT_MAX size limit for arrays (such as the byte[] backing String).
  • One tests encoding negotiation, and JRuby fails because we do not reject converting a UTF-8 sushi emoji into Windows-31J. This is not a bug in the stringio code... it is a bug in JRuby I have been unable to locate.

Testing this will require a new jruby-head build until we have a preview gem we can use.

Still waiting for consent to relicense from @orikremer.

I'm going to remove the draft from this because I'd like to get some input from the maintainers.

@headius headius marked this pull request as ready for review February 9, 2022 16:52
@kou
Copy link
Member

kou commented Mar 30, 2022

Thanks.

I confirmed this with jruby Docker image https://hub.docker.com/_/jruby .

It seems that we have a bootstrap issue. rake-compiler requires yaml https://github.com/rake-compiler/rake-compiler/blob/master/lib/rake/baseextensiontask.rb#L11 and yaml requires stringio but stringio.jar isn't built yet.

I can fix this problem because I'm a rake-compiler maintainer. I'll do it later.

Can we build stringio-*-java.gem by GitHub Actions? It's very helpful for release process. If we can build stringio-*-java.gem by GitHub Actions, we can download and gem push it to release a new version. We don't need to prepare JRuby and javac environment (or rake-compiler-dock environment) on local.

@headius
Copy link
Contributor Author

headius commented Apr 13, 2022

I'm still searching for @orikremer.

@headius
Copy link
Contributor Author

headius commented Apr 13, 2022

Can we build stringio-*-java.gem by GitHub Actions?

@kou Are we doing that for the regular gem right now? It's a good idea to have it build, and when we tag off a release we'll just download and push the built gems.

@headius headius mentioned this pull request Apr 13, 2022
4 tasks
@orikremer
Copy link

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

@orikremer Thank you!

All contributors have now given consent to relicense!

This will allow saving the built gem (mostly for JRuby) so it can
be uploaded without requiring a local Java install.
@headius
Copy link
Contributor Author

headius commented Apr 14, 2022

@kou I pushed a commit that builds the .gem file as the last step of successful GHA workflow, and indeed ran into the rake-compiler stringio.jar issue. Other than that I think it should work fine and saving the pkg dir will allow you to download the built gem.

How would you like to proceed?

kou added a commit to rake-compiler/rake-compiler that referenced this pull request Apr 15, 2022
Because yaml requires stringio implicitly. If stringio is required, we
can't use rake-compiler for stringio.

See also: ruby/stringio#21 (comment)
@kou
Copy link
Member

kou commented Apr 15, 2022

@headius Thanks! I've released rake-compiler with a fix for this.

Now, we can build and test with JRuby.

There are some warnings and tests failures/errors: https://github.com/ruby/stringio/runs/6034412008?check_suite_focus=true

Should we fix them in this pull request or defer them for follow-up pull requests?

For building gem in CI, we need to use https://github.com/actions/upload-artifact or https://github.com/softprops/action-gh-release to upload the built gem. I can take over it because I did it in other projects.

@kou
Copy link
Member

kou commented Apr 15, 2022

@hsbt Could you add me to https://rubygems.org/gems/stringio 's owners to push a new release?

@hsbt
Copy link
Member

hsbt commented Apr 15, 2022

@kou Sure, I send an invitation to you.

@kou
Copy link
Member

kou commented Apr 15, 2022

@hsbt Thanks! Accepted.

@headius
Copy link
Contributor Author

headius commented Apr 15, 2022

Should we fix them in this pull request or defer them for follow-up pull requests?

@kou I believe these are failing because jruby-head still ships its own built-in stringio and will not honor the version from the gem. I mentioned that in this comment and noted that three of these tests will probably not be fixable on JRuby before release (unsupported or broken due to unrelated JRuby issues).

We will need to push a preview gem for jruby-head to start using the gem. We can do that before this PR is merged if you would like to see it closer to green first! We have done that before with io-wait to allow testing jruby-head (cc @hsbt).

@kou
Copy link
Member

kou commented Apr 15, 2022

OK.
I"ll merge this and release a pre gem.

@kou kou merged commit cbbad15 into ruby:master Apr 15, 2022
@kou
Copy link
Member

kou commented Apr 15, 2022

headius added a commit to headius/jruby that referenced this pull request Apr 18, 2022
headius added a commit to headius/jruby that referenced this pull request Apr 18, 2022
@headius headius deleted the jruby branch April 18, 2022 15:26
@headius
Copy link
Contributor Author

headius commented Apr 18, 2022

I have pushed a PR for JRuby to use the gem: jruby/jruby#7178

Note most of these suites fail because we are finishing 2.7-3.1 features currently. When this is complete and doesn't regress, I will push a snapshot build that should get picked up by the setup-ruby process after tonight.

headius added a commit to headius/jruby that referenced this pull request Apr 18, 2022
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.

9 participants