Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip binaries #40

Closed
wants to merge 1 commit into from
Closed

Strip binaries #40

wants to merge 1 commit into from

Conversation

fauno
Copy link

@fauno fauno commented Sep 14, 2019

Stripped binaries are removed of unused objects and become lighter in
resource usage and have better performance.

Stripped binaries are removed of unused objects and become lighter in
resource usage and have better performance.
@fauno
Copy link
Author

fauno commented Sep 14, 2019

travis is failing with invalid option: --no-ri

I wanted to remove the ext/ dir to make the gems smaller, as far as I see with the --prune option the removed files are removed from the gemspec but then the gem I tested it with failed because it wanted ext/mri/extconf.rb... I don't know the internals of rubygems enough to find where it's required :)

Also, to take advantage of ccache I think the full path needs to remain the same between builds, would you be open to a patch about that?

@luislavena
Copy link
Owner

Hello @fauno, thanks for your contribution.

I didn't had time yet to review fully the impact of this change, but two points I would like to bring before anything:

  1. The changes requires to be portable across Ruby and compilers. There might not be strip or might be called differently in other platforms. This information is exposed by RbConfig::CONFIG and should be used instead of hardcoding. This also includes the file extensions. Ironically, Ruby's extensions have .so, even on Windows.
  2. Shelling out using backticks might not scale correctly. What would happen when things fail? How the user should be informed? gem-compiler relies on RubyGems' own building process, so perhaps we can rely on them. If not, perhaps the issue could be solved directly by extconf.rb itself and not at gem-compiler.

I'm leaving these notes here and let me know if you have explored those. Last but not least, I'm interesting to better understand your motivation of this change, if a concrete scenario or need raised it.

Thank you in advance and looking forward your feedback.

Cheers.

@fauno
Copy link
Author

fauno commented Sep 16, 2019 via email

@luislavena
Copy link
Owner

luislavena commented Sep 17, 2019

i can make it check for *nix systems instead of file extension. what compiler does ruby use on windows?

RubyInstaller uses MinGW (gcc) and there is also Visual Studio (MSVC). I wouldn't recommend excluding things by that method.

I took a quick look and Ruby already collects if strip is available using RbConfig::CONFIG["STRIP"]

In relation to process execution and capture, we should be following RubyGems' own approach, similar to the following, using open3:

https://github.com/rubygems/rubygems/blob/master/lib/rubygems/ext/builder.rb#L62-L81

I will not have time to tackle yet (probably in a few weeks) but feel free to update this PR and we will sync back soon.

Once again, thank you for your contribution and the details on the motivation, indeed makes a really useful case and looking forward integrate such changes to improve the usability of this tool.

Cheers.

@fauno
Copy link
Author

fauno commented Sep 30, 2019 via email

@luislavena
Copy link
Owner

hi, i didn't forget about this but i've been doing other things.

Don't worry, not going anywhere 😄

Appreciate the time you take to collaborate and investigate how we can reach the best option.

do you think it would be ok to detect if strip is present via RbConfig but still use strip --strip-unneeded via popen3?

I think we can work with that. Will be tricky to test striping since the test fake an extension, so doesn't produce a valid shared object file.

I will try at some point to workout some integration tests that generates a real C extension, but I think is OK for now.

One last nitpick: can you sort strip_binaries alphabetically? 😅

Thank you for your time and patience with my feedback.

And thank you for your contribution!

luislavena added a commit that referenced this pull request Dec 31, 2019
Introduce `--strip` to perform stripping on compiled extensions (using
same command as defined by `RbConfig::CONFIG["STRIP"]`).

Or using a custom stripping command using `--strip-cmd`, allowing
different executable and flags to be used.

This is not enabled by default.

Closes #40.
@luislavena
Copy link
Owner

Hello @fauno, thanks for your patience.

I had some time during the holidays to look at this and submitted an alternate version that I believe gives you enough flexibility. Please take a look to #48.

This will be part of upcoming release.

Cheers!

@fauno
Copy link
Author

fauno commented Jan 2, 2020

Hi! I was just thinking of this and found out the notification email! Great! Thanks!

We've been using gem-compiler in production for a few months now and everything's great. Here are some examples for creating a binary gem repository for Alpine (ruby < 2.7 requires a patch on rubygems to correctly detect the linux-musl target):

@luislavena
Copy link
Owner

This is excellent @fauno!!! Really happy to see this project being put to a good use (outside of me, 😁).

Just wanted to let you know that simplified symbol stripping UI/UX and reduced to just --strip, with optional custom command. I've updated the README in master to reflect the syntax and this will be part of the upcoming release.

Only need to finish some include/exclude pattern globing changes and will release the new version, probably this weekend.

Cheers
❤️ ❤️ ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants