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.

Sign up for GitHub

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

Jump to bottom

Bump Ruby to 3.1 #5447

Merged
merged 6 commits into from
Oct 7, 2022
Merged

Bump Ruby to 3.1 #5447

merged 6 commits into from
Oct 7, 2022

Conversation

jeffwidman
Copy link
Member

@jeffwidman jeffwidman commented Jul 29, 2022 •
edited
Loading

This is necessary to unblock #5030, because Ubuntu
22.04 doesn't provide an older version of openssl, so need a new
enough Ruby to work with the newer version of openssl. See #5030 (comment) for
the gory details.

And besides, we want to move to Ruby 3.1 anyway...

@jeffwidman jeffwidman requested a review from a team as a code owner July 29, 2022 06:19
@jeffwidman jeffwidman marked this pull request as draft July 29, 2022 06:19
@jeffwidman jeffwidman mentioned this pull request Jul 29, 2022
Merged
@jeffwidman jeffwidman mentioned this pull request Jul 29, 2022
Closed
@jeffwidman jeffwidman force-pushed the bump-ruby-to-3.1 branch 2 times, most recently from 74aba82 to 45d2f7c Compare August 1, 2022 19:16
jeffwidman
jeffwidman commented Aug 1, 2022
View reviewed changes
pub/dependabot-pub.gemspec
@@ -24,6 +24,8 @@ Gem::Specification.new do |spec|

spec.add_dependency "dependabot-common", Dependabot::VERSION

spec.add_development_dependency "webrick", ">= 1.7"
Copy link
Member Author

Choose a reason for hiding this comment

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

webrick was removed from the ruby 3.0 stdlib, so have to manually add it as a dependency.

jeffwidman
jeffwidman commented Aug 2, 2022
View reviewed changes
.rubocop.yml
@@ -154,6 +154,8 @@ Style/HashExcept:
Enabled: true
Style/HashLikeCase:
Enabled: false
Style/HashSyntax:
EnforcedShorthandSyntax: either
Copy link
Member Author

@jeffwidman jeffwidman Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This allows but doesn't require the new shorthand syntax sugar. The new syntax is a bit controversial... see detailed notes in the relevant commit message.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this makes sense to me. I'm not quite sure how I feel about the new syntax, in js/ts I think it's fine so probably just takes a bit of getting used to. Either way I probably would avoid major syntax style changes in this upgrade, and once everything works, re-evaluate on that stuff

jeffwidman
jeffwidman commented Aug 3, 2022
View reviewed changes
common/dependabot-common.gemspec Outdated
spec.required_ruby_version = ">= 2.7.0"
spec.required_rubygems_version = ">= 2.7.3"
spec.required_ruby_version = ">= 3.1.0"
spec.required_rubygems_version = ">= 3.3.3"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the minimum rubygems version required by ruby 3.1.

Copy link
Member

@jurre jurre Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we'd want to bump it in the Dockerfile in that case?

dependabot-core/Dockerfile

Line 74 in 8cdbf1e

ARG RUBYGEMS_SYSTEM_VERSION=3.2.20

I'd opened #5383, but apparently there were some issues with a past update of this

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, already bumped it there: https://github.com/dependabot/dependabot-core/pull/5447/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557L74

The reason it's bumped here is because otherwise Rubocop complains that spec.required_ruby_version is not >= to TargetRubyVersion

Copy link
Member Author

Choose a reason for hiding this comment

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

Just noticed your note about the past update... thx! We'll need to figure out what the root cause was as we have to bump rubygems in order to bump ruby... looks like #5044 (comment) and following discussion is a good place to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is that with #4973 these issues might no longer happen after upgrading RubyGems.

The rationale is that, if I understood correctly, those issues (and the RubyGems changes that caused them) were related to the implicit Bundler activation mechanism in RubyGems. However, with #4973 Dependabot will no longer rely on that, but will activate Bundler explicitly instead (through gem "bundler", "~> 1.17", for example).

I guess we need to try and see.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the root cause of the updater specs failing is this bumping of Ruby gems... more specifically as outlined in #5513:

When upgrading RubyGems to 3.3+, RubyGems gets confused about which Bundler version to use when shelling out to native helpers.

Which is exactly the problem I observe in #5447 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

I split the ruby gems upgrade out into a separate PR that needs to land before this one: #5823

That way it's easier for us to debug any possible issues as Rubygems vs Ruby issues.

@jeffwidman jeffwidman marked this pull request as ready for review August 3, 2022 07:29
@jeffwidman jeffwidman requested a review from a team August 3, 2022 07:29
@jeffwidman jeffwidman mentioned this pull request Aug 3, 2022
Merged
jeffwidman
jeffwidman commented Aug 3, 2022
View reviewed changes
.rubocop.yml
Style/MutableConstant:
Enabled: false
Style/RedundantFreeze:
Enabled: false
Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #5468 to re-enable these checks and fix the existing violations.

@jeffwidman jeffwidman force-pushed the bump-ruby-to-3.1 branch 2 times, most recently from 4e9f055 to 943477a Compare August 3, 2022 22:01
jeffwidman
jeffwidman commented Aug 3, 2022
View reviewed changes
.rubocop.yml Outdated Show resolved Hide resolved
@jeffwidman jeffwidman force-pushed the bump-ruby-to-3.1 branch 2 times, most recently from 0bc28ca to 03db314 Compare August 7, 2022 22:05
@jeffwidman jeffwidman marked this pull request as draft August 8, 2022 01:35
@jeffwidman jeffwidman marked this pull request as ready for review August 8, 2022 16:50
@jeffwidman
Copy link
Member Author

I think the remaining CI failure might be related to rubygems/rubygems#5812...

@jeffwidman
Copy link
Member Author

It looks like the failing test for bundler is because dependency_rubygems_uri now points at an unstubbed URL: "https://rubygems.org/api/v1/versions/bundler.json"

Heading to bed now, but tomorrow I'll check what this URL used to point at on main and why/when the change.

@deivid-rodriguez
Copy link
Contributor

I think that one will be fixed by 629d1f0!

@jeffwidman
Copy link
Member Author

I think you're on the right track, but unfortunately just cherry-picking that commit didn't fix it... I'll have to dig a little deeper.

@jeffwidman
Copy link
Member Author

jeffwidman commented Sep 30, 2022 •
edited
Loading

The failing updater specs were caused by unable to find Bundler v1:

(rdbg) stderr
"/usr/local/lib/ruby/site_ruby/3.1.0/rubygems/dependency.rb:313:in `to_specs': Could not find 'bundler' (~> 1.17) - did find: [bundler-2.3.22] (Gem::MissingSpecVersionError)\nChecked in 'GEM_PATH=/opt/bundler/v1/.bundle' , execute `gem env` for more information\n\tfrom /usr/local/lib/ruby/site_ruby/3.1.0/rubygems/dependency.rb:323:in `to_spec'\n\tfrom /usr/local/lib/ruby/site_ruby/3.1.0/rubygems/core_ext/kernel_gem.rb:62:in `gem'\n\tfrom /opt/bundler/v1/run.rb:3:in `<main>'\n"

Then response = JSON.parse(stdout) fails since stdout is empty...

The root cause turned out to be due to the Rubygems upgrade, which we have to do in order to bump to Ruby 3.1... it requires a Rubygems >= 3.3.3.

It turns out that #5513 solves that issue, so I split out the Rubygems upgrade into #5823. And then rebased this PR on top of that.

The remaining test failure (#5447 (comment)) also occurs in #5823, so I suspect once we figure out the problem with the Rubygems upgrade, then that will unblock this PR as well.

@jeffwidman jeffwidman mentioned this pull request Sep 30, 2022
Merged
@jeffwidman jeffwidman force-pushed the bump-ruby-to-3.1 branch 3 times, most recently from 137c8d4 to 7f901f4 Compare October 1, 2022 04:31
@jeffwidman jeffwidman mentioned this pull request Oct 1, 2022
Merged
@jeffwidman jeffwidman force-pushed the bump-ruby-to-3.1 branch 2 times, most recently from 44c4d5a to f3d9119 Compare October 7, 2022 01:34
pavera
pavera approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@pavera pavera left a comment

Choose a reason for hiding this comment

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

FWIW, LGTM :)

@jeffwidman jeffwidman force-pushed the bump-ruby-to-3.1 branch 2 times, most recently from 0c98d6c to b2f6bcc Compare October 7, 2022 17:45
@jeffwidman @mctofu
Bump Ruby to 3.1
2badf4b 
This is necessary to unblock
dependabot#5030, because Ubuntu
`22.04` doesn't provide an older version of `openssl`, so need a new
enough Ruby to work with the newer version of `openssl`.
See dependabot#5030 (comment)
for the gory details.

And besides, we want to move to Ruby 3.1 anyway...
@jeffwidman @mctofu
Drop debase, it doesn't yet support Ruby 3.1
a4dc9e6 
This dependency was added in dependabot#5086.

However, `debase` doesn't currently support Ruby 3 or 3.1: ruby-debug/ruby-debug-ide#223 (comment)

Ruby 3.1 now bundles the `debug` gem. This new `debug` gem has
functionality that replaces `debase` + the `ruby-debug-ide` gem so I
removed those as well.

Even though the `debug` gem is bundled, we still need to pin it in the
`gemspec` file because we use a customized `GEM_PATH`. The `debug` gem
bundled with Ruby by default is no longer found since it lives in the
default `GEM_PATH`.
@jeffwidman @mctofu
webrick must be manually installed
4caa170 
`webrick` was removed from the `ruby` `3.0` stdlib, so must be manually
installed.

It's only needed by `pub`, and only as a dev server used in tests, so
added it only for that specific ecosystem as a development dependency.
@jeffwidman @mctofu
Allow but don't require omitting hash values
6dbec51 
Ruby 3.1 added some syntax sugar for omitting hash values in certain
cases. This results in more terse code, at the expense of readability.

Like any good style change, this is obviously bikeshed worthy, and has
generated a lot of controversy around the community:
* https://batsov.com/articles/2022/01/20/bad-ruby-hash-value-omission/
* standardrb/standard#375
* Shopify/ruby-style-guide#393
* https://github.com/rubocop/ruby-style-guide#hash-literal-values

Personally, I can see places where it's useful, but also places where it
just makes the Ruby a bit more inscrutable.

So set the Rubocop value to `either` to allow, but not require it.
@jeffwidman @mctofu
Temp disable freezing/mutable Rubocop checks
2b7808f 
Temp disable these two checks as they are flagging a _lot_ of
violations. I plan to fix them, but prefer to do so in a follow-on PR in
order to keep the diff of this PR small / focused on Ruby 3.1 changes.
@jeffwidman @mctofu
debug replaces byebug
925c43d
@jeffwidman jeffwidman merged commit fed6d1d into dependabot:main Oct 7, 2022
@jeffwidman jeffwidman deleted the bump-ruby-to-3.1 branch October 7, 2022 22:25
@jeffwidman jeffwidman mentioned this pull request Oct 8, 2022
Merged
@pavera pavera mentioned this pull request Oct 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@landongrindheim landongrindheim landongrindheim left review comments

@deivid-rodriguez deivid-rodriguez deivid-rodriguez left review comments

@jurre jurre jurre approved these changes

@pavera pavera pavera approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeffwidman @deivid-rodriguez @pavera @jurre @landongrindheim