-
Notifications
You must be signed in to change notification settings - Fork 226
Refs #38357 - Support of ruby version 3.4 #914
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
Conversation
ab66b0b
to
632c98f
Compare
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.
Looking at https://stdgems.org/logger/ it states:
The gem type will be changed to bundled gem in a future version of Ruby
So it will be dropped (possibly Ruby 3.5). We also require the logging
gem in lib/proxy/log.rb
so I wonder if we can align on that.
Reading the concurrent-ruby source code I see you can assign the global logger so perhaps we should do that instead? You can see they deprecated the create_stdlib_logger
so they're moving away from it as well.
Having said that, I do think in some places (log_buffer
at least) we directly rely on the logger gem.
What do you think we should do?
I don't think its about the Logger rubygem - the logger is part of ruby itself. The same change was done in rails/rails#54621, too |
This is the key thing: for now it is, but in Ruby 3.5 it will be removed: ruby/ruby@d7e558e. We can expect Ruby 3.5 in December and I like to look forward. If we can get rid of it altogether now that'll save work in the future. |
Well, during a installation of the smart-proxy on Windows we got this issue because of the missing 'require logger'. |
It concerns me we don't see it in CI now and I question the location. We also have https://github.com/theforeman/smart-proxy/blob/develop/lib/smart_proxy_for_testing.rb. Does it mean we simply don't hit the code paths in our testing? Related: right now we're only testing on Ruby 3.0 but we should expand our matrix with Ruby 3.1 (Debian 12), 3.2 (Ubuntu 24.04) and 3.3 (EL 10 & Debian 13). Ideally also 3.4 for the future. Perhaps then we'd probably see it ourselves in CI? |
I added these versions. Only 3.4 failed to test:
|
49fdf50
to
addfb90
Compare
I tried without the "require logger" - but this wasn't found by our testing. I wonder why... |
Fixed the tests. How to continue with this @ekohl ? |
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.
Some minor nits but overall I like where this is heading.
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.
Looking at https://projects.theforeman.org/issues/38357 this should probably be Refs #38357
and be cherry picked into 3.15.0.
Pinned excon <= 1.2.5 for now. Update: Fixed. new 1.2.7 of excon was just released. |
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.
Thanks for updating this. What do you think about the last inline suggestion?
@lhellebr I'd like to include this in RC2 or GA if possible.
@sbernhard could you also squash the commits into 1 so Redmine issues is pleased and we'll also cherry pick correctly? Also a note to https://community.theforeman.org/c/development/9 will be nice for all developers. |
@ekohl rebase / squash done. |
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.
Thanks!
@sbernhard thanks a lot for picking this up. Would you mind creating the Discourse post to mention we'll do this? For developers the impact is that their Smart Proxy CI will start running on Ruby 3.4 now and that can cause failures. |
I hope this is ok: https://community.theforeman.org/t/smart-proxy-ci-now-running-on-ruby-3-4/43409 |
Support of ruby version 3.4.
Therefore we need to add the dependency to logger here. See ruby-concurrency/concurrent-ruby#1062