-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for the ruby-3.2.2
format in the ruby file:
Gemfile directive, and explicitly test the 3.2.2@gemset
format as rejected
#6954
Conversation
Increase test coverage and be explicit about what is and is not supported.
1f1d6d2
to
a096397
Compare
ruby file:
supports more version formatsruby file:
supports ruby-3.2.2
but not 3.2.2@gemset
args << Array(ruby_version_arg) if ruby_version_arg | ||
args << ruby_version_arg if ruby_version_arg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to be slightly more accurate in tests because the arg is not always given as an array in the Gemfile.
allow(Bundler).to receive(:read_file).with(project_root.join("foo")).and_return("nodejs 18.16.0\nruby #{version} # This is a comment\npnpm 8.6.12\n") | ||
allow(Bundler).to receive(:root).and_return(Pathname.new("/path/to/project")) | ||
it "raises an error" do | ||
expect { subject }.to raise_error(Gem::Requirement::BadRequirementError, "Illformed requirement [\"#{version}@gemset\"]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I covered the reason for rejecting @gemset
in the linked ticket. TL;DR rvm says it's incompatible with other version managers and you should use .ruby-gemset
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
def normalize_ruby_file(filename) | ||
file_content = Bundler.read_file(Bundler.root.join(filename)) | ||
# match "ruby-3.2.2" or "ruby 3.2.2" capturing version string up to the first space or comment | ||
if /^ruby(-|\s+)([^\s#]+)/.match(file_content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳 Thanks!
# Support the various file formats found in .ruby-version files. | ||
# | ||
# 3.2.2 | ||
# ruby-3.2.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about other ruby implementations, like jruby or truffleruby?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit weird since specifying jruby or truffleruny require 3 values: version, engine, and engine version. I tested a bunch of ways to allow this. Bundler needs version, engine version and engine. .ruby-version doesn't support syntax like that.
One of the not-great solutions I see is putting engine: and engine_version: in the options with file:, and having file be the underlying ruby version that is given by the engine. Bundler doesn't like getting the jruby's engine version as the ruby version, as far as I found in testing, so we'd have to load the ruby version from the environment if .ruby-version had jruby-0.9.2.0.
Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With asdf
, rbenv
, and ruby-build
only MRI rubies are the simple version.
Alternate values are the same between asdf
, rbenv
, and ruby-build
(which is what asdf-ruby
uses under the hood).
As such, it seems likely that all of these variations could appear in both .ruby-version
and .tool-versions
.
Many of those are not numeric at all, or are numeric but the numbers are not based on MRI versions:
artichoke-dev
jruby-dev
jruby-1.7.27
jruby-9.4.3.0
maglev-2.0.0-dev
mruby-dev
mruby-3.2.0
picoruby-3.0.0
rbx-5.0
ree-1.8.7-2012.02
truffleruby-dev
truffleruby-23.0.0
truffleruby+graalvm-dev
truffleruby+graalvm-23.0.0
This would be a decent set of tokens to test a solution against (after adding in some of the basic MRI options).
ruby file:
supports ruby-3.2.2
but not 3.2.2@gemset
ruby-3.2.2
format in the ruby file:
Gemfile directive, and explicitly test the 3.2.2@gemset
as rejected
ruby-3.2.2
format in the ruby file:
Gemfile directive, and explicitly test the 3.2.2@gemset
as rejectedruby-3.2.2
format in the ruby file:
Gemfile directive, and explicitly test the 3.2.2@gemset
format as rejected
…nstruction-coverage (cherry picked from commit 71ef923)
…nstruction-coverage (cherry picked from commit 71ef923)
What was the end-user or developer problem that led to this PR?
Follow up comments in issue #6876 brought up missing support in
.ruby-version
.What is your fix for the problem, implemented in this PR?
ruby-3.2.2
with theruby-
prefix, which is common and easy to support.Make sure the following tasks are checked