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

Add net-http dependency to frameworks using mail #44175

Conversation

composerinteralia
Copy link
Member

@composerinteralia composerinteralia commented Jan 14, 2022

In 5dd292f5 we added net- gems as dependencies of actionmailbox and
actionmailer.

That caused warnings in our Ruby 2.7 builds, where net-protocol is
getting loaded twice—both as a gem and as a Ruby library. These warnings
are visible in buildkite.

/usr/local/lib/ruby/2.7.0/net/protocol.rb:66: warning: already initialized constant Net::ProtocRetryError
/usr/local/bundle/gems/net-protocol-0.1.2/lib/net/protocol.rb:68: warning: previous definition of ProtocRetryError was here

It's getting loaded twice because the net-http that ships with Ruby 2.7
has a require_relative "protocol" that causes it to always
load the library in Ruby instead of the gem. By using the net-http gem
as well, we avoid this problem.

These warnings are also visibile in brand new rails apps created off
main, for example by running:

ruby -v
=> 2.7.3
rails new my_app --main
cd my_app
rails test

Loading multiple versions of the same gem can also cause cryptic
warnings. For example if you install webmock and enable it before
loading any Rails application files (something we do at GitHub), you can
end up with an error:

`<module:Net>': superclass mismatch for class InternetMessageIO (TypeError)

This commit does pull in URI, which can cause warnings when using
versions of bundler older than 2.2.33. But we've already got warnings
related to the digest gem when using bundler versions older than 2.3.0:
before this PR removing digest as a bundler dependency we end
up loading two version and getting the warnings:

ruby/2.7.3/lib/ruby/gems/2.7.0/gems/digest-3.1.0/lib/digest.rb:20: warning: already initialized constant Digest::REQUIRE_MUTEX
ruby/2.7.3/lib/ruby/2.7.0/digest.rb:6: warning: previous definition of REQUIRE_MUTEX was here

With or without this commit, we may need to either drop Ruby 2.7 support
or bump the minimum version of bundler if we want the warnings to go
away entirely.

Co-authored-by: Nick Holden

In [5dd292f][] we added net- gems as dependencies of actionmailbox  and
actionmailer.

That caused warnings in our Ruby 2.7 builds, where `net-protocol` is
getting loaded twice—both as a gem and as a Ruby library. These warnings
are visible in [buildkite][].

```
/usr/local/lib/ruby/2.7.0/net/protocol.rb:66: warning: already initialized constant Net::ProtocRetryError
/usr/local/bundle/gems/net-protocol-0.1.2/lib/net/protocol.rb:68: warning: previous definition of ProtocRetryError was here
```

[5dd292f]: rails@5dd292f
[buildkite]: https://buildkite.com/rails/rails/builds/84010#60f6c08a-f35d-4b87-a56d-2d06d43e8ee3

It's getting loaded twice because the net-http that ships with Ruby 2.7
has a [`require_relative "protocol"`][ruby lib] that causes it to always
load the library in Ruby instead of the gem. By using the `net-http` gem
as well, we [avoid this problem][gem].

[ruby lib]: https://github.com/ruby/ruby/blob/v2_7_3/lib/net/http.rb
[gem]: https://github.com/ruby/net-http/blob/38d8a19454faa74d892380846a2ba4d3e86487b9/lib/net/http.rb#L23

These warnings are also visibile in brand new rails apps created off
main, for example by running:

```
ruby -v
=> 2.7.3
rails new my_app --main
cd my_app
rails test
```

Loading multiple versions of the same gem can also cause cryptic
warnings. For example if you install [webmock][] and enable it before
loading any Rails application files (something we do at GitHub), you can
end up with an error:

```
`<module:Net>': superclass mismatch for class InternetMessageIO (TypeError)
```

[webmock]: https://github.com/bblimke/webmock

This commit does pull in URI, which can cause warnings when using
versions of bundler older than 2.2.33. But we've already got warnings
related to the digest gem when using bundler versions older than 2.3.0:
before this [PR removing digest as a bundler dependency][digest] we end
up loading two version and getting the warnings:

```
ruby/2.7.3/lib/ruby/gems/2.7.0/gems/digest-3.1.0/lib/digest.rb:20: warning: already initialized constant Digest::REQUIRE_MUTEX
ruby/2.7.3/lib/ruby/2.7.0/digest.rb:6: warning: previous definition of REQUIRE_MUTEX was here
```

[digest]: rubygems/rubygems@c19a9f2

With or without this commit, we may need to either drop Ruby 2.7 support
or bump the minimum version of bundler if we want the warnings to go
away entirely.

Co-authored-by: Nick Holden <nholden@github.com>
@matthewd
Copy link
Member

Having to add net-http as a dependency to gems that don't use it seems quite unfortunate indeed. Should we reconsider #44083? 🤔

@nholden
Copy link
Contributor

nholden commented Jan 18, 2022

Should we reconsider #44083?

Since net-smtp, net-imap, and net-pop no longer automatically included in Ruby 3.1, I don't think we can remove the gem dependencies added in #44083 and continue to support Ruby 3.1 until the mail gem adds them as gem dependencies (as proposed and discussed in mikel/mail#1439) or takes some other action (remove references to them entirely?).

@rafaelfranca: since you worked on #44083, do you have any other thoughts here?

@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 20, 2022

I think we should just accept the warnings or fix Ruby/RubyGems to load the right things. I don't think we should make a gem that is not a requirement a dependency just to avoid warnings. About reconsidering #44083, I think that actually means reconsidering support to Ruby 3.1 and I don't think we should do that just to support Ruby 2.7.

Those requirements in Rails are mostly temporary though, so one day when mail actually define its dependencies we will be able to remove it from Rails. But that means the warning will still exist unless we fix Ruby 2.7.

@composerinteralia
Copy link
Member Author

composerinteralia commented Jan 20, 2022

just accept the warnings

They are warnings in CI and in a new Rails app, but can become errors when using certain other libraries like webmock. We ran into these errors at GitHub.

fix Ruby/RubyGems to load the right things

Maybe we can change require_relative 'protocol' in Ruby 2.7's net-http to require 'net/protocol'?

The approach taken by the digest gem, where it returns early if the constant is already defined, doesn't quite work here because we load the gem first and the Ruby 2.7 library later.

I don't think we should do that just to support Ruby 2.7

Is dropping support for Ruby 2.7 before the next release a possibility?

But that means the warning will still exist unless we fix Ruby 2.7.

Yeah, good point

@rafaelfranca
Copy link
Member

Is dropping support for Ruby 2.7 before the next release a possibility?

Not really, we are only going to do that in Rails 8 if we keep the same policy.

Maybe we can change require_relative 'protocol' in Ruby 2.7's net-http to require 'net/protocol'?

I think that would be the best approach.

composerinteralia added a commit to composerinteralia/ruby that referenced this pull request Jan 20, 2022
Rails now includes net-protocol as a gem dependency (via the net-smtp,
net-imap, and net-pop gems).

On Ruby 2.7 net-http loads the built-in version of net-protocol via a
`require_relative`. If both the gem and the built-in version are loaded
we end up with warnings about redefined constants, or errors in some
cases.

Using `require 'net/protocol'` gives us the gem version if there is one,
or the built-in one if not, preventing us from loading both.

See rails/rails#44175 for details. I had thought
of adding net-http as a gem dependency in Rails, but it seems like folks
are not too thrilled with that idea.
@aaronjensen
Copy link
Contributor

aaronjensen commented Jan 25, 2022

This is an issue in 3.0.3 for us as well. We actually need to bundle exec irb in some situations in order to avoid a Gem::LoadError.

In our case it seems like irb is loading net/protocol ahead of time, before we require rails. I don't know why. If we use bin/rails c, it works fine, so we are doing that.

@ryanb
Copy link
Contributor

ryanb commented Jan 27, 2022

For those upgrading to Rails 7.0.1 on Ruby 2.7 and wanting to silence the net protocol warnings, you can add the net-http gem to your Rails app (like this PR would do).

# Silence net protocol warnings
# See https://github.com/rails/rails/pull/44175
gem 'net-http'

At least it's a stop-gap until this gets sorted out. :)

@rails-bot
Copy link

rails-bot bot commented Apr 27, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 27, 2022
@rails-bot rails-bot bot closed this May 4, 2022
@pjmartorell
Copy link

@ryanb I'm getting this error after adding your suggestion:

/opt/hostedtoolcache/Ruby/2.7.6/x64/lib/ruby/gems/2.7.0/gems/bundler-2.3.12/lib/bundler/runtime.rb:309:in `check_for_activated_spec!': You have already activated uri 0.10.0, but your Gemfile requires uri 0.11.0. Since uri is a default gem, you can either remove your dependency on it or try updating to a newer version of bundler that supports uri as a default gem. (Gem::LoadError)

I updated bundler to 2.3.14 but the error persists.

@ryanb
Copy link
Contributor

ryanb commented May 27, 2022

@pjmartorell That's tricky for me to debug since I'm not experiencing the error. It looks like a conflict between uri versions in your Gemfile.lock. Perhaps try setting it to 0.10.0 by adding this to your Gemfile.

gem 'uri', '0.10.0'

Does that work?

Update: Another thing to try, according to this stack overflow answer, is run this command to remove gems that aren't used in the current bundle.

bundle clean --force

@fimac
Copy link

fimac commented Jun 20, 2022

@ryanb I'm getting this error after adding your suggestion:

/opt/hostedtoolcache/Ruby/2.7.6/x64/lib/ruby/gems/2.7.0/gems/bundler-2.3.12/lib/bundler/runtime.rb:309:in `check_for_activated_spec!': You have already activated uri 0.10.0, but your Gemfile requires uri 0.11.0. Since uri is a default gem, you can either remove your dependency on it or try updating to a newer version of bundler that supports uri as a default gem. (Gem::LoadError)

I updated bundler to 2.3.14 but the error persists.

I'm seeing the same thing after adding net-http as a runtime dep. When bundle exec rake spec runs in our CI tests for Ruby 2.7 with rails 6 and 7, they fail with this same error. Ruby >= 3.1 with Rails 6 or 7 work fine.
https://github.com/cipherstash/activestash/runs/6960325147?check_suite_focus=true
Bundler version is bundler-2.3.16.

I think it might be because uri version 0.10.0 is a standard gem with Ruby 2.7, but net-http has uri 0.11.0 as a dependancy. https://rubygems.org/gems/net-http/versions/0.2.2/dependencies

In version 3.1 onwards the uri version is 0.11.0. https://stdgems.org/uri/.

Setting the default uri version to 0.11.0 stopped the error for me.

elia added a commit to nebulab/solidus_auth_devise that referenced this pull request Sep 9, 2022
`solidus_bolt` needs to be disabled for the dummy app otherwise it
fails due to the contrived nature of the app.

For rubies < 3 using the latest rails we need to use the external
net/http gem to avoid this error:

rake aborted!
LoadError: cannot load such file -- net/smtp
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `block in require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:299:in `load_dependency'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/mail-2.7.1/lib/mail.rb:9:in `<module:Mail>'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/mail-2.7.1/lib/mail.rb:3:in `<top (required)>'

Ref rails/rails#44175 (comment)
elia added a commit to nebulab/solidus_auth_devise that referenced this pull request Sep 9, 2022
`solidus_bolt` needs to be disabled for the dummy app otherwise it
fails due to the contrived nature of the app.

For rubies < 3 using the latest rails we need to use the external
net/http gem to avoid this error:

rake aborted!
LoadError: cannot load such file -- net/smtp
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `block in require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:299:in `load_dependency'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/mail-2.7.1/lib/mail.rb:9:in `<module:Mail>'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/mail-2.7.1/lib/mail.rb:3:in `<top (required)>'

Ref rails/rails#44175 (comment)
elia added a commit to nebulab/solidus_auth_devise that referenced this pull request Sep 9, 2022
`solidus_bolt` needs to be disabled for the dummy app otherwise it
fails due to the contrived nature of the app.

For rubies < 3 using the latest rails we need to use the external
net/http gem to avoid this error:

rake aborted!
LoadError: cannot load such file -- net/smtp
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `block in require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:299:in `load_dependency'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/activesupport-6.1.6.1/lib/active_support/dependencies.rb:332:in `require'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/mail-2.7.1/lib/mail.rb:9:in `<module:Mail>'
/home/circleci/project/vendor/bundle/v3.1/ruby/3.1.0/gems/mail-2.7.1/lib/mail.rb:3:in `<top (required)>'

Ref rails/rails#44175 (comment)
hsbt pushed a commit to composerinteralia/ruby that referenced this pull request Sep 16, 2022
Rails now includes net-protocol as a gem dependency (via the net-smtp,
net-imap, and net-pop gems).

On Ruby 2.7 net-http loads the built-in version of net-protocol via a
`require_relative`. If both the gem and the built-in version are loaded
we end up with warnings about redefined constants, or errors in some
cases.

Using `require 'net/protocol'` gives us the gem version if there is one,
or the built-in one if not, preventing us from loading both.

See rails/rails#44175 for details. I had thought
of adding net-http as a gem dependency in Rails, but it seems like folks
are not too thrilled with that idea.
hsbt pushed a commit to composerinteralia/ruby that referenced this pull request Sep 27, 2022
Rails now includes net-protocol as a gem dependency (via the net-smtp,
net-imap, and net-pop gems).

On Ruby 2.7 net-http loads the built-in version of net-protocol via a
`require_relative`. If both the gem and the built-in version are loaded
we end up with warnings about redefined constants, or errors in some
cases.

Using `require 'net/protocol'` gives us the gem version if there is one,
or the built-in one if not, preventing us from loading both.

See rails/rails#44175 for details. I had thought
of adding net-http as a gem dependency in Rails, but it seems like folks
are not too thrilled with that idea.
eileencodes added a commit that referenced this pull request Jan 5, 2023
This change adds net-http to the Gemfile to avoid errors from loading
the gem twice.

This is related to #44175 although I
cannot tell you why it fails on 6-1-stable but not 7-0-stable when that
change wasn't merged. The current theory is that 7.x doesn't include
webpacker by default so the dummy apps made there don't fail (because
net-http isn't required), but in 6.x webpacker is included.

Fixes failures like:

```
"/usr/local/lib/ruby/2.7.0/net/protocol.rb:66: warning: already initialized constant Net::ProtocRetryError
/usr/local/bundle/gems/net-protocol-0.2.1/lib/net/protocol.rb:68: warning: previous definition of ProtocRetryError was here
/usr/local/lib/ruby/2.7.0/net/protocol.rb:206: warning: already initialized constant Net::BufferedIO::BUFSIZE
/usr/local/bundle/gems/net-protocol-0.2.1/lib/net/protocol.rb:214: warning: previous definition of BUFSIZE was here
/usr/local/lib/ruby/2.7.0/net/protocol.rb:503: warning: already initialized constant Net::NetPrivate::Socket
/usr/local/bundle/gems/net-protocol-0.2.1/lib/net/protocol.rb:541: warning: previous definition of Socket was here
```
eileencodes added a commit that referenced this pull request Jan 5, 2023
This change adds net-http to the Gemfile to avoid errors from loading
the gem twice.

This is related to #44175 although I
cannot tell you why it fails on 6-1-stable but not 7-0-stable when that
change wasn't merged. The current theory is that 7.x doesn't include
webpacker by default so the dummy apps made there don't fail (because
net-http isn't required), but in 6.x webpacker is included.

Fixes failures like:

```
"/usr/local/lib/ruby/2.7.0/net/protocol.rb:66: warning: already initialized constant Net::ProtocRetryError
/usr/local/bundle/gems/net-protocol-0.2.1/lib/net/protocol.rb:68: warning: previous definition of ProtocRetryError was here
/usr/local/lib/ruby/2.7.0/net/protocol.rb:206: warning: already initialized constant Net::BufferedIO::BUFSIZE
/usr/local/bundle/gems/net-protocol-0.2.1/lib/net/protocol.rb:214: warning: previous definition of BUFSIZE was here
/usr/local/lib/ruby/2.7.0/net/protocol.rb:503: warning: already initialized constant Net::NetPrivate::Socket
/usr/local/bundle/gems/net-protocol-0.2.1/lib/net/protocol.rb:541: warning: previous definition of Socket was here
```
kidhab added a commit to foodcoops/sharedlists that referenced this pull request Jan 21, 2023
Fixes warnings as stated in rails/rails#44175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants