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

"Logger" warning with Ruby 3.3.5 #1061

Open
voxik opened this issue Sep 4, 2024 · 5 comments
Open

"Logger" warning with Ruby 3.3.5 #1061

voxik opened this issue Sep 4, 2024 · 5 comments

Comments

@voxik
Copy link
Contributor

voxik commented Sep 4, 2024

Running railties test suite with Ruby 3.3.5, I observer following failures due to warning which appears to come from concurrent-ruby:

* Test file: test/application/bin_setup_test.rb
/usr/share/gems/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/deprecation.rb:1: warning: logger 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 logger to your Gemfile or gemspec to silence this warning.
/usr/share/gems/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/deprecation.rb:1: warning: logger 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 logger to your Gemfile or gemspec to silence this warning.
Run options: --seed 53132

# Running:

F

Failure:
ApplicationTests::BinSetupTest#test_bin_setup_output [test/application/bin_setup_test.rb:51]:
--- expected
+++ actual
@@ -2,10 +2,16 @@
 The Gemfile's dependencies are satisfied
 
 == Preparing database ==
+/usr/share/gems/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/deprecation.rb:1: warning: logger 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 logger to your Gemfile or gemspec to silence this warning.
 Created database 'app_development'
 Created database 'app_test'
 
 == Removing old logs and tempfiles ==
+/usr/share/gems/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/deprecation.rb:1: warning: logger 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 logger to your Gemfile or gemspec to silence this warning.
 
 == Restarting application server ==
+/usr/share/gems/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/deprecation.rb:1: warning: logger 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 logger to your Gemfile or gemspec to silence this warning.
 "


rails test test/application/bin_setup_test.rb:30

.

Finished in 7.311424s, 0.2735 runs/s, 0.9574 assertions/s.
2 runs, 7 assertions, 1 failures, 0 errors, 0 skips

This is likely caused by ruby/ruby@9ae91eb

@eregon
Copy link
Collaborator

eregon commented Sep 4, 2024

I'll run the test suite to see if it repros that:
https://github.com/eregon/concurrent-ruby/actions/runs/10705209923/job/29680039267

Probably we need to add logger as a gem dependency then, from https://bugs.ruby-lang.org/issues/20309

@eregon
Copy link
Collaborator

eregon commented Sep 4, 2024

Right I see it in https://github.com/eregon/concurrent-ruby/actions/runs/10705209923/job/29680039267#step:4:41

$ /opt/hostedtoolcache/Ruby/3.3.5/x64/bin/ruby -I/home/runner/work/concurrent-ruby/concurrent-ruby/vendor/bundle/ruby/3.3.0/gems/rspec-core-3.13.1/lib:/home/runner/work/concurrent-ruby/concurrent-ruby/vendor/bundle/ruby/3.3.0/gems/rspec-support-3.13.1/lib /home/runner/work/concurrent-ruby/concurrent-ruby/vendor/bundle/ruby/3.3.0/gems/rspec-core-3.13.1/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb --color --backtrace --order defined --format documentation
/home/runner/work/concurrent-ruby/concurrent-ruby/lib/concurrent-ruby/concurrent/concern/deprecation.rb:1: warning: logger 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 logger to your Gemfile or gemspec to silence this warning.

It doesn't fail concurrent-ruby's CI but we should still fix it of course.

@eregon
Copy link
Collaborator

eregon commented Sep 4, 2024

The README says:

The design goals of this gem are:

  • ...
  • Remain free of external gem dependencies

So we have no choice but to break that goal then?

concurrent-ruby doesn't seem to use much of Logger, but it uses at least currently Logger::FATAL, Logger::DEBUG and Logger::SEV_LABEL.
It has its own replacement of Logger because the stdlib one was deadlocking apparently (it would be good to check if that is still the case):

def self.create_simple_logger(level = Logger::FATAL, output = $stderr)
# TODO (pitr-ch 24-Dec-2016): figure out why it had to be replaced, stdlogger was deadlocking
lambda do |severity, progname, message = nil, &block|

So it might be feasible to remove the usage of Logger, except in create_stdlib_logger where it could be an autoload. Or since that's already deprecated maybe remove it entirely.
Instead of a Logger::<LEVEL> we'd take a Symbol or so, and Concurrent::Concern::Logging#log is marked as private API so there shouldn't be external users.
We still have the issue that create_simple_logger takes an Integer for the level and that should keep working though.

@eregon
Copy link
Collaborator

eregon commented Sep 4, 2024

I cannot reproduce the deadlock with the stdlib logger with 3.3.5.
But given how little concurrent-ruby uses logging I think it's best to keep using its simple logger and avoid a gem dependency for this.

The original problem was noticed in this commit.

And create_stdlib_logger/use_stdlib_logger have been re-added as deprecated in 959e764, 8 years ago. So it might be OK to remove it now.

eregon added a commit to eregon/concurrent-ruby that referenced this issue Sep 4, 2024
* To fix the Ruby 3.3.5 warnings:
  ruby-concurrency#1061
* concurrent-ruby only uses 7 constants from Logger, so just copy those over.
@eregon
Copy link
Collaborator

eregon commented Sep 4, 2024

I came up with a PR to remove the dependency on logger: #1062

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants