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

Improving the Ruby Feature. #764

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

ThomasOwens
Copy link

This is a work-in-progress draft pull request demonstrating some improvements to the Ruby feature to address #603 and #757.

In the current state, the Ruby Feature now has parity with the Node Feature. The issue where multiple Ruby version managers have been installed has been solved. Currently, RVM is used. Although I prefer rbenv (and the poll seems to perhaps favor rbenv as well), I noticed the Node Feature uses NVM and doesn't provide an option for nodenv or any other Node version managers. Since Node is frequently used with Ruby for web development (Rails, Sinatra), it seemed to make since to make the first step resolving the key problem and making it mimic the Node Feature.

keyserver hkp://pgp.mit.edu
keyserver hkp://ipv4.pool.sks-keyservers.net
keyserver hkp://keyserver.ubuntu.com
keyserver hkp://keyserver.ubuntu.com:80"
Copy link
Author

Choose a reason for hiding this comment

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

I added the keyservers recommended in the RVM documentation in addition to the ones previously included.

test/ruby/scenarios.json Outdated Show resolved Hide resolved
@@ -10,12 +10,17 @@
"proposals": [
Copy link
Author

Choose a reason for hiding this comment

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

Implementing an os-provided Ruby is difficult.

The mcr.microsoft.com/devcontainers/base:bullseye image does not include Ruby. The Ruby devcontainer image (images/src/ruby/.devcontainer/Dockerfile in the devcontainer/images repository) is based upon a Ruby image from the official Ruby Docker images and not an image in the Microsoft container registry, which does include an installed Ruby. This is why my (not yet committed) test and change to handle this os-provided option isn't working.

I'm even hesitant to include a "none" option. But I did aim for parity with the Node Feature, and it offers a none option which just installs NVM. So I carried that option over here. I did notice, though, that Node also has the issue where their devcontainer image is based on an official Node docker image and not an image in the MCR.

@nevans
Copy link

nevans commented Dec 18, 2023

Copied from #757 (comment), although I did make a few additional (mostly minor) edits to this comment:

@ThomasOwens Thank you so much for taking this on! To summarize my current thoughts here:

As an incremental improvement, this is fine. But if we are going to bump the version number for backward incompatibility, I personally vote for delaying until we can make rvm fully optional. This is only a minor improvement over the current version: rbenv is unusable now, and (after this PR, in its current form) rbenv will still be unusable without more backward incompatible changes.

I personally consider the "minimum feature set" to be:

  • Don't set GEM_HOME or GEM_PATH at all. They will screw up alternate version managers or other customization added by the user (either before or after this feature). (Adding rvm or rbenv specific directories to PATH should be just fine.)
  • Support os-provided, at least for apt-based distributions. Perhaps the code for this can be copied from other features.
  • Use the version of ruby that's already on the PATH, if the version number matches the requested version. This allows the ruby feature to cooperate with the official docker hub ruby image or the user's own custom base image. Perhaps the code for this can be copied from other features.
  • When the version of the available ruby does not match the requested version, install the requested version somehow that won't break (existing or future) standard installs of rvm or rbenv. How it's installed is not part of my "minimum feature set", so long as my env vars constraint is met. 🙂

My recommendation on how to satisfy my "minimum feature set":

  • Do not install either rvm or rbenv (unless requested by the user by a config option).
  • Do install ruby-build to /usr/local, unless it's already installed on the PATH or (perhaps) disabled by a config option. As far as I can tell, this is the only version management tool that is actively supported by the core teams of CRuby, JRuby, and TruffleRuby. It works just fine without rbenv. And, even if it is unused, it isn't large and installing it shouldn't hurt anything.
  • When necessary, install from source using ruby-build to either /opt/ruby or /usr/local. Ensure the default PATH can find all of ruby's bins.

I'm not sure if it should be part of the "minimum feature set", but I think we should probably do one of the following, to aid with backward-incompatible upgrade pains:

  • Ensure that the user can use rvm, even with GEM_HOME and GEM_PATH unset. Check the feature against at least a two base images: the default devcontainer base image, and the default docker hub ruby image. I suspect rvm might work without those env vars on the default devcontainer images, but it might break if the user bases the feature on the docker hub ruby image (which also sets these env vars!!!).
  • Document that users may need to set these environment variables themselves to make everything play nicely with rvm.

None of the following are in my personal minimum feature set... but they would all be nice to have:

  • If you make the install location user-configurable, document that variable as only applying to the ruby installed by ruby-build (i.e: not the os-provided version, to any pre-installed version, nor to any version installed via rvm or rbenv). Document that the user may need to update their PATH to match any custom install location.
  • If the OS provided version matches the requested version but isn't installed, install it (at least for apt distros). The version detection code in other official features can probably be copied to accomplish this.
  • A config option to install a version manager: none or rvm or rbenv (or chruby).
  • Install via rvm when rvm is installed (by the feature, or already on PATH).
  • Install via rbenv install when rbenv is installed (by the feature, or already on PATH).
  • If we need (or want) any further customization to how our default installation of rubygems behaves, use their recommended 'operating_system.rb' and not environment variables. Environment variables are great for overrides, but (IMO) they are unsuitable for defaults.

What do you think? (My apologies: I'll probably be too busy with other end-of-year stuff both at work and in my personal time, and I probably can't really offer any more than my criticism right now.)

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.

None yet

2 participants