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

Upgrading ruby to 3.3 #23142

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Upgrading ruby to 3.3 #23142

merged 1 commit into from
Nov 13, 2024

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 12, 2024

3.2 ref

Highlights:

  • optional: Regexp.timeout = 1.0 or Regexp.new('^a*b?a*()\1$', timeout: Float::INFINITY)
  • All methods wishing to delegate keyword arguments through *args must now be marked with ruby2_keywords, with no exception.
  • Hash#shift (empty returns nil instead of default)
  • Struct no longer needs keyword_init
  • removed: Fixnum, Bignum Random::DEFAULT, Struct::Group, Struct::Passwd
  • removed: Dir.exists?, File.exists?
  • removed: Kernel#=~, Kernel#taint, Kernel#untaint, Kernel#tainted?, Kernel#trust, Kernel#untrust, Kernel#untrusted?
  • lib no longer included: libyaml. (was in psych. May need libyaml-dev on ubuntu)
  • lib no longer included: libffi. (was in fiddle - only used on windows)
  • add Queue#pop, SizedQueue#push, SizedQueue#pop added param timeout:

3.3

punt:

  • ERB::Util.html_escape is made faster than CGI.escapeHTML
  • no longer need require "set" (supporting older ruby versions)
    • this is not hurting anyone, and this change is not compatible for ruby <3.2 - which we still support

External PRs

These were changed in external libraries and make using them easier, but are not required for our upgrade.

Related PRs

providers:

current solution

gem install --verbose ovirt-engine-sdk -v4.6.0 -- --with-cflags="-Wno-error=incompatible-function-pointer-types -Wno-error=implicit-function-declaration"

@@ -16,6 +16,7 @@ jobs:
ruby-version:
- '3.0'
- '3.1'
- '3.3'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbrock Even if we jump from 3.1 to 3.3 in a release, we should include 3.2 with 3.3 so if they fail in CI, we can determine if it's a problem unique to 3.3 or also a problem in 3.2

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, since we will never use 3.2, I'm mixed on this one.
Already got 2 thumbs up, so I'll add it.
But it seems a waste of cycles and unnecessary granularity

@bdunne
Copy link
Member

bdunne commented Aug 14, 2024

For build, EL9 ships with ruby 3.0 by default and has dnf modules for 3.1 and 3.3, but not 3.2.

@Fryguy
Copy link
Member

Fryguy commented Aug 15, 2024

so weird that el9 jumped a version 😕

@kbrock
Copy link
Member Author

kbrock commented Aug 16, 2024

The gem build failure for ovirt-engine-sdk is frustrating.
I can see that @jrafanie and others had issues building it for oVirt/ovirt-engine-sdk-ruby#11

Hopefully it shouldn't be too hard. Guess this is where it all begins. Wish they had a C guide titled "this is what changed over the past 20 years" - because that code has a bunch of stuff I do not recognize

@jrafanie
Copy link
Member

ov_http_request.c:75:29: warning: excess elements in array initializer
   75 |         .reserved = { NULL, NULL }
      |                             ^~~~
ov_http_request.c:75:29: note: (near initialization for
‘ov_http_request_type.function.reserved’)
ov_http_request.c: In function ‘ov_http_request_define’:
ov_http_request.c:347:77: error: ‘rb_cData’ undeclared (first use in this
function)
347 |     ov_http_request_class = rb_define_class_under(ov_module,
"HttpRequest", rb_cData);
|                                                                         
^~~~~~~~
ov_http_request.c:347:77: note: each undeclared identifier is reported only once
for each function it appears in
ov_http_request.c: At top level:
cc1: note: unrecognized command-line option ‘-Wno-self-assign’ may have been
intended to silence earlier diagnostics
cc1: note: unrecognized command-line option ‘-Wno-parentheses-equality’ may have
been intended to silence earlier diagnostics
cc1: note: unrecognized command-line option ‘-Wno-constant-logical-operand’ may
have been intended to silence earlier diagnostics
make: *** [Makefile:248: ov_http_request.o] Error 1

This looks alittle different. There's no option reported in the output to silence it.

@Fryguy
Copy link
Member

Fryguy commented Aug 16, 2024

That's also an error, whereas the others were warnings.

@jrafanie
Copy link
Member

jrafanie commented Aug 16, 2024

That's also an error, whereas the others were warnings.

sorry, it's different than the one we had previously where it told you that you can ignore it by opting into that behavior:

implicit function declarations [-Wimplicit-function-declaration]

oVirt/ovirt-engine-sdk-ruby#11

This one is a undeclared variable if I'm reading correctly.

@kbrock kbrock self-assigned this Aug 20, 2024
@micwoj92
Copy link

micwoj92 commented Sep 9, 2024

@jrafanie in the linked issue from your last comment you tried to build version 4.6.0 of ovirt engine sdk. This error is because the CI pulls 4.4.1 and it is unrelated. This issue has already been fixed with latest version. See oVirt/ovirt-engine-sdk-ruby@97df88c

@kbrock
Copy link
Member Author

kbrock commented Sep 10, 2024

@kbrock kbrock marked this pull request as ready for review September 10, 2024 21:33
@kbrock kbrock requested a review from Fryguy as a code owner September 10, 2024 21:33
@kbrock kbrock changed the title upgrading ruby to 3.3 [WIP] upgrading ruby to 3.3 Sep 10, 2024
@kbrock kbrock added the wip label Sep 10, 2024
miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Sep 10, 2024
@Fryguy
Copy link
Member

Fryguy commented Sep 12, 2024

@kbrock I find this site more useful for detailing changes in Ruby:

@kbrock kbrock force-pushed the ruby33 branch 3 times, most recently from 5322c93 to 02a88ad Compare September 20, 2024 22:31
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@kbrock
Copy link
Member Author

kbrock commented Nov 6, 2024

update:

  • no longer warn for ruby >= 3.2

unsure: Do we want to force ruby => 3.1?

@kbrock kbrock force-pushed the ruby33 branch 2 times, most recently from d61924f to 83a892b Compare November 11, 2024 21:34
@kbrock kbrock changed the title [WIP] upgrading ruby to 3.3 Upgrading ruby to 3.3 Nov 12, 2024
@kbrock
Copy link
Member Author

kbrock commented Nov 12, 2024

update:

  • testing with 3.1, 3.3
  • allow 3.1-3.4
  • warn >= 3.4

@miq-bot
Copy link
Member

miq-bot commented Nov 12, 2024

Checked commit kbrock@1e99a38 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@jrafanie
Copy link
Member

  • Hash#shift (empty returns nil instead of default)

Is there anything we need to do here? I assume it's difficult to find.

@kbrock
Copy link
Member Author

kbrock commented Nov 13, 2024

@jrafanie no. I searched through the code and it looks like we are all set, but left it as "outstanding" to let people know that a few of these may still be lurking.

ok. Ignoring specs:

  • Array.new(size, default_value): 1 instance. all set.
  • Array.new(size) {}: 12 instances. all set
    • Array.new(size) do: 4 instances.

@kbrock kbrock removed the wip label Nov 13, 2024
raise "Ruby versions >= 3.4.0 are unsupported!" if RUBY_VERSION >= "3.4.0"

ruby ">= 3.1.1", "< 3.5.0"
warn "Ruby versions >= 3.4.0 are untested!" if RUBY_VERSION >= "3.4.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this resolve the issue around rubocop and warning for testing of untested rubies?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. adding the padding on the top ( < 3.5 instead of < 3.4) with the warning in there will help us in the future too.

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just minor question. As long as we have smoke tested images/appliances with ruby 3.3, I'm good with shipping this!

❤️

@jrafanie
Copy link
Member

@bdunne are you good with smoke tests of appliances/pods with ruby 3.3? Are we good to go?

@kbrock add radjabov if this is for backport.

@jrafanie
Copy link
Member

LGTM, @bdunne @Fryguy merge when ready.

@Fryguy Fryguy merged commit 3fb39c3 into ManageIQ:master Nov 13, 2024
8 checks passed
@kbrock kbrock deleted the ruby33 branch November 14, 2024 14:38
@jrafanie
Copy link
Member

🎉 🕺 🏆 🎉 🕺 🏆 🎉 🕺 🏆

@Fryguy
Copy link
Member

Fryguy commented Nov 14, 2024

Backported to radjabov in commit a4e4a73.

commit a4e4a739cdd1feeda8948253bfdd8564b6508592
Author: Jason Frey <fryguy9@gmail.com>
Date:   Wed Nov 13 17:30:47 2024 -0500

    Merge pull request #23142 from kbrock/ruby33
    
    Upgrading ruby to 3.3
    
    (cherry picked from commit 3fb39c3b87d88a144d8bf03cf49b7a42dbf5bd8d)

Fryguy added a commit that referenced this pull request Nov 14, 2024
Upgrading ruby to 3.3

(cherry picked from commit 3fb39c3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants