RakeBuilder: avoid frozen string issue #1819

olleolleolle · 2017-01-16T12:57:58Z

Description:

This PR adds a missing #dup ("convert frozen string literal to a mutable String object instance") in the same way that 45966be did.

The problem only appears when there're args to .build. To trigger this behavior, I duplicated a test and added a variable with a non-empty array of an empty string (to have benign test data).

What it looks like without the fix

Without this change, the user's failure will look like this:

Gem::Ext::BuildError: ERROR: Failed to build gem native extension.
can't modify frozen String, created at
/home/travis/.rvm/rubies/jruby-9.1.7.0/lib/ruby/stdlib/rubygems/ext/rake_builder.rb:11

Reproduce

To reproduce the issue, write a mkrf_conf.rb which calls build. Here is an example ext/mkrf_conf.rb with a failing build: https://github.com/sickill/rainbow/blob/master/ext/mkrf_conf.rb

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.

- See 45966be which had this fix most everywhere else

olleolleolle · 2017-01-16T13:22:49Z

The failure to build was one 1.8.7 with the text:

rake aborted!
Installing gems 'bundler:1.12.0' 'builder:2.1.2' 'artifice:~> 0.6.0' 'sinatra:~> 1.4.7' 'rake:10.0.2' 'compact_index:~> 0.11.0' 'rack:< 2' for the tests to use failed!
./spec/support/rubygems_ext.rb:56:in `install_gems'
./spec/support/rubygems_ext.rb:40:in `setup'
/home/travis/build/rubygems/rubygems/bundler/Rakefile:58
/home/travis/build/rubygems/rubygems/bundler/Rakefile:29:in `invoke'
/home/travis/build/rubygems/rubygems/bundler/Rakefile:28:in `invoke'
/home/travis/build/rubygems/rubygems/bundler/Rakefile:80
/home/travis/build/rubygems/rubygems/bundler/Rakefile:29:in `invoke'
/home/travis/build/rubygems/rubygems/bundler/Rakefile:28:in `invoke'
/home/travis/.rvm/gems/ruby-1.8.7-p371/bin/ruby_executable_hooks:15
Tasks: TOP => spec:deps
(See full trace by running task with --trace)

teoljungberg · 2017-01-16T13:23:19Z

test/rubygems/test_gem_ext_rake_builder.rb

+  #
+  # It should not fail with a non-empty args list either
+  def test_class_build_with_args
+    File.open File.join(@ext, 'mkrf_conf.rb'), 'w' do |mkrf_conf|


There's a fair amount of duplication between this test-case and the one above. A meaningful one would be to extract would be this file creation. Especially since the two files are identical.

olleolleolle · 2017-01-16T14:05:20Z

@teoljungberg There! It worked.

teoljungberg · 2017-01-16T14:05:59Z

test/rubygems/test_gem_ext_rake_builder.rb

@@ -12,16 +12,9 @@ def setup
    FileUtils.mkdir_p @ext
    FileUtils.mkdir_p @dest_path
  end
-
+  


This should probably not be included.

Thanks for careful reading, T.

- this avoids the reek warning Prima Donna Method; avoids confusing the reader

segiddins · 2017-01-16T16:02:51Z

Good catch!
@homu r+

homu · 2017-01-16T16:02:52Z

📌 Commit cffb000 has been approved by segiddins

homu · 2017-01-16T16:03:09Z

⌛ Testing commit cffb000 with merge e0dd862...

RakeBuilder: avoid frozen string issue # Description: This PR adds a missing `#dup` ("convert frozen string literal to a mutable String object instance") in the same way that 45966be did. The problem only appears when there're `args` to `.build`. To trigger this behavior, I duplicated a test and added a variable with a non-empty array of an empty string (to have benign test data). **What it looks like without the fix** Without this change, the user's failure will look like this: ``` Gem::Ext::BuildError: ERROR: Failed to build gem native extension. can't modify frozen String, created at /home/travis/.rvm/rubies/jruby-9.1.7.0/lib/ruby/stdlib/rubygems/ext/rake_builder.rb:11 ``` **Reproduce** To reproduce the issue, write a `mkrf_conf.rb` which calls `build`. Here is an example `ext/mkrf_conf.rb` with a failing build: https://github.com/sickill/rainbow/blob/master/ext/mkrf_conf.rb # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [x] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).

homu · 2017-01-16T16:44:03Z

💔 Test failed - status

olleolleolle · 2017-01-16T18:26:24Z

@segiddins The 1.8.7 picks wrong deps now and then. Don't know why. It works half the time.

hmistry · 2017-01-16T19:01:17Z

lib/rubygems/ext/rake_builder.rb

@@ -9,7 +9,7 @@ class Gem::Ext::RakeBuilder < Gem::Ext::Builder

  def self.build(extension, directory, dest_path, results, args=[], lib_dir=nil)
    if File.basename(extension) =~ /mkrf_conf/i then
-      cmd = "#{Gem.ruby} #{File.basename extension}"
+      cmd = "#{Gem.ruby} #{File.basename extension}".dup
      cmd << " #{args.join " "}" unless args.empty?


Rewriting " #{args.join " "}" as " #{args.join(" ")}" is clearer to reader there's a string param passed into join and it's not " #{args.join " + "}". Maybe use single quotes to be more obvious.

@hmistry These are good ideas. Would you accept a PR from me about it?

I don't have authority to accept PR's, sorry. Ask @segiddins or other RG core team members. I'm sure they won't mind since it's a low risk fix.

segiddins · 2017-01-16T20:17:42Z

@homu retry

RakeBuilder: avoid frozen string issue # Description: This PR adds a missing `#dup` ("convert frozen string literal to a mutable String object instance") in the same way that 45966be did. The problem only appears when there're `args` to `.build`. To trigger this behavior, I duplicated a test and added a variable with a non-empty array of an empty string (to have benign test data). **What it looks like without the fix** Without this change, the user's failure will look like this: ``` Gem::Ext::BuildError: ERROR: Failed to build gem native extension. can't modify frozen String, created at /home/travis/.rvm/rubies/jruby-9.1.7.0/lib/ruby/stdlib/rubygems/ext/rake_builder.rb:11 ``` **Reproduce** To reproduce the issue, write a `mkrf_conf.rb` which calls `build`. Here is an example `ext/mkrf_conf.rb` with a failing build: https://github.com/sickill/rainbow/blob/master/ext/mkrf_conf.rb # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [x] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).

homu · 2017-01-16T20:17:59Z

⌛ Testing commit cffb000 with merge 1c95b26...

homu · 2017-01-16T21:03:11Z

☀️ Test successful - status

RakeBuilder: avoid frozen string issue # Description: This PR adds a missing `#dup` ("convert frozen string literal to a mutable String object instance") in the same way that 45966be did. The problem only appears when there're `args` to `.build`. To trigger this behavior, I duplicated a test and added a variable with a non-empty array of an empty string (to have benign test data). **What it looks like without the fix** Without this change, the user's failure will look like this: ``` Gem::Ext::BuildError: ERROR: Failed to build gem native extension. can't modify frozen String, created at /home/travis/.rvm/rubies/jruby-9.1.7.0/lib/ruby/stdlib/rubygems/ext/rake_builder.rb:11 ``` **Reproduce** To reproduce the issue, write a `mkrf_conf.rb` which calls `build`. Here is an example `ext/mkrf_conf.rb` with a failing build: https://github.com/sickill/rainbow/blob/master/ext/mkrf_conf.rb # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [x] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). (cherry picked from commit 1c95b26)

By enabled Danger, we enable the installation of Rubocop on the CI environment. Rubocop depends on the "rainbow" library which has a build error on certain versions of RubyGems. This has been fixed in newer versions of RubyGems, so let's just update RubyGems prior to trying to install anything. See: ku1ik/rainbow#44 See: rubygems/rubygems#1819

Related to recent build failures and rubygems/rubygems#1819

RakeBuilder: avoid frozen string issue

4c3b832

- See 45966be which had this fix most everywhere else

bronzdoc added the status: triage label Jan 16, 2017

RakeBuilder: test with non-empty args list given

579e2ef

teoljungberg reviewed Jan 16, 2017

View reviewed changes

RakeBuilder test: Extract fixture file creation

5254f79

teoljungberg reviewed Jan 16, 2017

View reviewed changes

RakeBuilder test: rename method

cffb000

- this avoids the reek warning Prima Donna Method; avoids confusing the reader

olleolleolle mentioned this pull request Jan 16, 2017

Install issues with 2.2.1 ku1ik/rainbow#44

Closed

hmistry suggested changes Jan 16, 2017

View reviewed changes

homu merged commit cffb000 into rubygems:master Jan 16, 2017

bronzdoc removed the status: triage label Jan 16, 2017

olleolleolle deleted the patch-1 branch January 17, 2017 07:35

This was referenced Jan 23, 2017

Revert pinning of Bundler to 1.13.7 shoes/shoes4#1372

Closed

Arc compatibility with Shoes 3 shoes/shoes4#1364

Merged

This was referenced Jan 23, 2017

1.14.2 bundle install can't install rainbow: "can't modify frozen String" rubygems/bundler#5351

Closed

Gem::Ext::BuildError "can't modify frozen String" under Ruby v2.3.3 and bundler v1.14.2 ku1ik/rainbow#49

Closed

dentarg added a commit to dentarg/dyno_metadata that referenced this pull request Feb 7, 2017

Install latest RubyGems on Travis

864c254

Related to recent build failures and rubygems/rubygems#1819

agrberg mentioned this pull request Feb 17, 2017

Plivo having trouble building on Travis CI plivo/plivo-ruby#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RakeBuilder: avoid frozen string issue #1819

RakeBuilder: avoid frozen string issue #1819

olleolleolle commented Jan 16, 2017 •

edited

Loading

olleolleolle commented Jan 16, 2017

teoljungberg Jan 16, 2017

olleolleolle commented Jan 16, 2017

teoljungberg Jan 16, 2017

olleolleolle Jan 16, 2017

segiddins commented Jan 16, 2017

homu commented Jan 16, 2017

homu commented Jan 16, 2017

homu commented Jan 16, 2017

olleolleolle commented Jan 16, 2017

hmistry Jan 16, 2017

olleolleolle Jan 17, 2017

hmistry Jan 17, 2017

segiddins commented Jan 16, 2017

homu commented Jan 16, 2017

homu commented Jan 16, 2017

RakeBuilder: avoid frozen string issue #1819

RakeBuilder: avoid frozen string issue #1819

Conversation

olleolleolle commented Jan 16, 2017 • edited Loading

Description:

Tasks:

olleolleolle commented Jan 16, 2017

teoljungberg Jan 16, 2017

Choose a reason for hiding this comment

olleolleolle commented Jan 16, 2017

teoljungberg Jan 16, 2017

Choose a reason for hiding this comment

olleolleolle Jan 16, 2017

Choose a reason for hiding this comment

segiddins commented Jan 16, 2017

homu commented Jan 16, 2017

homu commented Jan 16, 2017

homu commented Jan 16, 2017

olleolleolle commented Jan 16, 2017

hmistry Jan 16, 2017

Choose a reason for hiding this comment

olleolleolle Jan 17, 2017

Choose a reason for hiding this comment

hmistry Jan 17, 2017

Choose a reason for hiding this comment

segiddins commented Jan 16, 2017

homu commented Jan 16, 2017

homu commented Jan 16, 2017

olleolleolle commented Jan 16, 2017 •

edited

Loading