Fix hang when gemspec has incompatible encoding

[deivid-rodriguez]

What was the end-user problem that led to this PR?

The problem was that if a gemspec had a non ascii character on it, and the external encoding is not utf-8, bundler would fail with a very cryptic error. CircleCI is very likely to reproduce this bug since its base images default to LANG=C. See here for an example.

In bundler's master, bundle install seems to hang in this same situation. I have no idea why but CPU usage goes crazy (100%) when running something like LANG=C bundle install with such a gemspec.

Note that adding a utf-8 magic encoding comment seems to fix the problem, but I think bundler should just work in this situation, or at least give the user a more helpful error message. And of course never hang either.

What was your diagnosis of the problem?

My diagnosis was that bundler was reading an incompatible character from the gemspec and was not able to deal with it, nor give a useful error message.

What is your fix for the problem, implemented in this PR?

My fix initial fix was to at least give a better error message, preventing the method building the error message itself from blowing up due to incompatible encodings. This is the initial patch that I wrote to improve the error message:

diff --git a/lib/bundler/dsl.rb b/lib/bundler/dsl.rb
index 37bfe3674..79ef563d1 100644
--- a/lib/bundler/dsl.rb
+++ b/lib/bundler/dsl.rb
@@ -581,7 +581,14 @@ The :#{name} git source is deprecated, and will be removed in Bundler 2.0.
 
       def parse_line_number_from_description
         description = self.description
-        if dsl_path && description =~ /((#{Regexp.quote File.expand_path(dsl_path)}|#{Regexp.quote dsl_path.to_s}):\d+)/
+        return [nil, description] unless dsl_path
+
+        quoted_expanded_dsl_path = Regexp.quote(File.expand_path(dsl_path))
+        quoted_dsl_path = Regexp.quote(dsl_path.to_s)
+
+        return [nil, description] unless Encoding.compatible?(quoted_dsl_path, description) && Encoding.compatible?(quoted_expanded_dsl_path, description)
+
+        if description =~ /((#{quoted_expanded_dsl_path}|#{quoted_dsl_path}):\d+)/
           trace_line = Regexp.last_match[1]
           description = description.sub(/#{Regexp.quote trace_line}:\s*/, "").sub("\n", " - ")
         end

With that patch, the error would be more clear, pointing at the exact line in the gemspec where the incompatible character was found.

However, after that I considered that maybe bundler doesn't need to read the gemspec as a text file, but could read it "binarily". That seemed to fix the problem indeed and made the bundle install succeed in this situation.

Why did you choose this fix out of the possible options?

I chose this fix because not only fixes the error message but also seems to make bundler just work.

[ghost]

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

[deivid-rodriguez]

I run the specs on my fork and things seem to be ok, except that binread is not available on 1.8.7. I was very surprised that you still run specs against 1.8.7 :O. If you're ok with the patch, let me know how you would like to deal with that.

[deivid-rodriguez]

By the way, I'm now seeing #6254 that seems the best way to go in terms of making this error message more clear. 👍. But as I said, things seem to just hang on master unless I apply this patch...

[hsbt]

I was very surprised that you still run specs against 1.8.7

You can replace binread to File.open('/path/to/file', 'rb'){|f| f.read }

[deivid-rodriguez]

Thanks for the tip, @hsbt! I found a read_file method in bundler.rb that does exactly what you suggest so I used it and now all specs pass!

[indirect]

@deivid-rodriguez good catch, thanks!

@bundlerbot r+

[bundlerbot]

📌 Commit be337de has been approved by indirect

[bundlerbot]

⌛ Testing commit be337de with merge 83c23ec...

[bundlerbot]

☀️ Test successful - status-travis
Approved by: indirect
Pushing 83c23ec to master...

[deivid-rodriguez]

Thanks for the quick review!

[segiddins]

This is awesome, thanks!