8000 Relax version requirement for rubyzip by ioanatia · Pull Request #491 · jruby/warbler · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Relax version requirement for rubyzip #491

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
Merged
merged 6 commits into from
Jun 9, 2021
Merged

Relax version requirement for rubyzip #491

merged 6 commits into from
Jun 9, 2021

Conversation

ioanatia
Copy link
Contributor

closes #490

I haven't actually test that it works with the latest version of rubyzip, but looking at the usage of the gem it looks like it should work.

Not sure whether the CI job will actually install the latest version of the gem or not.

olleolleolle
olleolleolle reviewed Mar 25, 2021
View reviewed changes
@ioanatia @olleolleolle
Update warbler.gemspec
a464ba2 
Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>
@ioanatia ioanatia requested a review from olleolleolle March 25, 2021 16:58
olleolleolle
olleolleolle previously approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member
@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this change!

@olleolleolle
Copy link
Member

The test failures on 9.2.0.0 seem perhaps to be about that, on 9.2 (whereas 9.1 does not have that).

  1) Warbler::Jar in a jar project with a .gemspec compiles included gems when compile_gems is true
     Failure/Error: file_list(%r{gems.*\.class$}).size.should >= 80 # depending on RubyZip version
       expected: >= 80
            got:    45
     # ./spec/warbler/jar_spec.rb:186:in `block in (root)'

@olleolleolle olleolleolle dismissed their stale review April 9, 2021 06:12

The test failures ought to be addressed. Perhaps the test can be relaxed, perhaps only modified.

@ioanatia
Copy link
Contributor Author

I'll check the test failures.

@ioanatia ioanatia force-pushed the ioanatia/upgrade_rubyzip branch from ebc9b28 to 95026c5 Compare May 28, 2021 16:30
@ioanatia
Copy link
Contributor Author
ioanatia commented May 28, 2021 •
edited
Loading

CI failed with:

ERROR:  Could not find a valid gem 'bundler' (< 2), here is why:
          Unable to download data from https://rubygems.org/ - Received fatal alert: handshake_failure (https://api.rubygems.org/specs.4.8.gz)
The command "gem install bundler -v '< 2'" failed and exited with 2 during .

I tried to trigger a rebuild with a git commit --amend --no-edit and git push -f.

@ioanatia ioanatia requested a review from olleolleolle May 28, 2021 16:35
@olleolleolle
Copy link
Member

Someone had an issue a bit like this, here: rubygems/rubygems#4150

@headius Are we committed to keeping 9.0.5.0 in the matrix still?

@ioanatia
Copy link
Contributor Author
ioanatia commented Jun 2, 2021

@olleolleolle @headius I removed 9.0.5.0 from .travis.yml and now the tests are green.
Are there any other changes that need to be made if 9.0.5.0 is removed?

@olleolleolle
Copy link
Member

@ioanatia https://github.com/jruby/warbler#warbler--- the README has a promise of "9.0.0.0 and up", which I believe to be not the case anymore if we fail to run our suite on that range. Could we say, @headius, that we no longer support the < 9.1?

@olleolleolle
Copy link
Member

@ioanatia Awesome that you edited the CI, this was new information, good!

@ioanatia
Copy link
Contributor Author
ioanatia commented Jun 2, 2021 •
edited
Loading

the README has a promise of "9.0.0.0 and up", which I believe to be not the case anymore if we fail to run our suite on that range

Thanks, the README is updated.
@olleolleolle if we go with this approach, should we also add 9.1.0.0 to .travis.yml or having jruby-9.1.17.0 is sufficient?

@olleolleolle
8000
Copy link
Member

@ioanatia I think latest 9.1 is sufficient for the CI.

@ioanatia
Copy link
Contributor Author
ioanatia commented Jun 9, 2021

AFAICS jruby 9.0 is supposed to be compatible with Ruby 2.2, but support for Ruby 2.2 has ended in June 2018.
It might be okay to move forward and remove support for 9.0 in newer versions of warbler @olleolleolle @headius

@deivid-rodriguez
Copy link
Contributor

Technically jruby 9.1 is Ruby 2.3 compatible and support for Ruby 2.3 has also ended. In addition to that, bundler 2 does not really work on jruby 9.1 due to some issues in jruby fixed in the 9.2 series (see ruby/setup-ruby#108).

So maybe it's ok to drop support for jruby 9.1 too in the next version.

@deivid-rodriguez
Copy link
Contributor

Also I just installed jruby 9.1 through ruby-build and I got this message:

WARNING: jruby-9.1.17.0 is past its end of life and is now unsupported.
It no longer receives bug fixes or critical security updates.

It seems official that jruby 9.1 is out of support, so I think it's ok to move on.

@deivid-rodriguez deivid-rodriguez mentioned this pull request Jun 9, 2021
Merged
@enebo
Copy link
Member
enebo commented Jun 9, 2021

We have not supported 9.1 for a long time now. 9.2+ seems reasonable to me unless someone shows up with a convincing story why we should continue to support it.

olleolleolle
olleolleolle approved these changes Jun 9, 2021
View reviewed changes
Copy link
Member
@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

Thanks all, for chiming in! Merging now!

@olleolleolle olleolleolle merged commit 772ac94 into jruby:master Jun 9, 2021
@olleolleolle
Copy link
Member
8000 olleolleolle commented Jun 9, 2021

@ioanatia Thanks for the strong momentum and follow-through!

@ioanatia
Copy link
Contributor Author

Nice! Thanks for merging this @olleolleolle - any chance to have a new release of this gem soon?

@olleolleolle olleolleolle mentioned this pull request Jun 10, 2021
Open
9 tasks
@olleolleolle
Copy link
Member

@ioanatia That's a little longer process, but with @deivid-rodriguez working on a GitHub Actions CI change, and we are at a green build, so if you want to support the process, please investigate what information we have about performing releases. I am new to that process, and am documenting my steps in a coordination Issue linked here.

@deivid-rodriguez
Copy link
Contributor

I found that this PR might've introduced an issue.

Before, the rake integration task was failing, but at least completing and showing "Passed: 1, Failed: 3". See the end of the log here: https://travis-ci.org/github/jruby/warbler/jobs/771640167.

With this PR it's failing to even start with the error: "Error installing /home/travis/.m2/repository/rubygems/rubyzip/2.3.0/rubyzip-2.3.0.gem: rubyzip requires Ruby version >= 2.4.".

The "solution" seems to (again) to drop support for jruby 9.1 as well, so I'm going to propose that as a separate PR.

And that surfaces yet another issue because the rake integration job is shown as green, where it's actually broken. I'll try to propose yet another PR to fix that as well.

@deivid-rodriguez deivid-rodriguez mentioned this pull request Jun 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olleolleolle olleolleolle olleolleolle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow newer versions of rubyzip
4 participants
@ioanatia @olleolleolle @deivid-rodriguez @enebo
0