Gem::Requirement.new in required_ruby_version raises BadRequirementError when version is read from elsewhere #12579
Comments
|
With Ruby 2.7.8 / bundler 2.4.22, it was still working With Ruby 2.7.8 / bundler 2.4.22, these version updates it stopped working with the following error: |
|
Thank you for providing that pointer, @Earlopain . You're correct that my PR #12732 is definitely relevant to this issue. I think that #12732 does indeed essentially fix the original problem raised by @CamJN . It does avoid the exception that @CamJN reported. That being said, though, the spec provided by @CamJN fails, even on latest First of all, we should slightly tweak @CamJN's spec, to this: context 'when file contains `required_ruby_version` as an interpolated string' do
it 'sets target_ruby to the minimal version satisfying the requirements' do
content = <<~HEREDOC
ruby_version = "2.6"
Gem::Specification.new do |s|
s.name = 'test'
s.required_ruby_version = ">= \#{ruby_version}.0"
s.licenses = ['MIT']
end
HEREDOC
create_file(gemspec_file_path, content)
expect(target_ruby.version).to eq 2.6
end
endThe most notable change there is that I've added a backslash to escape the interpolation within the heredoc: rubocop/lib/rubocop/rspec/cop_helper.rb Lines 9 to 12 in 6a4cf81 On latest This is because the current RuboCop code doesn't determine a target Ruby version based on |
|
Regarding the error mentioned by @capripot , unfortunately, that is not fixed by #12732, because @capripot's example has an array wrapping the Ruby version specification in the gemspec, which is not something that #12732 handles. This spec fails on latest context 'when file contains `required_ruby_version` as an interpolated string in an array' do
it 'uses the default target ruby version' do
content = <<~HEREDOC
Gem::Specification.new do |s|
s.name = 'test'
s.required_ruby_version = [">= \#{File.read(File.expand_path(".ruby-version", __dir__)).strip}"]
s.licenses = ['MIT']
end
HEREDOC
create_file(gemspec_file_path, content)
expect(target_ruby.version).to eq default_version
end
endwith this error However, that spec will pass if we remove the array wrapping around the |
|
Ah right, my bad, I only checked the example from OP. Good point with the interpolation happening, I didn't catch that. Do you want to make a PR for this, since you've already fixed that other case? |
|
@Earlopain : Yes, I'm working up a PR right now. Thanks for asking! |
This fixes an issue mentioned in [this comment](#12579 (comment)) on issue #12579. (I believe that the main/original issue mentioned in issue #12579 has already been fixed by #12732.) This change is a related/similar followup to #12732 (5026393). That PR avoided an exception that was occurring in the `RuboCop::TargetRuby::GemspecFile` target Ruby finder when a project's gemspec determines the value of `required_ruby_version` in some dynamic way (such as by reading from a `.ruby-version` file). However, #12732 did not handle a case where the dynamic value for the gemspec's `required_ruby_version` is wrapped within an array, and, in such a case, currently, an exception like the following will occur: ``` Gem::Requirement::BadRequirementError: Illformed requirement [">= \#{File.read('.ruby-version').rstrip}"] ./lib/rubocop/target_ruby.rb:123:in `new' ./lib/rubocop/target_ruby.rb:123:in `find_default_minimal_known_ruby' ./lib/rubocop/target_ruby.rb:83:in `find_version' ./lib/rubocop/target_ruby.rb:29:in `initialize' ./lib/rubocop/target_ruby.rb:259:in `new' ``` (As with the scenario that _was_ fixed by #12732, I believe that this is essentially a "latent" issue that is now occurring in more projects than previously, due to #12645 having increased the precedence of the `GemspecFile` target Ruby finder in the `RuboCop::TargetRuby::SOURCES` list.) This change avoids this exception by only attempting to parse a target Ruby version from an array value for a gemspec's `required_ruby_version` if every value in the array is a string. When the values within the `required_ruby_version` array are _not_ all strings, now, rather than raising an unhandled exception, the `GemspecFile` finder will return `nil` for the target Ruby version, and so we will continue on to the other TargetRuby finder classes, in order to find a target Ruby version.
|
After updating my checkout of this repo to afddf65 which was HEAD at the time, and re-adding my test (with the Here's the rspec output. |
|
@CamJN Thanks for re-running your spec against the latest The spec failure that you are seeing now is different than the one that you were seeing before, and that new failure is expected. I mentioned why in my previous comment:
If you want to specify a RuboCop target Ruby version of |
|
Fair enough, I do think trying to parse the gemspec file rather than evaluate it is fighting against what the file is (a ruby program, not a config file), but it is very much the rubocop way of thinking about things to try to parse rather than evaluate. |
|
@CamJN : Yeah, I hear your point, and, while working on this, I did sort of wonder whether it would be possible to somehow evaluate rather than parse the gemspec file. But that might have issues of its own. For example, it's conceivable that (since it is a Ruby program, as you mentioned) actually evaluating the gemspec might produce some sort of side effect, such as writing a file on the local machine, or really anything else (such as making a request to a remote server). In this sense, RuboCop might (perhaps rightly) consider that it would be overstepping its bounds to actually evaluate the gemspec, in order to try to extract a target Ruby version from it. So, in that paradigm, we are confined to merely trying to parse the gemspec file. Thus, somewhat unfortunately, but maybe for the best, overall, we thereby limit ourselves to only being able to parse some manually specified, finite subset of the infinite possible ways that the gemspec could be written. |
Expected behavior
Rubocop should evaluate the gemspec file rather than try to parse the unevaluated Ruby.
Actual behavior
Steps to reproduce the problem
RuboCop version
The text was updated successfully, but these errors were encountered: