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

[#5222/#5223] Drop old rubies #6052

Merged
merged 6 commits into from
May 18, 2021
Merged

[#5222/#5223] Drop old rubies #6052

merged 6 commits into from
May 18, 2021

Conversation

gbp
Copy link
Member

@gbp gbp commented Jan 6, 2021

Relevant issue(s)

Closes #5222
Closes #5223
Required for #5159
Preferable for #5830

What does this do?

Drops support for Ruby 2.3 and 2.4

Why was this needed?

These old Rubies are EOL and blocking upgrades to Rails and other gems

Implementation notes

We want to retain support for installing on Stretch which comes with ruby 2.3. So this PR uses rbenv/ruby-build to install a support version of Ruby when the default package is too old.

Todo

  • update installer scripts to install rbenv/ruby-build
  • install a later version of ruby
  • remove old ruby version conditionals
  • update daemons to use new ruby
  • update crontab to use new ruby

@gbp
Copy link
Member Author

gbp commented Jan 6, 2021

@sagepe as discussed, this is the current work done on dropping support for old rubies.

@sagepe
Copy link
Member

sagepe commented Apr 20, 2021

@gbp - for the time being this is probably the best approach if you don't want to reman blocked. I might end up packaging updated Ruby versions for Debian, but it's not a priority right now. I guess we could add support for the Brightbox Ruby packlages for Ubuntu to this install script if a supported version of Ubuntu is detected, but that might be more complexity than is necessary in the immediate term.

gbp added a commit that referenced this pull request Apr 20, 2021
Out ugly implementation isn't fully complete compared to the mySociety
deployment scripts where Ugly templates are parsed using Perl.

Switching to ERB allows us to use conditionals. This will allow us to
selectively use rbenv for daemons and crontab.

See #6052
gbp added a commit that referenced this pull request Apr 20, 2021
Our Ugly implementation isn't fully complete compared to the mySociety
deployment scripts where Ugly templates are parsed using Perl.

Switching to ERB allows us to use conditionals. This will allow us to
selectively use rbenv for daemons and crontab.

See #6052
@gbp gbp force-pushed the drop-old-rubies branch from 404a43a to 6f27340 Compare May 2, 2021 13:14
@gbp gbp marked this pull request as ready for review May 2, 2021 13:14
@gbp
Copy link
Member Author

gbp commented May 4, 2021

@garethrees I think everything needed to get this merged is now done. I think the only commit which need reviewing is 6f27340 as @sagepe has already taken a look at the install scripts.

Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

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

Seems to work well! 🐎

Couple of comments inline, but at the minimum we'll want to add some upgrade notes.

We should give a short high-level description of what we're doing (there's probably a prior example from dropping 1.8.7), but there were some specifics I noticed that would be non-obvious.

  1. Our upgrade guidance says to run script/rails-post-deploy, but this change requires the full installer to be run. We'll want to make sure we both give people clear instructions of how to enable the change, and not blat their systems. The example from last time we had to do this is probably a good starting point, and hopefully should be much reduced due to not needing to do anything too weird and our better handling of rbenv paths.
  2. The daemons and config files will need to be manually regenerated with USE_RBENV set.
  3. If transitioning from an existing install rake errors out (see below). The solution is to run bundle pristine.
    default: Installing rspec-rails 5.0.1
    default: Bundle complete! 86 Gemfile dependencies, 168 gems now installed.
    default: Bundled gems are installed into `/home/vagrant/bundle/`
    default: rake aborted!
    default: LoadError: incompatible library version - /home/vagrant/bundle/ruby/2.5.0/gems/thin-1.8.0/lib/thin_parser.so
    default: /home/vagrant/bundle/ruby/2.5.0/gems/activesupport-5.2.5/lib/active_support/dependencies.rb:291:in `require'
    default: /home/vagrant/bundle/ruby/2.5.0/gems/activesupport-5.2.5/lib/active_support/dependencies.rb:291:in `block in require'
    default: /home/vagrant/bundle/ruby/2.5.0/gems/activesupport-5.2.5/lib/active_support/dependencies.rb:257:in `load_dependency'
    default: /home/vagrant/bundle/ruby/2.5.0/gems/activesupport-5.2.5/lib/active_support/dependencies.rb:291:in `require'
    default: /home/vagrant/bundle/ruby/2.5.0/gems/thin-1.8.0/lib/thin.rb:39:in `<top (required)>'
    default: /home/vagrant/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/bundler-2.2.17/lib/bundler/runtime.rb:66:in `require'
    default: /home/vagrant/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/bundler-2.2.17/lib/bundler/runtime.rb:66:in `block (2 levels) in require'
    default: /home/vagrant/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/bundler-2.2.17/lib/bundler/runtime.rb:61:in `each'
    default: /home/vagrant/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/bundler-2.2.17/lib/bundler/runtime.rb:61:in `block in require'
    default: /home/vagrant/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/bundler-2.2.17/lib/bundler/runtime.rb:50:in `each'
    default: /home/vagrant/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/bundler-2.2.17/lib/bundler/runtime.rb:50:in `require'
    default: /home/vagrant/.rbenv/versions/2.5.8/lib/ruby/gems/2.5.0/gems/bundler-2.2.17/lib/bundler.rb:173:in `require'
    default: /home/vagrant/alaveteli/config/application.rb:21:in `<top (required)>'
    default: /home/vagrant/alaveteli/rakefile:4:in `require_relative'
    default: /home/vagrant/alaveteli/rakefile:4:in `<top (required)>'
    default: /home/vagrant/bundle/ruby/2.5.0/gems/rake-13.0.3/exe/rake:27:in `<top (required)>'
    default: (See full trace by running task with --trace)

@@ -34,7 +34,7 @@ clear_daemon() {

install_daemon() {
echo -n "Creating /etc/init.d/$SITE-$1... "
(su -l -c "cd '$REPOSITORY' && bundle exec rake config_files:convert_init_script DEPLOY_USER='$UNIX_USER' VHOST_DIR='$DIRECTORY' RUBY_VERSION='$RUBY_VERSION' SCRIPT_FILE=config/$1-debian.example" "$UNIX_USER") > /etc/init.d/"$SITE-$1"
(su -l -c "cd '$REPOSITORY' && bundle exec rake config_files:convert_init_script DEPLOY_USER='$UNIX_USER' VHOST_DIR='$DIRECTORY' VCSPATH='$SITE' SITE='$SITE' RUBY_VERSION='$RUBY_VERSION' USE_RBENV=$USE_RBENV RAILS_ENV='production' SCRIPT_FILE=config/$1-debian.example" "$UNIX_USER") > /etc/init.d/"$SITE-$1"
Copy link
Member

Choose a reason for hiding this comment

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

How come we've added RAILS_ENV=production in this commit?

Copy link
Member

Choose a reason for hiding this comment

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

Also VCSPATH and SITE on this line? How come these are needed now vs before? (I'm sure there's a good reason, but I don't understand from looking at the commit in isolation)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. IIRC this is makes the rake config_files:convert... calls consistent. Not sure if it really is necessary

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying we shouldn't do it; I just think it should be explained (and its own commit is probably a better place to do that).

The addition of RAILS_ENV=production would resolve #2392, and as far as I can tell, that's what we should be doing. I do remember something confusing about all this, but can't find any specific reason why we shouldn't make this specific change. Maybe it was #2242 (comment)? That doesn't really suggest that we shouldn't make this change, since we only generate /etc/init.d/$SITE if we're not creating a $DEVELOPMENT_INSTALL.

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've split this change into 3 commits:

  • 4a9601c with the new USE_RBENV variable,
  • 7180f42 makes the variables consistent,
  • 245833e adds the RAILS_ENV variable, now based on DEVELOPMENT_INSTALL

script/install-as-user Show resolved Hide resolved
README.md Show resolved Hide resolved
@gbp gbp force-pushed the drop-old-rubies branch from 6f27340 to 245833e Compare May 12, 2021 08:44
@gbp gbp merged commit 4dd8339 into develop May 18, 2021
@garethrees
Copy link
Member

Hurrah!!! 🍾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop support for Ruby 2.4 Drop support for Ruby 2.3
3 participants