Bug #20714
openHandle optional dependencies in `bundled_gems.rb`
Description
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
Updated by Eregon (Benoit Daloze) 2 months ago
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
.
Updated by Eregon (Benoit Daloze) 2 months ago
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.
Updated by Earlopain (A S) 2 months ago
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?
Updated by Eregon (Benoit Daloze) 2 months ago
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.
Updated by deivid (David Rodríguez) 2 months ago
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?
Updated by Earlopain (A S) 2 months ago
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.
Updated by hsbt (Hiroshi SHIBATA) 2 months ago
- Status changed from Open to Assigned
- Assignee set to hsbt (Hiroshi SHIBATA)
Updated by hsbt (Hiroshi SHIBATA) 2 months ago
- Related to Feature #20309: Bundled gems for Ruby 3.5 added
Updated by deivid (David Rodríguez) about 1 month ago · Edited
@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.
Updated by hsbt (Hiroshi SHIBATA) about 1 month ago
@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.
Updated by Earlopain (A S) about 1 month ago
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.
Updated by Eregon (Benoit Daloze) about 1 month ago · Edited
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.
Updated by deivid (David Rodríguez) about 1 month ago
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.
Updated by Earlopain (A S) about 1 month ago
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?
Updated by Eregon (Benoit Daloze) about 1 month ago
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).
Updated by deivid (David Rodríguez) about 1 month ago
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.
Updated by hsbt (Hiroshi SHIBATA) about 1 month ago
By the way, I share @Earlopain's questions about the additional patch
Ah, thanks. It's simple bug. I'll fix at next week.
Updated by Earlopain (A S) about 1 month ago
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.
Updated by hsbt (Hiroshi SHIBATA) about 1 month ago
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.
Updated by Earlopain (A S) about 1 month ago
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.
Updated by Eregon (Benoit Daloze) about 1 month ago
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 (A S) '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.