-
Notifications
You must be signed in to change notification settings - Fork 72
THREESCALE-11906: Upgrade to Ruby 3.3 #4129
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
base: master
Are you sure you want to change the base?
Conversation
akostadinov
left a comment
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.
Maybe we need to update io-console to 0.7.1 to keep it with the default in ruby and avoid native compilation errors on fedora 42.
https://stdgems.org/io-console/
But it's alright to merge as is and try.
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.
I doesn't work for me, bundle exec rails assets:clobber assets:precompile fails with this error:
$ bundle exec rails assets:clobber
bin/rails aborted!
ArgumentError: wrong number of arguments (given 3, expected 1..2) (ArgumentError)
RE_NON_ASCII = Regexp.new('([\x00-\xFF])', Regexp::IGNORECASE, 'n') # [^\0-\177]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/home/jlledom/redhat/porta/config/application.rb:38:in `<main>'
/home/jlledom/redhat/porta/Rakefile:3:in `require_relative'
/home/jlledom/redhat/porta/Rakefile:3:in `<main>'
bin/rails:4:in `<main>'
(See full trace by running task with --trace)What I did:
- Updated my
.tool-versionsto point to ruby 3.3 - Installed bundler 2.5.22
| ruby 3.1.5 | ||
| ruby 3.3.1 | ||
| nodejs 18.20.4 | ||
| python 2.7.18 |
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.
I know it's not related, but, why do we need python?
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.
I believe this was needed for macs, not sure why...
Maybe @josemigallas or @lvillen can confirm whether this is still needed.
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.
It's required by some dependency. A while ago python 3 was made the default in macOS and broke such dependency, so porta could not run.
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.
I doubt anything modern will need python 2.7.18, maybe you need to try again with 3.
Agree on upgrading io-console. Either here or in another PR |
Yes, there is some other thing needed. And also, this PR is currently blocked by #4130, which is WIP, so this one is also WIP 🙃 |
b0aa8f2 to
cea300a
Compare
1eaed14 to
f1148a2
Compare
| system-builder-ruby31: &system-builder-ruby31 | ||
| image: quay.io/3scale/system-builder:5207de2 | ||
| system-builder-ruby33: &system-builder-ruby33 | ||
| image: quay.io/3scale/system-builder:ruby-3.3 |
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.
Where can I see the dockerfile for this?
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.
| # This module adds a 'present' literal to the liquid expressions, so that | ||
| # expressions such as the following can be used: | ||
| # {% if current_account.applications.first == present %} | ||
| # This is required because existing templates use it | ||
| # Previously, this feature was added like this: | ||
| # Liquid::Expression::LITERALS['present'] = :present? | ||
| # but in the latest Liquid this constant hash is frozen |
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.
I trust you, but is this feature tested in the test suite?
069db8e to
a22b3a3
Compare
jlledom
left a comment
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.
It all works for me 👍
❌ 1 blocking issue (3 total)
|
akostadinov
left a comment
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.
Looks awesome!
| prism (1.4.0) | ||
| prometheus-client-mmap (0.28.0) | ||
| rb_sys (~> 0.9) | ||
| prometheus-client-mmap (1.3.0) |
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.
Needed for Ruby 3.3 support: https://gitlab.com/gitlab-org/ruby/gems/prometheus-client-mmap/-/blob/master/CHANGELOG.md?ref_type=heads#v110
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.
Unfortunately, this is causing issues in midstream builds.
- This version requires Rust
Installing prometheus-client-mmap 1.3.0 with native extensions
Building native extensions. This could take a while...
Bundler::InstallError: Gem::Ext::BuildError: ERROR: Failed to build gem native extension.
current directory: /opt/system/vendor/bundle/ruby/3.3.0/gems/prometheus-client-mmap-1.3.0/ext/fast_mmaped_file_rs
/usr/bin/ruby -I/usr/share/rubygems extconf.rb
checking for rustc... no
*** extconf.rb failed ***
Could not create Makefile due to some reason, probably lack of necessary
libraries and/or headers. Check the mkmf.log file for more details. You may
need configuration options.
Provided configuration options:
--with-opt-dir
--without-opt-dir
--with-opt-include=${opt-dir}/include
--without-opt-include
--with-opt-lib=${opt-dir}/lib64
--without-opt-lib
--with-make-prog
--without-make-prog
--srcdir=.
--curdir
--ruby=/usr/bin/$(RUBY_BASE_NAME)
extconf.rb:28:in `<main>': rustc not found. prometheus-client-mmap now requires Rust. (RuntimeError)
To see why this extension failed to compile, please check the mkmf.log which can be found here:
/opt/system/vendor/bundle/ruby/3.3.0/extensions/x86_64-linux/3.3.0/prometheus-client-mmap-1.3.0/mkmf.log
extconf failed, exit code 1
Gem files will remain installed in /opt/system/vendor/bundle/ruby/3.3.0/gems/prometheus-client-mmap-1.3.0 for inspection.
Results logged to /opt/system/vendor/bundle/ruby/3.3.0/extensions/x86_64-linux/3.3.0/prometheus-client-mmap-1.3.0/gem_make.out
This can be fixed easily by installing the rust-toolset package.
- Rust dependencies are attempted to be installed:
current directory: /opt/system/vendor/bundle/ruby/3.3.0/gems/prometheus-client-mmap-1.3.0/ext/fast_mmaped_file_rs
make DESTDIR\= sitearchdir\=./.gem.20251230-1-b9lap2 sitelibdir\=./.gem.20251230-1-b9lap2
cargo rustc --locked --manifest-path ./Cargo.toml --target-dir target --lib --profile release -- -C linker=gcc -L native=/usr/lib64 -C link-arg=-Wl,-z,relro -C link-arg=-Wl,--as-needed -C link-arg=-Wl,-z,now -C link-arg=-specs=/usr/lib/rpm/redhat/redhat-hardened-ld -C link-arg=-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1
Updating crates.io index
warning: spurious network error (3 tries remaining): [56] Failure when receiving data from the peer (Received HTTP code 403 from proxy after CONNECT)
warning: spurious network error (2 tries remaining): [56] Failure when receiving data from the peer (Received HTTP code 403 from proxy after CONNECT)
warning: spurious network error (1 tries remaining): [56] Failure when receiving data from the peer (Received HTTP code 403 from proxy after CONNECT)
error: failed to get `hashbrown` as a dependency of package `fast_mmaped_file_rs v0.1.0 (/opt/system/vendor/bundle/ruby/3.3.0/gems/prometheus-client-mmap-1.3.0/ext/fast_mmaped_file_rs)`
Caused by:
download of config.json failed
Caused by:
failed to download from `https://index.crates.io/config.json`
Caused by:
[56] Failure when receiving data from the peer (Received HTTP code 403 from proxy after CONNECT)
make: *** [Makefile:572: target/release/libfast_mmaped_file_rs.so] Error 101
make failed, exit code 2
Gem files will remain installed in /opt/system/vendor/bundle/ruby/3.3.0/gems/prometheus-client-mmap-1.3.0 for inspection.
Results logged to /opt/system/vendor/bundle/ruby/3.3.0/extensions/x86_64-linux/3.3.0/prometheus-client-mmap-1.3.0/gem_make.out
/usr/share/rubygems/rubygems/ext/builder.rb:125:in `run'
/usr/share/rubygems/rubygems/ext/builder.rb:51:in `block in make'
/usr/share/rubygems/rubygems/ext/builder.rb:43:in `each'
/usr/share/rubygems/rubygems/ext/builder.rb:43:in `make'
/usr/share/rubygems/rubygems/ext/ext_conf_builder.rb:42:in `build'
/usr/share/rubygems/rubygems/ext/builder.rb:193:in `build_extension'
/usr/share/rubygems/rubygems/ext/builder.rb:227:in `block in build_extensions'
This doesn't work because build environment is hermetic.
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.
Even if the build fails due to the rust issue, I think it's fine to merge this. We'll have to fix midstream to support rust. I think it's fine to depend on rust stuff, native components will require rust more frequently I think, since it's gradually replacing C. So if we don't do it for prometheus, we'll end up doing it anyway for some other gem.
And you know, we really don't want to make the Rust cult angry, in fact, we should completely rewrite porta and apisonator in Rust, as a goodwill gesture to them.
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.
probably we need to somehow point hermeto at the Cargo.toml but we only have it later, when the gem is downloaded :(
maybe we need to add the matching prometheus-client-mmap sources as build sources and then point at their Cargo for additional deps fetch.
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.
we should completely rewrite porta and apisonator in Rust, as a goodwill gesture to them
As a first step lets quickly migrate to https://artichoke.github.io/artichoke/artichoke/, I think we will be safe if our interpreter was written in a proper language.
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.
An amazing piece of engineering, now, do you think the Rust cult would spare my life I write a rust compiler in Ruby?
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.
Almost free of memory leaks, of course
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.
spare my life I write a rust compiler in Ruby?
If that ruby compiler run on top artichoke, perhaps. Just make sure not to build a rust interpreter in ruby and run it with the Ruby's YJIT rust implementation because the thing may become conscious and destroy us all.
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.
since it's gradually replacing C. So if we don't do it for prometheus, we'll end up doing it anyway for some other gem.
I'm not sure if it will happen in the foreseeable future of the project.
We can't merge this until we figure out how to build rust-dependant projects. Otherwise, all future builds will be blocked forever 😞
src/lib.rs
Outdated
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.
I understand this is only a PoC but it can live in a special sub-dir in the worst case (like script/yarn). In the best case it will only live inside the midstream repo.
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.
Not sure about midstream, it's required to run cargo fetch to populate the Cargo.lock from Cargo.toml.
But yeah, once (if) we make it work, we can make the required adjustments.
What this PR does / why we need it:
Because Ruby 3.1 is EOL
Which issue(s) this PR fixes
https://issues.redhat.com/browse/THREESCALE-11906
Verification steps
Special notes for your reviewer:
There was a previous failed attempt to upgrade here: #3864