-
Notifications
You must be signed in to change notification settings - Fork 935
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
Add former default gems as a dependency for Ruby 3.1 compatibility #1439
Conversation
…ruby-head build They were recently removed from Ruby's default gems. Ref: https://bugs.ruby-lang.org/issues/17873 Ref: mikel/mail#1439 Ref: teamcapybara/capybara#2468
Actually this cause double loading warnings for ruby 3.0 and older. I'm not too sure yet how best to fix this. |
David Rodríguez, bundler's maintainer is looking at the issue, I'll keep an eye and report back here when there's a solution: https://bugs.ruby-lang.org/issues/17873#note-18 |
So the double loading issue was fixed in rubygems/bundler, we can probably merge this now. |
net-smtp has been removed from default gems in Ruby 3.1, but it is used by the `mail` gem. Remove when mikel/mail#1439 is fixed
See the following: * rails/rails#42366 * mikel/mail#1439 * https://bugs.ruby-lang.org/issues/17873 In essence, the net/smtp gem config was changed in ruby 3.1 and the mail gem needs to merge in their fix.
Any news here? |
@@ -14,6 +14,9 @@ Gem::Specification.new do |s| | |||
s.rdoc_options << '--exclude' << 'lib/mail/values/unicode_tables.dat' | |||
|
|||
s.add_dependency('mini_mime', '>= 0.1.1') | |||
s.add_dependency('net-smtp') | |||
s.add_dependency('net-imap') | |||
s.add_dependency('net-pop') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these gems no-op on older Rubies (1.8, 1.9, etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These gems yes, however they themselves have digest
as dependency which is causing some doable loading problem:
/Users/byroot/.gem/ruby/2.7.2/gems/digest-3.0.0/lib/digest.rb:6: warning: already initialized constant Digest::REQUIRE_MUTEX
/opt/rubies/2.7.2/lib/ruby/2.7.0/digest.rb:6: warning: previous definition of REQUIRE_MUTEX was here
....
I already submitted the fix in bundler rubygems/rubygems#4989, hopefully it should be fixed soon. I totally understand if you want to wait for bundler to fix this first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is already fixed @jeremy @casperisfine. Can we release this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to wrap these in an if statement that is only true for Ruby 3.1+?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is those gems can't be installed on older versions, @casperisfine's reply only shows part of the issue.
For example on 2.0.0:
$ gem i net-smtp
Fetching: timeout-0.2.0.gem (100%)
Successfully installed timeout-0.2.0
Fetching: io-wait-0.2.1.gem (100%)
ERROR: Error installing net-smtp:
io-wait requires Ruby version >= 2.6.0.
$ gem i net-imap
ERROR: Error installing net-imap:
io-wait requires Ruby version >= 2.6.0.
$ gem i net-pop
ERROR: Error installing net-pop:
io-wait requires Ruby version >= 2.6.0.
Also #1439 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nertzy I am pretty sure that an if statement in a .gemspec ends up getting evaluated at gem release time, and trying to condition on ruby version would end up being on what ruby version is running in the environment doing the gem release, and then fixed in the release.
My understanding is you can't actually do dynamic conditions based on the running environment in a gemspec like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally those gems ('net-smtp', 'net-imap' and 'net-pop') would all NOOP when installed on an older unsupported Ruby. That would just solve it for everyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm still trying to find solutions upstream, but ultimately would you be open to drop pre 2.5 support to get this moved forward now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, CRuby devs don't seem generally happy to need to explicitly support (even as noop) such old and EOL Ruby versions based on recent discussions, which brings the age-old question: do gems depending on net-* and specifically mail
still need to support Ruby <= 2.5 today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can still go this way as well.
Bump minor (or major) version and we can still maintain releases compat with <= 2.5 if needed in "legacy" branch.
Btw. I'm happy to help with anything moving this forward.
Could we merge master and add Ruby 3.1 to the GitHub Actions matrix? |
It was recently removed from the default gems: - https://bugs.ruby-lang.org/issues/17873 - ruby/ruby#4530
58704be
to
651a59a
Compare
Hum:
That's a problem, not sure how we can possibly handle this. |
If they noop (#1439 (comment)), maybe the |
What do you mean? |
Ah nevermind, seems like I didn't properly understand @jeremy's question. They definitely don't noop. So I don't see how we can support both 3.1 and 2.4 |
A somewhat crazy idea:
Alternatively maybe those 3 gems should really have "empty/noop/using stdlib" releases with |
Ref: mikel/mail#1439 Some gems depend on io-wait, but still support older rubies, so they have to chose between droping support or not listing io-wait. But io-wait could act a a noop on older rubies.
Ref: mikel/mail#1439 Some gems depend on io-wait, but still support older rubies, so they have to chose between droping support or not listing io-wait. But io-wait could act a a noop on older rubies.
Ref: mikel/mail#1439 Some gems depend on io-wait, but still support older rubies, so they have to chose between droping support or not listing io-wait. But io-wait could act a a noop on older rubies. ruby/io-wait@75fcb74c32
I think @simi's suggestion at #1439 (comment) is the best way to go. In order to reduce maintainers' burden to implement that solution, I created #1472, including the changes being proposed here. |
My PR includes a lot of cleanups of old Ruby 1.8 compatibility code that can be now removed. It can be argued that reviewing those changes actually increases maintainers' burden 😅. If that's the case, I'm super happy to remove those cleanups from my PR. |
Necessary until mikel/mail#1439 got merged.
Make use of parallel and matrix builds. Note Ruby v3.1 is not yet supported due to a breakage of `mail` gem due to a missing `net-smtp` dependency: mikel/mail#1439 (comment)
We need to do this as the `mail` gem hasn't yet been updated. See mikel/mail#1439.
- Bump PORTREVISION for dependency change This commit also reverts da6b780. Reference: mikel/mail#1439
- Bump PORTREVISION for dependency change This commit also reverts da6b780. Reference: mikel/mail#1439
Necessary until mikel/mail#1439 got merged.
Closing this in favour of #1472 |
When moving to Ruby 3.1 some parts of the standard library were moved to default gems[0][1]. While we're on Rails 6.x they need to be included in the gem file, as discussed in this this GitHub issue[2]. Once we upgrade to Rails 7 these requirments can be dropped. [0] https://bugs.ruby-lang.org/issues/17873 [1] https://stdgems.org/#default-gems-ruby-301 [2] mikel/mail#1439 (comment)
example: $ bundle exec rake test ... snip ... /Users/alexdean/.rvm/rubies/ruby-2.6.4/lib/ruby/2.6.0/net/protocol.rb:30: warning: method redefined; discarding old protocol_param /Users/alexdean/.rvm/gems/ruby-2.6.4/gems/net-protocol-0.2.1/lib/net/protocol.rb:32: warning: previous definition of protocol_param was here /Users/alexdean/.rvm/rubies/ruby-2.6.4/lib/ruby/2.6.0/net/protocol.rb:38: warning: method redefined; discarding old ssl_socket_connect /Users/alexdean/.rvm/gems/ruby-2.6.4/gems/net-protocol-0.2.1/lib/net/protocol.rb:40: warning: previous definition of ssl_socket_connect was here /Users/alexdean/.rvm/rubies/ruby-2.6.4/lib/ruby/2.6.0/net/protocol.rb:66: warning: already initialized constant Net::ProtocRetryError /Users/alexdean/.rvm/gems/ruby-2.6.4/gems/net-protocol-0.2.1/lib/net/protocol.rb:68: warning: previous definition of ProtocRetryError was here /Users/alexdean/.rvm/rubies/ruby-2.6.4/lib/ruby/2.6.0/net/protocol.rb:79: warning: method redefined; discarding old initialize /Users/alexdean/.rvm/gems/ruby-2.6.4/gems/net-protocol-0.2.1/lib/net/protocol.rb:81: warning: previous definition of initialize was here /Users/alexdean/.rvm/rubies/ruby-2.6.4/lib/ruby/2.6.0/net/protocol.rb:82: warning: method redefined; discarding old io /Users/alexdean/.rvm/rubies/ruby-2.6.4/lib/ruby/2.6.0/net/protocol.rb:84: warning: method redefined; discarding old message changes made so far don't seem to resolve the error, so this might not be useful. unsure, but it seems like these might be due to bug in mail and/or rubygems. mikel/mail#1439
Necessary until mikel/mail#1439 got merged.
Alpine 3.17 comes with ruby-3.1, which runs into some compattibility issues for certain gems. So this includes some patches to make sure it's compattible: 1. openssl is pinned to 3.0.1, the versions shipped with ruby 3.1[0] 2. net-smtp, net-imap and net-pop gems are added as they are no longer bundled[1] 3. psych is kept on a version <4 as version 4 is backwards incompattible[2]. [0]:https://gitlab.com/gitlab-org/gitlab/-/blob/v15.10.0-ee/Gemfile#L22 [1]:mikel/mail#1439 [2]:https://bugs.ruby-lang.org/issues/1786
Alpine 3.17 comes with ruby-3.1, which runs into some compattibility issues for certain gems. So this includes some patches to make sure it's compattible: 1. openssl is pinned to 3.0.1, the versions shipped with ruby 3.1[0] 2. net-smtp, net-imap and net-pop gems are added as they are no longer bundled[1] 3. psych is kept on a version <4 as version 4 is backwards incompattible[2]. [0]:https://gitlab.com/gitlab-org/gitlab/-/blob/v15.10.0-ee/Gemfile#L22 [1]:mikel/mail#1439 [2]:https://bugs.ruby-lang.org/issues/1786
It was recently removed from the default gems:
cc @mikel @jeremy