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

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

Merged

Conversation

martinemde
Copy link
Member

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?

  • Increase test coverage to explicitly test what is and is not supported.
  • Add support for ruby-3.2.2 with the ruby- prefix, which is common and easy to support.

Make sure the following tasks are checked

Increase test coverage and be explicit about what is and is not supported.
@martinemde martinemde force-pushed the martinemde/improve-ruby-file-instruction-coverage branch from 1f1d6d2 to a096397 Compare September 13, 2023 03:50
@martinemde martinemde changed the title Gemfile ruby file: supports more version formats Gemfile ruby file: supports ruby-3.2.2 but not 3.2.2@gemset Sep 13, 2023
Comment on lines -28 to +30
args << Array(ruby_version_arg) if ruby_version_arg
args << ruby_version_arg if ruby_version_arg
Copy link
Member Author

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\"]")
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 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.

Copy link
Contributor

@pboling pboling left a 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)
Copy link
Contributor

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
Copy link
Member

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?

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 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?

Copy link
Contributor

@pboling pboling Sep 14, 2023

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).

@martinemde martinemde merged commit 71ef923 into master Sep 14, 2023
92 checks passed
@martinemde martinemde deleted the martinemde/improve-ruby-file-instruction-coverage branch September 14, 2023 19:43
@deivid-rodriguez deivid-rodriguez changed the title Gemfile ruby file: supports ruby-3.2.2 but not 3.2.2@gemset Add support for the ruby-3.2.2 format in the ruby file: Gemfile directive, and explicitly test the 3.2.2@gemset as rejected Sep 21, 2023
@deivid-rodriguez deivid-rodriguez changed the title Add support for the ruby-3.2.2 format in the ruby file: Gemfile directive, and explicitly test the 3.2.2@gemset as rejected 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 Sep 21, 2023
deivid-rodriguez pushed a commit that referenced this pull request Sep 21, 2023
…nstruction-coverage

(cherry picked from commit 71ef923)
deivid-rodriguez pushed a commit that referenced this pull request Sep 21, 2023
…nstruction-coverage

(cherry picked from commit 71ef923)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants