Bug #20714: Handle optional dependencies in `bundled_gems.rb`

• Status: Assigned
• Assignee: hsbt (Hiroshi SHIBATA)
• Target version: -
• ruby -v: 3.3.5
• Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN

[Earlopain (Earlopain _)]

I've encountered a few places around bundled gems where the library doesn't care if the gem is available, but will still provide some functionallity if it is.

The way to accomplish that right now seems to be by setting $VERBOSE = nil and resetting it later again to not bother the user with the warning about the gem. However, this has the effect of silencing the warning about other gems as well, that may not be prepared about the bundling.

From ruby/reline for example: https://github.com/ruby/reline/blob/c90f08f7e308d2f1cdd7cfaf9939fe45ce546fd2/lib/reline/terminfo.rb#L1-L15
Or the logging gem: https://github.com/TwP/logging/blob/df41715364f7eb8c65098cd3c3316677ef1f3784/lib/logging.rb#L9-L15

I propose to simply delay the warning to the next require.

GitHub PR at https://github.com/ruby/ruby/pull/11545

[Earlopain (Earlopain _)]

• Description updated (diff)

[Eregon (Benoit Daloze)]

One potential problem with setting $VERBOSE to nil is that's not thread-safe, and so might hide valid warnings from other threads requiring around the same time.
Maybe it should be some explicit method in Gem or so to suppress that warning, or a keyword argument like require "foo", optional: true.

[Eregon (Benoit Daloze)]

The problem is any new API would be rather cumbersome to use (in a way that still works on older Rubies), so probably this fix is good as a quick measure.

I think it would be good to think about a longer-term solution too though.

[Earlopain (Earlopain _)]

Thanks for you reply, I was about to write something similar about the older rubies.

At first I was writing about how I don't think a new api is really necessary since the compatibility code can simply be dropped once required_ruby_version is at or above the ruby version that would error but I don't think that is entirely true. For example, on ruby 3.4 the following script produces this output:

begin
  require "webrick"
rescue LoadError
end

if defined?(Webrick)
  puts "Do some things"
end

# test.rb:2: warning: webrick was loaded from the standard library, but is not part of the default gems starting from Ruby 3.0.0.
# You can add webrick to your Gemfile or gemspec to silence this warning.

So even though webrick is gone since Ruby 3.0, I'd still have to do some trickery to optionally enhance it without triggering the warning.

I see two solutions:

• require "foo", optional: true like you suggested
• Drop warnings when ruby would throw an error here. When the require will raise, it doesn't need to warn. This is how I already imagined it works. I guess it doesn't for visibility?

[Eregon (Benoit Daloze)]

Maybe a good solution is to make $VERBOSE thread-local or fiber-local at some point, then at least this concern and related ones would be solved once and for all.

[deivid (David Rodríguez)]

Drop warnings when ruby would throw an error here. When the require will raise, it doesn't need to warn. This is how I already imagined it works. I guess it doesn't for visibility?

I think this is the best solution. The warning is also misleading if the require fails because it suggests that the require succeeded ("webrick was loaded from...").

Is it as simple as https://github.com/ruby/ruby/pull/11550?

[Earlopain (Earlopain _)]

deivid (David Rodríguez) wrote in #note-6:

Is it as simple as https://github.com/ruby/ruby/pull/11550?

I had something a little different in mind (trim SINCE) but I think that is a nice solution. The PR looks reasonable to me.

[hsbt (Hiroshi SHIBATA)]

• Status changed from Open to Assigned
• Assignee set to hsbt (Hiroshi SHIBATA)

[hsbt (Hiroshi SHIBATA)]

• Related to Feature #20309: Bundled gems for Ruby 3.5 added

[deivid (David Rodríguez)]

@hsbt (Hiroshi SHIBATA) found an issue with the simpler approach. It will no longer show information about bundled gem extraction "after the fact". So, it for example keeps warning that csv will no longer be part of Ruby starting from Ruby 3.4 when using Ruby 3.3, but once users upgrade to Ruby 3.4, they will start getting an standard load error instead of the current message "csv is not part of the default gems starting from Ruby 3.4.0".

I think the more complicated approach proposed here does not have that issue, but it does have the thread safety concerns raised.

I think the issue found by @hsbt (Hiroshi SHIBATA) may be an acceptable compromise, but I'm not sure.

[hsbt (Hiroshi SHIBATA)]

@deivid (David Rodríguez) Thanks for sharing. My concern is here

I will consider what's better approach for us with https://github.com/ruby/ruby/pull/11550 and https://github.com/ruby/ruby/pull/11545 before 3.4.0-preview2.

[Earlopain (Earlopain _)]

Maybe I have another solution alltogether:

begin
  gem 'fiddle'
rescue LoadError
  # Not available, no require => no warning. Future ruyb versions will hit this.
  return
end

# No warning on old versions, will warn for one minor version. So, still need to suppress the warning somehow so that other consumers not equiped to handle this get notified.
verbose = $VERBOSE
$VERBOSE = nil
require 'fiddle'
$VERBOSE = verbose

Then @deivid (David Rodríguez) PR would not be as pressing if existing usage of $VERBOSE in reline, etc. gets modified. This seems to work fine with and without bundler. Let me know if that looks reasonable and won't make any issues. Users could still just do it the other way of course but it might alleviate the need for a new api like require ..., optional: true.

Still, that one version that doesn't warn seems significant.

[Eregon (Benoit Daloze)]

https://github.com/ruby/ruby/pull/11550 seems a good solution to me.

I think it is expected to only get the LoadError for csv on 3.4 and not also the warning.
People seeing the LoadError for csv would intuitively add csv to their Gemfile, which is the same they would do to address the warning.
For non-Bundler usages as far as I understand require 'csv' would still work so that's unaffected.

[deivid (David Rodríguez)]

I think this would affect both Bundler and non Bundler usages. Currently the warnings recommends to install the gem from RubyGems in non Bundler context, that will also be lost in Ruby 3.4 for gems like csv.

That said, I also tend to think this is an acceptable compromise. As @hsbt (Hiroshi SHIBATA) noticed, users upgrading directly from Ruby 3.2 to 3.4, or in general users used to the old Ruby 3.2 behavior that fail to see the Ruby 3.3 warnings for whatever reason may find this a bit harder. But I expect most users will go through Ruby 3.3 at some point and get some heads up about this.

[Earlopain (Earlopain _)]

I see that https://github.com/ruby/ruby/pull/11550 was merged which is great but I'm confused about https://github.com/ruby/ruby/commit/ff3f61556fb62d12d57d017f4c54f1a8fd5208be because it basically reverts that PR. If a dependency is optional depends on each specific use and while this applies to fiddle through reline and how it uses it, other cases not from stdlib/bundled are ignored.

I'm also not sure it the conditions are correct. For fiddle, it will never use OPTIONAL since the LoadError is truthy, and other gems always warn for the same reason. I don't see how this now behaves differently than before. Could you explain?

[Eregon (Benoit Daloze)]

deivid (David Rodríguez) wrote in #note-14:

I think this would affect both Bundler and non Bundler usages. Currently the warnings recommends to install the gem from RubyGems in non Bundler context, that will also be lost in Ruby 3.4 for gems like csv.

It correctly doesn't warn when not using Bundler, since it remains a bundled gem so it can be required like any other bundled gem:

$ ruby -ve 'require "csv"'            
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux]
$ touch Gemfile
$ bundle exec ruby -ve 'require "csv"'
ruby 3.3.5 (2024-09-03 revision ef084cc8f4) [x86_64-linux]
-e:1: warning: csv was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.4.0.
You can add csv to your Gemfile or gemspec to silence this warning.

(and it's the same (no warning) if some gem requires csv and is used not under Bundler).

[deivid (David Rodríguez)]

I meant that I think in Ruby 3.4 (prior to my patch), the warning would also apply to non Bundler contexts that lead to a LoadError, where it would inform that csv may not always be present and may need an explicit install from RubyGems.org. But I have not tried this myself, just assumed it from reading the code, so maybe I misunderstood.

By the way, I share @Earlopain's questions about the additional patch introduced after https://github.com/ruby/ruby/pull/11550.

[hsbt (Hiroshi SHIBATA)]

By the way, I share @Earlopain's questions about the additional patch

Ah, thanks. It's simple bug. I'll fix at next week.

[Earlopain (Earlopain _)]

https://github.com/ruby/ruby/commit/3a9e48b9a4860022f43d8101c0f3249299437886 this seems better but I still question the rationale.

I don't see the point in giving fiddle special treatment with OPTIONAL. What problem is this solving? All it does is supress the warning from reline (which is suppressed directly in the gem right now anyways) when fiddle is gone in 3.5 but all the other gems that are still do warn. It seems needlessly inconsistent.

Again, if a gem is optional depends on how it is used. Don't give everyone else with fiddle the same treatment just because reline does it in a specific way. My opinion is that either all gems should warn indefinitely or none.

[hsbt (Hiroshi SHIBATA)]

All it does is supress the warning from reline (which is suppressed directly in the gem right now anyways) when fiddle is gone in 3.5

Yes, I only accept that point with your request in this time. I may add syslog to OPTIONAL with logging cases.

Again, if a gem is optional depends on how it is used. Don't give everyone else with fiddle the same treatment just because reline does it in a specific way. My opinion is that either all gems should warn indefinitely or none.

OK, I'll revert all related changes if that's what you said that. I'm considering how to suppress needless warning with your request, But I can't collaborate with your tone.

[Earlopain (Earlopain _)]

I apologize for my tone, I will try to be more understanding. I am just a bit frustrated with the warnings in general, especially with the ones that were backported 3.3.5. I know it is already reverted but they do still happen now.

It would have been nicer if you instead made your changes as a PR so that others could have given feedback instead of checking latest commits and writing about it here after.

Anyways, let's see if others have an opinion. I'll just take it as it comes now.

[Eregon (Benoit Daloze)]

OK, I'll revert all related changes if that's what you said that. I'm considering how to suppress needless warning with your request, But I can't collaborate with your tone.

I don't see anything wrong about @Earlopain (Earlopain _) 's tone. And his concerns seem valid to me. Probably there was a misunderstanding?

I do find it strange that @hsbt (Hiroshi SHIBATA) would merge the PR after general consensus here and then push a commit which basically reverts the PR except for one specific case, and not even mention it here.
Maybe there was some unexpected test failure or so which prompted a quick change?
It's a bit as if there was a bug and a fix is merged, and then silently reverted. That would be unexpected, right?

@hsbt (Hiroshi SHIBATA) Could you explain why you think it's better to only do this behavior for fiddle and not for other gems? I don't see that in the commit message or anywhere.
This issue description is clear that it's not about that one specific case.

[hsbt (Hiroshi SHIBATA)]

I have no idea to correctly handling optional dependency now. I reverted related commit for Ruby 3.4 at https://github.com/ruby/ruby/commit/441069c093ab0d21efff1f0f3144fdf412f9f675.

I'll try that targeted Ruby 3.5 or feature.

[deivid (David Rodríguez)]

@hsbt (Hiroshi SHIBATA) My patch in https://github.com/ruby/ruby/pull/11550 seems to work correctly, though, right? I understand the concern of bumping multiple rubies at a time, skipping warnings, but I think the benefits it provides (handling optional dependencies correctly) overcome this potential issue.

[Earlopain (Earlopain _)]

https://github.com/ruby/ruby/commit/8cd36a6dab170f4127c58c07b1a388dd3813fb7a removed the first batch of gems from the list of warnings. I agree with it and don't particularly care if these warn a few years after removal, as long as it stops at one point. The warning message is still confusing for future ruby versions though.

The issue I see with your PR is that for one version, they will still warn regardless. So, for a somewhat specific example:

• Some gem x requires fidlde and handles the potential LoadError
• Gem x starts to warn on Ruby 3.4, but this gem is already equiped to handle it. The warning is unnecessary
• Other gems loaded after x may also require fiddle but will not warn since the warning was already emitted
• On Ruby 3.5, gem x works and doesn't emit a warning anymore
• However, gems loaded after x will error if they failed to declare fiddle as their dependency

I guess you could just rely on the multitude of different gemfiles people have and guess that there is someone without gem x to report the warning to the other gem author. But I'm unsure if that is a safe bet to make.

I don't think you can properly handle it in bundled_gems.rb with what is currently available, the information about if a require should warn or not is simply not there.

[deivid (David Rodríguez)]

My PR would not warn fiddle require in x at all, because the require did not succeed, right? That was the point, to not warn optional dependencies.

[Eregon (Benoit Daloze)]

I think the trouble is on require 'fiddle' in 3.4, e.g. in reline, it would actually succeed (and return true):

source 'https://rubygems.org'

gem 'reline'

$ bundle install
$ bundle exec ruby -ve 'require "reline"; p Fiddle'
ruby 3.4.0preview2 (2024-10-07 master 32c733f57b) +PRISM [x86_64-linux]
Fiddle

If we remove the $VERBOSE changes in lib/reline/terminfo.rb:

$ bundle exec ruby -e 'require "reline"; p Fiddle' 
/home/eregon/tmp/reline-gemfile/vendor/bundle/ruby/3.4.0+0/gems/reline-0.5.11/lib/reline.rb:9: warning: fiddle was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add fiddle to your Gemfile or gemspec to silence this warning.
Fiddle

So I think https://github.com/ruby/ruby/pull/11550 wouldn't solve that.

[Earlopain (Earlopain _)]

Yes, the trouble is that for one version the require will still actually work. I don't think your PR actually changes anything in that regard, except for stopping to show the warnings in more future releases of Ruby (which can also be accomplished by just removing the gems from the SINCE constant).

My PR https://github.com/ruby/ruby/pull/11545 supresses the warning if it would not be shown anyways, so that it can be emitted in subsequent requires where it would actually matter. But it comes with its own set of problems https://bugs.ruby-lang.org/issues/20714#note-2

[deivid (David Rodríguez)]

Oh, indeed. You're right.

[Eregon (Benoit Daloze)]

I think https://github.com/ruby/ruby/pull/11545 is a good change and worth merging.
It doesn't hide any warning, but at least doesn't do extra work if $VERBOSE is nil (= optional callers), and it warns for non-VERBOSE=nil (= not optional) callers which is a bug fix.
The fact that $VERBOSE is not thread-local is not great, but it's an orthogonal pre-existing issue which can be addressed separately.
@hsbt (Hiroshi SHIBATA) Could you review that PR?