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

Bump Ruby to 3.3.1 #9597

Merged
merged 14 commits into from Apr 30, 2024
Merged

Bump Ruby to 3.3.1 #9597

merged 14 commits into from Apr 30, 2024

Conversation

jurre
Copy link
Member

@jurre jurre commented Apr 24, 2024

This PR upgrades Ruby from 3.1.4 to 3.3.1 and Rubygems from 2.3.1 to 2.5.9.

Changelogs:

These releases bring a lot of exciting performance improvements, including the ability to enable Ruby's YJIT, which seems promising, even for the relatively short lifetime of our jobs.

Bundler v1

We’d been blocked on adopting Ruby 3.2+ for quite some time, so let me address what I’ve done to unblock us from that.

In Ruby 3.2, support for Object#untaint was removed. Here is some context on that, but the important bit for this PR is, that method has been a no-op since Ruby 2.7. It hasn’t done anything for years.

The initial versions of Bundler 2 would ship with a special case to omit the method on newer versions of Ruby.

So, it seems safe to say that we do not care about untaint in the context of bundler.

What I’ve done here to make it work is include a patch onto Object to stub out #untaint and load it into Bundler v1, both in our native helpers and when we run bundle install, by wrapping the command in a new script.

Native helper specs

In rubygems/rubygems#6793 Net::HTTP was vendored and the namespace changed to ::Gem::Net::HTTP. This caused our native helpers to no longer have network requests intercepted by WebMock. I've added a copy of WebMock's Net::HTTP adapter for the new namespace.

One thing of note is I've found our bundler v1 specs run against the latest Bundler version. Production code and tests from dependabot-core do use Bundler 1.17 in that context, since bundler/helpers/v1/run.rb loads it via gem "bundler", "~> 1.17". Our native helper specs are loaded via BUNDLER_VERSION=1.17.3 bundle exec rspec and this seems to not load that version of bundler. This is surprising to me and something we might address, but maybe we'll remove support for Bundler 1 before we care about this.

@github-actions github-actions bot added the L: ruby:bundler RubyGems via bundler label Apr 24, 2024
@jurre jurre force-pushed the jurre/getting-jitty-with-it branch 3 times, most recently from 52b3b8b to 45b7f7c Compare April 24, 2024 18:54
@github-actions github-actions bot added L: elixir:hex Elixir packages via hex L: java:gradle Maven packages via Gradle L: terraform Terraform packages L: java:maven Maven packages via Maven L: dart:pub Dart packages via pub L: javascript L: python labels Apr 24, 2024
@jurre jurre force-pushed the jurre/getting-jitty-with-it branch from 50a8b2a to 835c861 Compare April 25, 2024 11:06
@JamieMagee
Copy link
Contributor

FYI Ruby 3.3.1 was recently released: https://www.ruby-lang.org/en/news/2024/04/23/ruby-3-3-1-released/

@JamieMagee
Copy link
Contributor

I opened an issue upstream about the new Sorbet warning with bigdecimal: sorbet/sorbet#7854

@jurre
Copy link
Member Author

jurre commented Apr 25, 2024

Thanks, yeah we should go straight to that, I started this work locally before it was released 😄 I assume it'll be an easy lift once we're on 3.3.0 though 👍

There's a few failures in the native helper specs, which are down to the Rubygems version changing with the Ruby version. Addressing those now.

@jurre
Copy link
Member Author

jurre commented Apr 26, 2024

Something slightly concerning that I found while working through these test failures:

Our tests for the helpers in bundler/helpers/v1 run against whatever the is latest version of Bundler that's installed, and not against 1.17.3, but our production code and the tests that call the native helpers from dependabot-core do use bundler 1.17.3. So these native helper tests are not reliable right now.

The reason this happens is because we call gem "bundler", "~> 1.17.3" in run.rb, and our test setup bypasses this and uses bundle exec rspec to invoke the tests directly, bypassing the specific bundler setup we're using. We are setting BUNDLER_VERSION=1.17.3 when executing these tests, but it does not appear to be altering the version of bundler the tests are running with.

I verified this by setting a debugger in this environment and grabbing Bundler::VERSION when running the tests natively, and I modified the helpers to write the bundler version to a file when running in integration mode.

I've verified that this happens both before and after the Ruby upgrade, so it's not being introduced in this PR, this has been the case for a long time, probably since the beginning or when we reshuffled how bundler is loaded in this setup.

@jurre
Copy link
Member Author

jurre commented Apr 26, 2024

I believe the native tests are failing because WebMock is no longer able to stub the requests because of rubygems/rubygems#6793 and so the tests are making real network requests now.

@jurre jurre force-pushed the jurre/getting-jitty-with-it branch from 835c861 to be507fc Compare April 26, 2024 09:40
@jurre jurre changed the title Bump Ruby to 3.3.0 Bump Ruby to 3.3.1 Apr 26, 2024
@jurre jurre force-pushed the jurre/getting-jitty-with-it branch 4 times, most recently from 998801c to 4533928 Compare April 26, 2024 17:17
@jurre jurre marked this pull request as ready for review April 26, 2024 17:21
@jurre jurre requested a review from a team as a code owner April 26, 2024 17:21
This WebMock adapter is mostly a direct copy from WebMock itself, just
replaces `Net::HTTP` with `::Gem::Net:HTTP`, would like to keep them as
similar as possible if we ever need to keep changes in sync
@jurre jurre force-pushed the jurre/getting-jitty-with-it branch from 4533928 to c8f9fc1 Compare April 26, 2024 18:50
@@ -54,7 +54,7 @@ COPY --chown=dependabot:dependabot LICENSE $DEPENDABOT_HOME

# Install Ruby from official Docker image
# When bumping Ruby minor, need to also add the previous version to `bundler/helpers/v{1,2}/monkey_patches/definition_ruby_version_patch.rb`
COPY --from=docker.io/library/ruby:3.1.4-bookworm --chown=dependabot:dependabot /usr/local /usr/local
COPY --from=docker.io/library/ruby:3.3.1-bookworm --chown=dependabot:dependabot /usr/local /usr/local
Copy link
Contributor

Choose a reason for hiding this comment

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

@jurre I think you also need to add some versions to a Bundler monkeypatch as explained in the comment above?

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

I left a small comment.

Nice work on this upgrade, it seems very tricky, and I love that you managed to keep Bundler v1 support, allowing the decision to drop that to not block this upgrade! 💪

@JamieMagee
Copy link
Contributor

Is YJIT enabled by default in Ruby 3.3? The release notes only mention that it is enabled by default for Rails 7.2.

@landongrindheim
Copy link
Member

Is YJIT enabled by default in Ruby 3.3? The release notes only mention that it is enabled by default for Rails 7.2.

The docs suggest it's disabled by default. I'll try to get this PR out this week, then once we're sure of its stability, we can enable YJIT 😄

@JamieMagee JamieMagee mentioned this pull request Apr 29, 2024
@JamieMagee
Copy link
Contributor

@landongrindheim I created #9633 to track it.

landongrindheim and others added 2 commits April 29, 2024 18:34
This is being done for two reasons:

1. We're upgrading to Ruby 3.3 (so we need to list previous versions)
2. Several CVEs were resolved in Ruby 3+ releases. We should use these

Co-authored-by: Bryan Dragon <25506+bdragon@users.noreply.github.com>
Co-authored-by: Bryan Dragon <25506+bdragon@users.noreply.github.com>
@@ -26,7 +26,7 @@ def source_requirements
Gem::Specification.new("Ruby\0", requested_version)
end

%w(2.5.3 2.6.10 2.7.7 3.0.5 3.2.1).each do |version|
%w(2.5.3 2.6.10 2.7.8 3.0.7 3.1.5 3.2.4).each do |version|
Copy link
Member

Choose a reason for hiding this comment

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

@deivid-rodriguez We upgraded this list to include the latest Rubies. I think this is what we're supposed to do, but I'm not 100% confident that I understand the monkey patch. Can you give a 👍/👎 re: whether this lines up with the intent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it seems correct to me, the list needs to complete the list of "supported rubies". Since Dependabot now uses Ruby 3.3, it seems correct 👍.

I think basically this is needed so that Dependabot also works even in situations where the application being updated is not compatible with the Ruby version Dependabot is using internally. I'd like to support this upstream, and we have an issue tracking it but not sure when/if it will happen.

@@ -12,7 +12,7 @@ def index
Gem::Specification.new("ruby\0", requested_version)
end

%w(2.5.3p105 2.6.10p210 2.7.6p219 3.0.4p208).each do |version|
%w(2.5.3p105 2.6.10p210 2.7.6p219 3.0.4p208 3.1.4p223).each do |version|
Copy link
Member

Choose a reason for hiding this comment

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

@deivid-rodriguez I've included the current Ruby in this (Bundler v1) list. There's a more recent release (Ruby 3.1.5), but we're not running it yet. Does this list seem reasonable to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say you can add 3.1.5 and 3.2.4 too? I don't think the specific patch level is super important sice normally gems with set their requirements with something like required_ruby_version = ">= 2.6.0", so all patch levels should have the same result. But best to use latest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if the patch part (p105 for 2.5.3, etc) is needed or not. I suspect it was some Bundler 1 thing that made that necessary so just in case I'd keep them. Unfortunately I don't know of a good way of figuring out the patchlevel other than installing each ruby and checking the value of RUBY_PATCHLEVEL.

Copy link
Member

Choose a reason for hiding this comment

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

We've had this out for about four hours now without issues. I'm going to merge it. I'll have to do a followup PR to add the patch part 😄 Thanks for all your input 🙇

Copy link
Member

Choose a reason for hiding this comment

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

I've opened #9645 to add the patch levels 🙂

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

👍 🎉

Per review feedback:

> I'd say you can add 3.1.5 and 3.2.4 too? I don't think the specific
patch level is super important sice normally gems with set their
requirements with something like required_ruby_version = ">= 2.6.0", so
all patch levels should have the same result. But best to use latest.

I've updated the Ruby versions to those which have received security
patches recently.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: dart:pub Dart packages via pub L: elixir:hex Elixir packages via hex L: java:gradle Maven packages via Gradle L: java:maven Maven packages via Maven L: javascript L: python L: ruby:bundler RubyGems via bundler L: terraform Terraform packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants