Bundler 1.16.2 regression: gemspec referencing code with non-ascii characters requires "encoding: UTF-8" magic comment

[ivoanjo]

Hello there and thanks for Bundler!

I'm the author of the Persistent-💎 gem, which uses as its namespace a module Persistent💎.

The gemspec for the gem thus includes:

Gem::Specification.new do |spec|
  spec.name          = 'persistent-dmnd'
  spec.version       = Persistent💎::VERSION

Because I support this gem also on Ruby 1.9.3, I've added a # encoding: UTF-8 at the top of every file so Ruby correctly handles the emoji.

But, while debugging a related TruffleRuby issue (see oracle/truffleruby#1376) we discovered that whereas with Bundler 1.16.1 the gem's gemspec file works correctly on Ruby 2.0->2.6 without a # encoding: UTF-8 comment at the top of the file:

$ ruby -v
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]
$ bundle -v
Bundler version 1.16.1
$ git diff
diff --git a/persistent-dmnd.gemspec b/persistent-dmnd.gemspec
index 72c50a6..e4b3dad 100644
--- a/persistent-dmnd.gemspec
+++ b/persistent-dmnd.gemspec
@@ -1,4 +1,3 @@
-# encoding: UTF-8
 
 # Persistent-💎: Ruby gem for easily creating immutable data structures
 # Copyright (c) 2017 Ivo Anjo <[email protected]>
$ bundle install
Warning: the running version of Bundler (1.16.1) is older than the version that created the lockfile (1.16.2). We suggest you upgrade to the latest version of Bundler by running `gem install bundler`.
[snip]
Bundle complete! 9 Gemfile dependencies, 18 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.

...on bundler 1.16.2 this breaks:

$ bundle -v
Bundler version 1.16.2
$ bundle install
Traceback (most recent call last):
        24: .rvm/gems/ruby-2.5.1/bin/ruby_executable_hooks:15:in `<main>'
        23: .rvm/gems/ruby-2.5.1/bin/ruby_executable_hooks:15:in `eval'
        22: .rvm/gems/ruby-2.5.1/bin/bundle:23:in `<main>'
        21: .rvm/gems/ruby-2.5.1/bin/bundle:23:in `load'
        20: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/exe/bundle:22:in `<top (required)>'
        19: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/friendly_errors.rb:124:in `with_friendly_errors'
        18: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/exe/bundle:30:in `block in <top (required)>'
        17: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/cli.rb:18:in `start'
        16: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
        15: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/cli.rb:27:in `dispatch'
        14: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
        13: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
        12: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
        11: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/cli.rb:223:in `install'
        10: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/settings.rb:136:in `temporary'
         9: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/cli.rb:224:in `block in install'
         8: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/cli/install.rb:62:in `run'
         7: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler.rb:135:in `definition'
         6: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/definition.rb:35:in `build'
         5: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/dsl.rb:12:in `evaluate'
         4: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/dsl.rb:42:in `eval_gemfile'
         3: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/dsl.rb:51:in `rescue in eval_gemfile'
         2: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/dsl.rb:51:in `message'
         1: .rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/dsl.rb:51:in `to_s'
.rvm/gems/ruby-2.5.1/gems/bundler-1.16.2/lib/bundler/dsl.rb:575:in `to_s': incompatible character encodings: UTF-8 and ASCII-8BIT (Encoding::CompatibilityError)

[deivid-rodriguez]

Hello @ivoanjo! Maybe #6279 is the culprit... Can you confirm?

[ivoanjo]

Thanks for the reply @deivid-rodriguez . I can confirm that reverting the call to read_file(file) back to path.read in load_gemspec_uncached fixed it, so the regression defnitely came from there.

[deivid-rodriguez]

I think reverting the fix could be a good solution since the error message is more clear now. However, the spec I added to prove my fix hangs if I revert it, so I think we need to investigate why that happens before considering that.

[deivid-rodriguez]

Ok, So I had a look at this.

Reverting #6279, and adding a rescue to the exception formatting method seems to stop the hang on bundler's master and give a meaninful error message instead, and keeps things working as they are now on 1-16 stable (it just works).

However, I have not been able to reproduce your issue (incompatible character encodings: UTF-8 and ASCII-8BIT (Encoding::CompatibilityError)) unless I explicitly introduce a syntax error on a file with a UTF-8 name. 🤔

This is the diff I'm working with (the new spec added passes on both branches and fails without the changes in lib, the old one passes on master but fails on 1.16-stable because the old behavior is kept on this branch).

Not sure why this difference between master and 1.16-stable, whether it is acceptable, and whether the rest of the specs pass with these changes (running them now).

diff --git a/lib/bundler.rb b/lib/bundler.rb
index b7002db99..010b26f3b 100644
--- a/lib/bundler.rb
+++ b/lib/bundler.rb
@@ -427,7 +427,7 @@ EOF
 
     def load_gemspec_uncached(file, validate = false)
       path = Pathname.new(file)
-      contents = read_file(file)
+      contents = path.read
       spec = if contents.start_with?("---") # YAML header
         eval_yaml_gemspec(path, contents)
       else
diff --git a/lib/bundler/dsl.rb b/lib/bundler/dsl.rb
index 868116327..d76ffbda6 100644
--- a/lib/bundler/dsl.rb
+++ b/lib/bundler/dsl.rb
@@ -588,6 +588,8 @@ The :#{name} git source is deprecated, and will be removed in Bundler 2.0.#{addi
           description = description.sub(/#{Regexp.quote trace_line}:\s*/, "").sub("\n", " - ")
         end
         [trace_line, description]
+      rescue
+        [nil, description]
       end
     end
 
diff --git a/spec/install/gemspecs_spec.rb b/spec/install/gemspecs_spec.rb
index 5b6778bdd..a435635a8 100644
--- a/spec/install/gemspecs_spec.rb
+++ b/spec/install/gemspecs_spec.rb
@@ -46,7 +46,7 @@ RSpec.describe "bundle install" do
     expect(the_bundle).to include_gems "activesupport 2.3.2"
   end
 
-  it "does not hang when gemspec has incompatible encoding" do
+  it "shows a friendly error when gemspec has incompatible encoding" do
     create_file "foo.gemspec", <<-G
       Gem::Specification.new do |gem|
         gem.name = "pry-byebug"
@@ -60,7 +60,38 @@ RSpec.describe "bundle install" do
       gemspec
     G
 
-    expect(out).to include("Bundle complete!")
+    expect(out).to include("[!] There was an error while loading `foo.gemspec`: invalid multibyte char (US-ASCII)")
+    expect(out).to match(/ #  from .*\/foo.gemspec:4/)
+    expect(out).to include(" #  -------------------------------------------")
+    expect(out).to include(' #    gem.version = "3.4.2"')
+    expect(out).to include(' >    gem.author = "David Rodríguez"')
+    expect(out).to include(' #    gem.summary = "Good stuff"')
+    expect(out).to include(" #  -------------------------------------------")
+  end
+
+  if RUBY_VERSION >= "2.0"
+    it "shows a friendly error when syntax errors happen on files with UTF-8 names" do
+      create_file("💎.rb", <<-RUBY)
+        VERSION = "0.0.1
+      RUBY
+
+      create_file("persistent-dmnd.gemspec", <<-G)
+        require_relative "💎"
+
+        Gem::Specification.new do |gem|
+          gem.name = "persistent-dmnd"
+          gem.version = VERSION
+          gem.author = "Ivo Anjo"
+          gem.summary = "Unscratchable stuff"
+        end
+      G
+
+      install_gemfile <<-G
+        gemspec
+      G
+
+      expect(out).to include("unterminated string meets end of file")
+    end
   end
 
   context "when ruby version is specified in gemspec and gemfile" do

[deivid-rodriguez]

The changes don't seem to break any other specs. Any ideas on the different behavior between master and 1.16-stable?

[ivoanjo]

Thanks you @deivid-rodriguez for digging down so fast into the issue!
Hopefully a fix is found that solves both issues.

As for reproducing it, I can reliably reproduce by cloning the repository and changing the gemspec. If it helps, I'm using Ubuntu 17.10 and rvm to set up Ruby.

[deivid-rodriguez]

Yeah, me too. I meant that I haven't been able to reproduce with a spec.

[eregon]

Note that it's a bit tricky to reproduce the issue with the Persistent💎 gemspec above.
One way is to have an external file (say version.rb) loaded by require like this:

module Persistent💎
  VERSION = '1.0.2'
end

and then the gemspec reads from that constant with Persistent💎::VERSION:

require_relative 'version.rb'

Gem::Specification.new do |spec|
  spec.name          = 'persistent-dmnd'
  spec.version       = Persistent💎::VERSION
  spec.authors       = 'Ivo Anjo'
  spec.email         = '[email protected]'
end

The required file has implicitly an encoding of UTF-8, while the gemspec got a binary encoding when using read_file, and so both Persistent💎 end up referring to different things in that context, leading to an error.

Note that path.read likely depends on the locale encoding, which seems unreliable.
Probably path.read(encoding: Encoding::UTF_8) is better and then the result does no longer depend on the current locale.
That means Bundler would arbitrarily set the source encoding of gemspec to UTF-8, if there is no magic encoding comment, but that seems acceptable and more reliable than the locale encoding.

Finally, it's worth noting the original gemspec should have the magic encoding comment, to work on older Bundler and with a locale != UTF-8 (I guess it would fail without the magic encoding comment in that situation).

[deivid-rodriguez]

Yeah, I did try exactly that, and I was getting that the constant Persistent💎 was undefined. That seems consistent with your explanation, right?

[eregon]

Yes, that's expected and shows the regression.
The Encoding::CompatibilityError above is just an error while trying to format the missing constant error message nicely IIRC.

[deivid-rodriguez]

And explicitly passing the encoding to path.read fixed the spec on master too! Thanks for that tip! :) I'll open a PR shortly.

[eregon]

Thank you for the quick fix @deivid-rodriguez!

[deivid-rodriguez]

No problem, it was my regression after all 😅

[ivoanjo]

+1 Thanks for the fix @deivid-rodriguez and hooray for much improved unicode support after your two PRs!