Fix required_ruby_version with prereleases and improve error message

[deivid-rodriguez]

Description:

Fixes #2343, by making the required_ruby_version gemspec setting work with prereleases and shows a better error message when the check does not pass.

Not sure how stable the RUBY_DESCRIPTION string is across ruby releases, maybe I can check the format is what we expect?

---

Tasks:

• Describe the problem / feature
• Write tests
• Write code to solve the problem
• Get code review from coworkers / friends

I will abide by the code of conduct.

[deivid-rodriguez]

Sorry for the big diff in this method, this is the gotcha of this aligment style: keeping it obscures the point of commits.

[MSP-Greg]

@deivid-rodriguez

I haven't had the time to think about this, and I'm not sure of it's application. I'm also not sure about how many people use trunk builds or preview builds. If it's as few as I think, most are probably capable of building/installing a gem from a specific git repo branch, so requirements don't need to be tight like a published gem. I think that made sense...

While gem releases are a discrete set, trunk is a little different. In some respects, I think a requirement for trunk/head should be done in terms of the svn/revision, not whether it's dev, preview, or whatever.

Either way, the message and to some respects, even gem env could provide more helpful info.

[deivid-rodriguez]

Yeah, I see your point.

But when we read the issue, we seem to all acknowledged this as a bug and expected this to work with a prerelease (spec.required_ruby_version = ">= 2.6.0.preview2"). We're used to rubygems not knowing about specific revisions of gems (only bundler can handle that), but knowing about gem pre-releases, so it seems reasonable that rubygems can equally handle ruby prereleases in required_ruby_version, even if it can't really compare specific revisions.

With this patch, rubygems resolves 2.6.0.preview1 < 2.6.0.preview2 < 2.6.0.rc1 < 2.6.0.rc2 < 2.6.0. I think that's good enough. Aiming for correct comparison with specific revisions is out of the scope, since it's not even defined how the required_ruby_version option would look if we allowed specifying revisions.

[MSP-Greg]

I know, just kick the trunk guys to the curb...

You're right. One thing, I dl'd a preview tarball, and, as in trunk, in RUBY_DESCRIPTION, there isn't a period between RUBY_VERSION and RUBY_PATCHLEVEL_STR.

IOW, in RUBY_DESCRIPTION, I believe it's 2.6.0preview2, not 2.6.0.preview2.

Thanks, Greg

[deivid-rodriguez]

Correct, that's how I'm parsing it from RUBY_DESCRIPTION. But to play nice with how rubygems interprets versions, every segment needs to be separated with dots, I think, otherwise rubygems considers 2.6 as the release, and 0preview2 as the prerelease segment.

[MSP-Greg]

@deivid-rodriguez

Sorry, I just saw the change in Gem.ruby_version (added period):

version << ".#{RUBY_DESCRIPTION.match(/\Aruby #{RUBY_VERSION}([^ ]+) /)[1]}"

Will \Aruby be an issue with JRuby? Maybe \A[a-z]+?

[deivid-rodriguez]

Nice catch! Too bad that we don't test against jruby 😢

So I tried it and jruby keeps MRI constants to define the version of the ruby specification they support (RUBY_VERSION, RUBY_PATCHLEVEL), and uses extra variables for its own versioning (RUBY_ENGINE, RUBY_ENGINE_VERSION). So in my opinion the correct thing to do is to keep required_ruby_version for MRI versioning (RUBY_VERSION, RUBY_PATCHLEVEL), which is actually what defines the compatibiliy of the language, so it makes more sense.

I added a spec to "prove" what the behavior would be for jruby and that the current implementation would work.

[deivid-rodriguez]

Actually this could break if a non mri release had minimum compatibility with a prerelease of MRI (in that case I guess RUBY_PATCHLEVEL would be -1 and the regexp would break). I don't know if such a ruby implementation exists or will exist, but to be safest we should probably use a separate regexp for non mri RUBY_DESCRIPTIONs?

[MSP-Greg]

I don't know if such a ruby implementation exists or will exist

Neither do I. Nor do I know if non mri's have previews, betas, rc's, etc, which is what the regex is picking up now?

and the regexp would break

By break, return nil? Could always or it with ""...

[deivid-rodriguez]

The RUBY_DESCRIPTION in the case of jruby looks like this: "jruby 9.2.1.0 (2.5.0)...", that's why we'd need to change the regexp if the case I mentioned before wants to be supported. Otherwise we'd call [] on nil since the regexp won't match, I think.

[MSP-Greg]

The RUBY_DESCRIPTION in the case of jruby looks like this: "jruby 9.2.1.0

I should have been clearer. What about if it's a preview/beta/rc? I don't know myself...

Maybe change

version << ".#{RUBY_DESCRIPTION.match(/\Aruby #{RUBY_VERSION}([^ ]+) /)[1]}" 
 -- to --
version << ".#{RUBY_DESCRIPTION[/\A[a-z]+ #{RUBY_VERSION}([^ ]+) /, 1] || ''}"

Not sure if the ending period will mess things up, but I don't think so...

[deivid-rodriguez]

The thing is we're not trying to match the version of jruby but the version of the compatible MRI counterpart, namely, the string between the parentheses. At least that would seem like the most reasonable behavior to me.

[MSP-Greg]

I just looked at JRuby versions of Puma & Eventmachine, and neither seemed to have 'ruby' version requirements.

At least that would seem like the most reasonable behavior to me.

With different ruby engines, (ruby vs jruby), I don't know...

Hence, LGTM...

[deivid-rodriguez]

I think requiring a minimum engine version would be a separate feature that was proposed in the past, but rejected because the ecosystem was not ready yet. See #1468.

[ioquatix]

Thanks for working on this and I really appreciate everyones effort sorting this out.

[segiddins]

This seems reasonable, but I really dont have enough experience with the different ruby version constants set by different implementations to be confident this will work universally. I'd like those with more relevant experience to weigh in.

[deivid-rodriguez]

Yeah, I also wonder if we should add a feature request to ruby so that they expose this info and we don't have to parse RUBY_DESCRIPTION.

[MSP-Greg]

Yeah, I also wonder if we should add a feature request to ruby so that they expose this info and we don't have to parse RUBY_DESCRIPTION

Considering this, and looking over test_case.rb and the handling of ENV & RUBY_ elements, might a later PR refactor the code to use constant arrays holding all the elements that are saved/restored?

At present, a change to either results in changes to both the save and restore portions of the code, where using arrays would require one change.

IOW, use arrays for the names of all the ENV & RUBY_ items saved/restored, and hashes like orig_env and orig_ruby_info for the original values, rather than all the separate instance variables...

[hsbt]

@bundlerbot r+

[ghost]

Build succeeded

• continuous-integration/travis-ci/push