Respect encodings when reading gemspecs #6599

deivid-rodriguez · 2018-06-24T19:50:34Z

This PR fixes #6598. Thanks @eregon for the help and the explanation that helped me understand the issue :)!

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

On gems using UTF-8 symbols defined in other files in their gemspecs, bundle install would crash.

What was your diagnosis of the problem?

The problem was that since #6279 gemspecs are read binarily. However, files required from them as read as text. That means that the constants defined on those files and used in the gemspec are interpreted different and thus don't match.

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

My fix is to go back to reading gemspec as text again. Explictly passing the encoding when reading them still fixes the problem that the PR introducing the regression was meant to fix.

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

I chose this fix because it fixes the problem, and keeps the situation that the PR introducing the regression fixed also working.

deivid-rodriguez · 2018-06-24T19:58:11Z

spec/install/gemspecs_spec.rb

+        gem.author = "Ivo Anjo"
+        gem.summary = "Unscratchable stuff"
+      end
+    G


Looking at how nice the version.rb file in this diff looks, we might want to change the G tag to RUBY everywhere :)

eregon

Looks great to me!

eregon · 2018-06-24T20:22:05Z

lib/bundler.rb

@@ -444,7 +444,7 @@ def load_gemspec(file, validate = false)

    def load_gemspec_uncached(file, validate = false)
      path = Pathname.new(file)
-      contents = read_file(file)
+      contents = path.read(encoding: Encoding::UTF_8)


From the CI:

lib/bundler.rb:447:28: C: Style/HashSyntax: Use hash rockets syntax.

Thanks, should be fixed now!

eregon · 2018-06-24T21:15:49Z

lib/bundler.rb

@@ -444,7 +444,7 @@ def load_gemspec(file, validate = false)

    def load_gemspec_uncached(file, validate = false)
      path = Pathname.new(file)
-      contents = read_file(file)
+      contents = path.read(:encoding => Encoding::UTF_8)


It's surprising Bundler still supports the very old Ruby 1.8.7.
Maybe File.open(file, "r:UTF-8", &:read) would work on 1.8.7 too.

Totally, I was bitten by the same thing in the PR that caused this regression as well.

Master (bundle 2) does not really support 1.8.7 anymore (the minimum required ruby version has been bumped to ruby 2.3). However, specs are still run against 1.8.7 in order to "ease backports". In my opinion this is wrong because easing backports comes at the cost of difficulting regular contributions (like this one). But it's no big deal I guess.

Just tried your suggestion, let's see if it works...

Thanks for the info, I agree in general. I filed #6600 just before seeing this, let's see.

It didn't work... 😞 Up to the maintainers how to follow up from here.

eregon · 2018-06-24T22:06:29Z

Maybe it's OK to skip this spec on 1.8.7?
IIRC, Ruby 1.8.7 has very limited support for Unicode characters, notably I think it doesn't quite like them in constants.
The rest seems to work fine, only this spec fails.

deivid-rodriguez · 2018-06-24T22:38:18Z

Fine with me! Let me skip it.

colby-swandale · 2018-06-25T06:37:47Z

spec/install/gemspecs_spec.rb

@@ -63,6 +63,33 @@
    expect(out).to include("Bundle complete!")
  end

+  it "reads gemspecs respecting their encoding" do
+    skip "Too cool for 1.8" if RUBY_VERSION < "1.9"


This is not informative, please use a comment that explains why we need to skip 1.8 in this test.

What about "unicode was probably not fully supported on 1.8"?

Basically the reason is that we have decided it's not worth even trying to make this work on 1.8. We're not sure it's actually supported and blind guessing from Travis logs feedback is... tough.

Pushed a new skip message, hopefully more serious and informative :)

colby-swandale

I'm happy to skip testing this spec on ruby 1.8.6

colby-swandale · 2018-06-26T10:28:55Z

@bundlerbot r+

bundlerbot · 2018-06-26T10:28:56Z

📌 Commit 6df5585 has been approved by colby-swandale

bundlerbot · 2018-06-26T10:29:05Z

⌛ Testing commit 6df5585 with merge cb18acc...

@eregon

…n, r=colby-swandale Respect encodings when reading gemspecs This PR fixes #6598. Thanks @eregon for the help and the explanation that helped me understand the issue :)! ### What was the end-user problem that led to this PR? On gems using UTF-8 symbols defined in other files in their gemspecs, `bundle install` would crash. ### What was your diagnosis of the problem? The problem was that since #6279 gemspecs are read binarily. However, files required from them as read as text. That means that the constants defined on those files and used in the gemspec are interpreted different and thus don't match. ### What is your fix for the problem, implemented in this PR? My fix is to go back to reading gemspec as text again. Explictly passing the encoding when reading them still fixes the problem that the PR introducing the regression was meant to fix. ### Why did you choose this fix out of the possible options? I chose this fix because it fixes the problem, and keeps the situation that the PR introducing the regression fixed also working.

bundlerbot · 2018-06-26T10:54:28Z

☀️ Test successful - status-travis
Approved by: colby-swandale
Pushing cb18acc to master...

@eregon

…n, r=colby-swandale Respect encodings when reading gemspecs This PR fixes #6598. Thanks @eregon for the help and the explanation that helped me understand the issue :)! ### What was the end-user problem that led to this PR? On gems using UTF-8 symbols defined in other files in their gemspecs, `bundle install` would crash. ### What was your diagnosis of the problem? The problem was that since #6279 gemspecs are read binarily. However, files required from them as read as text. That means that the constants defined on those files and used in the gemspec are interpreted different and thus don't match. ### What is your fix for the problem, implemented in this PR? My fix is to go back to reading gemspec as text again. Explictly passing the encoding when reading them still fixes the problem that the PR introducing the regression was meant to fix. ### Why did you choose this fix out of the possible options? I chose this fix because it fixes the problem, and keeps the situation that the PR introducing the regression fixed also working. (cherry picked from commit cb18acc)

deivid-rodriguez commented Jun 24, 2018

View reviewed changes

eregon reviewed Jun 24, 2018

View reviewed changes

Respect encodings when reading gemspecs

74a0058

deivid-rodriguez force-pushed the fix/gemspec_encoding_regression branch from 1e76cd1 to 74a0058 Compare June 24, 2018 20:32

eregon reviewed Jun 24, 2018

View reviewed changes

Try fix 1.8.7 build

d3e7940

Skip encoding spec on 1.8.7

4a59d70

colby-swandale reviewed Jun 25, 2018

View reviewed changes

colby-swandale approved these changes Jun 25, 2018

View reviewed changes

More informative skip message

6df5585

colby-swandale added this to the 1.16.3 milestone Jun 26, 2018

bundlerbot merged commit 6df5585 into rubygems:master Jun 26, 2018

deivid-rodriguez deleted the fix/gemspec_encoding_regression branch June 27, 2018 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect encodings when reading gemspecs #6599

Respect encodings when reading gemspecs #6599

deivid-rodriguez commented Jun 24, 2018

deivid-rodriguez Jun 24, 2018

eregon left a comment

eregon Jun 24, 2018

deivid-rodriguez Jun 24, 2018

eregon Jun 24, 2018

deivid-rodriguez Jun 24, 2018

eregon Jun 24, 2018

deivid-rodriguez Jun 24, 2018

eregon commented Jun 24, 2018

deivid-rodriguez commented Jun 24, 2018

colby-swandale Jun 25, 2018

deivid-rodriguez Jun 25, 2018

deivid-rodriguez Jun 25, 2018

deivid-rodriguez Jun 25, 2018

colby-swandale left a comment

colby-swandale commented Jun 26, 2018

bundlerbot commented Jun 26, 2018

bundlerbot commented Jun 26, 2018

bundlerbot commented Jun 26, 2018

Respect encodings when reading gemspecs #6599

Respect encodings when reading gemspecs #6599

Conversation

deivid-rodriguez commented Jun 24, 2018

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

What was your diagnosis of the problem?

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

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

Choose a reason for hiding this comment

eregon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon commented Jun 24, 2018

deivid-rodriguez commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colby-swandale left a comment

Choose a reason for hiding this comment

colby-swandale commented Jun 26, 2018

bundlerbot commented Jun 26, 2018

bundlerbot commented Jun 26, 2018

bundlerbot commented Jun 26, 2018