Skip to content
This repository has been archived by the owner on Mar 2, 2021. It is now read-only.

Add wrappers for strip commands so that files are copied to the local drive before stripped. #4

Merged
merged 2 commits into from
Feb 20, 2014

Conversation

larskanis
Copy link
Contributor

Stripping files on a local folder instead of a Virtualbox shared folder
is needed to work around this bug: https://www.virtualbox.org/ticket/8463

Without that wrapper strip fails with something like this:

vagrant@precise64:/vagrant/nokogiri$ i686-w64-mingw32-strip -S tmp/x86-mingw32/stage/lib/nokogiri/2.0/nokogiri.so
i686-w64-mingw32-strip:tmp/x86-mingw32/stage/lib/nokogiri/2.0/stBxc8W1: Protocol error

@larskanis
Copy link
Contributor Author

@tjschuck, @luislavena: Did you notice this issue, too, when using strip? Or is there something wrong with my rake-compiler-dev-box-setup?

@tjschuck
Copy link
Owner

@larskanis I tried to look at this last week, but ran into #5, which kept it from working at all for me, with or without your changes. Still haven't had the time to fix that one yet :(

@luislavena
Copy link
Contributor

@larskanis Interesting that I didn't hit this issue.

Will take a look the upcoming days, but can't promise anything until the first week of november 😢

local drive before stripped.

Stripping files on a local folder instead of a Virtualbox shared folder
is needed to work around this bug: https://www.virtualbox.org/ticket/8463
@larskanis
Copy link
Contributor Author

@tjschuck I did a rebase to current master.

I get the 'Protocol error' for any strip call with an unstripped dll. Already stripped dlls don't produce this error.

I'm not sure, if the 'Protocol error' is dependent on the host environment. I'm using VirtualBox packaged to and running on Ubuntu-13.10, but got this error on older Ubuntu-releases, too. Would be great, if someone can verify this issue and the attached workaround. @simi can you?

@simi
Copy link

simi commented Nov 17, 2013

@larskanis I had this issue also. But I did some steps (manually edited config yml for rake-compiler maybe ???) to avoid it. Anyway I'll test this one more time and I'll send ping here with 👍 or 👎.

@tjschuck
Copy link
Owner

tjschuck commented Dec 4, 2013

@larskanis Which gems cause this error for you when compiling? I haven't run into it, so I'd like to try to reproduce!

@simi
Copy link

simi commented Dec 4, 2013

@tjschuck FFI for example.

@tjschuck
Copy link
Owner

tjschuck commented Dec 4, 2013

Just a note-to-self that the version bumping in ffi/ffi#298 is also necessary, or else nothing will work.

@larskanis
Copy link
Contributor Author

@tjschuck Does that mean you can reproduce the issue with strip when building ffi?

@tjschuck
Copy link
Owner

tjschuck commented Dec 5, 2013

@larskanis Yep, ran into the same "Protocol error" building ffi.

@luislavena
Copy link
Contributor

@larskanis does this happen with the testing project?

https://github.com/luislavena/test-ruby-c-extension

I want to exclude possible issues caused by libffi compilation itself.

@larskanis
Copy link
Contributor Author

test-ruby-c-extension does not use strip in any way, so it is not affected by this issue. libffi equally does not use strip, but the Rakefile of the ffi gem. Also zlib makes use of strip in it's Makefile - this is why nokogiri is affected by this issue.

I don't know, what's so special in the way strip accesses it's files, but there was no other command that causes this "Protocol error", so far.

@luislavena
Copy link
Contributor

@larskanis If I remember correctly, the issue was that cross-compiled Rubies generated huge debug symbols, so extensions compiled against them resulted in being bloated.

I think perhaps a change on the way Ruby is cross-compiled would help to solve that.

I'm going to play a bit with this today.

@larskanis
Copy link
Contributor Author

@luislavena Yes, that would probably give the same result as stripping the .so files. On the other hand it will not help for third party libraries like zlib, that make use of strip in their Makefile.

@luislavena
Copy link
Contributor

@larskanis thank you for your fast response.

I'm playing catch-up with all the OSS I'm involved, so working today on review these changes and ruby ffi specially.

FYI: I'm on #rubyinstaller on IRC just in case

@luislavena
Copy link
Contributor

@tjschuck this addition works perfectly for me.

For testing, I've added the following to sample_gem

  ENV['RUBY_CC_VERSION'].to_s.split(':').each do |ruby_version|
    platforms = {
      "x86-mingw32" => "i686-w64-mingw32",
      "x64-mingw32" => "x86_64-w64-mingw32"
    }
    platforms.each do |platform, prefix|
      task "copy:sample_gem_ext:#{platform}:#{ruby_version}" do |t|
        %w[lib tmp/#{platform}/stage/lib].each do |dir|
          so_file = "#{dir}/#{ruby_version[/^\d+\.\d+/]}/sample_gem_ext.so"
          if File.exists?(so_file)
            sh "#{prefix}-strip -S #{so_file}"
          end
        end
      end
    end
  end

And it worked without issues.

Thank you @larskanis for sending this.

@tjschuck perhaps you can add us both as contributors so we can merge those changes? I have another pull request with other updates (adding Ruby 2.1 too)

Thank you.

@tjschuck
Copy link
Owner

Sorry for the delay, friends. "Work" and all that :)

Looking at this now, but likely merging shortly.

tjschuck added a commit that referenced this pull request Feb 20, 2014
Add wrappers for strip commands so that files are copied to the local drive before stripped.
@tjschuck tjschuck merged commit 9dd6906 into tjschuck:master Feb 20, 2014
tjschuck pushed a commit that referenced this pull request Feb 20, 2014
This relates to #4, as strip seems
to have issues working with VirtualBox shared folder mechanism.

Testing this first will help us implement the wrappers descrived
in the above ticket.
@simi
Copy link

simi commented Feb 20, 2014

❤️

@larskanis
Copy link
Contributor Author

Thank you all for taking the time to review and merge this!

@larskanis larskanis deleted the use-wrappers-for-strip branch February 24, 2014 08:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants