Skip to content

Commit

Permalink
Auto merge of #1819 - olleolleolle:patch-1, r=segiddins
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
homu committed Jan 16, 2017
2 parents 35f57b6 + cffb000 commit 1c95b26
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 18 deletions.
2 changes: 1 addition & 1 deletion lib/rubygems/ext/rake_builder.rb
Expand Up @@ -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?
run cmd, results
end
Expand Down
51 changes: 34 additions & 17 deletions test/rubygems/test_gem_ext_rake_builder.rb
Expand Up @@ -14,14 +14,7 @@ def setup
end

def test_class_build
File.open File.join(@ext, 'mkrf_conf.rb'), 'w' do |mkrf_conf|
mkrf_conf.puts <<-EO_MKRF
File.open("Rakefile","w") do |f|
f.puts "task :default"
end
EO_MKRF
end

create_temp_mkrf_file('task :default')
output = []
realdir = nil # HACK /tmp vs. /private/tmp

Expand All @@ -39,15 +32,31 @@ def test_class_build
end
end

def test_class_build_fail
File.open File.join(@ext, 'mkrf_conf.rb'), 'w' do |mkrf_conf|
mkrf_conf.puts <<-EO_MKRF
File.open("Rakefile","w") do |f|
f.puts "task :default do abort 'fail' end"
end
EO_MKRF
# https://github.com/rubygems/rubygems/pull/1819
#
# It should not fail with a non-empty args list either
def test_class_build_with_args
create_temp_mkrf_file('task :default')
output = []
realdir = nil # HACK /tmp vs. /private/tmp

build_rake_in do |rake|
Dir.chdir @ext do
realdir = Dir.pwd
non_empty_args_list = ['']
Gem::Ext::RakeBuilder.build 'mkrf_conf.rb', nil, @dest_path, output, non_empty_args_list
end

output = output.join "\n"

refute_match %r%^rake failed:%, output
assert_match %r%^#{Regexp.escape @@ruby} mkrf_conf\.rb%, output
assert_match %r%^#{Regexp.escape rake} RUBYARCHDIR=#{Regexp.escape @dest_path} RUBYLIBDIR=#{Regexp.escape @dest_path}%, output
end
end

def test_class_build_fail
create_temp_mkrf_file("task :default do abort 'fail' end")
output = []

build_rake_in(false) do |rake|
Expand All @@ -60,6 +69,14 @@ def test_class_build_fail
assert_match %r%^rake failed%, error.message
end
end


def create_temp_mkrf_file(rakefile_content)
File.open File.join(@ext, 'mkrf_conf.rb'), 'w' do |mkrf_conf|
mkrf_conf.puts <<-EO_MKRF
File.open("Rakefile","w") do |f|
f.puts "#{rakefile_content}"
end
EO_MKRF
end
end
end

0 comments on commit 1c95b26

Please sign in to comment.