Clean built artifacts after building extensions #6133

deivid-rodriguez · 2022-12-15T23:10:23Z

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

Currently installing extensions leaves around intermediary files that may end up taking a lot of disk space.

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

Add a clean step after installing the extension.

This PR is based on the useful work by @eloyesp at #4017.

Fixes #3958.

Make sure the following tasks are checked

Describe the problem / feature
Write tests for features and bug fixes
Write code to solve the problem
Make sure you follow the current code style and write meaningful commit messages without tags

eloyesp

I like these simplifications a lot, just wanted to add that there might be a secondary bug that was hidden by the build artifacts.

That is: when bundle install a git sourced gem, it adds some non-existing paths to the load path, I've added a check for that on Bundler::Runtime#setup breaking multiple escenarios.

The problem is that I'm not entirely sure how rubygems and bundler interact when building extensions on git sourced gems.

Hope my comments help somehow.

lib/rubygems/ext/builder.rb

lib/rubygems/ext/ext_conf_builder.rb

deivid-rodriguez · 2022-12-16T10:53:27Z

That is: when bundle install a git sourced gem, it adds some non-existing paths to the load path, I've added a check for that on Bundler::Runtime#setup breaking multiple escenarios.

Yeah, building extensions for git gems is a big hack at the moment, and it indeed has the consequence of some paths to be added to the $LOAD_PATH that don't exist. That's actually what prevents us to refactor things further and also stop copying build artifacts to lib_dir, because that's apparently what makes extensions for git gems actually work.

We should refactoring things further to just be able to make, make install, make clean to install extensions, no matter the source. But it sounds too much for this PR.

Some tests check that the shared objects are actually installed, but checking an intermediate build file instead of the installed one.

The installed file not always have the `.so` extension. Co-authored-by: Eloy Espinaco <eloyesp@gmail.com>

deivid-rodriguez · 2022-12-16T14:10:16Z

After investigating the issues here, I think this is safe to do.

I could only see this breaking if someone is intentionally changing the $LOAD_PATH to point to where the intermediate artifacts live (normally the ext/ folder of an installed gem). These artifacts are already installed in two places right now (the lib folder of the installed gem, and the extensions folder), and both are already in the $LOAD_PATH by default. So it makes no sense that someone did this, and if they did, it can't hardly be considered a breaking change in my opinion. Also, cleanup is currently limited to extconf extension builder, so the scope is even more limited.

So from my side this is ready.

eloyesp · 2022-12-16T15:18:38Z

Just one more question, will this impact bundler installs? Is bundler installing gems using the extconf builder or is it running his own builder?

deivid-rodriguez · 2022-12-16T15:28:41Z

Yes, Bundler delegates to RubyGems to install extensions, so it will use whatever builder each gem uses. If gems define their extension through an extconf.rb file, then Bundler will use that builder from RubyGems.

simi · 2022-12-16T16:18:04Z

Thanks for digging in this @deivid-rodriguez. I did other review of the build artifact handling. I agree this seems safe. I did quick test to see impact on sassc issue sass/sassc-ruby#200 and bundle size went from 65MB to 25MB.

simi · 2022-12-16T16:18:28Z

Thanks @eloyesp for pushing this forward!

RubyGems started to clean built artifacts after building extensions since Ruby 3.2.0: rubygems/rubygems#6133 Related: rubygems/rubygems#6205

RubyGems cleans built artifacts after building extensions since 3.4.0: rubygems/rubygems#6133 Therefore cld3-ruby now needs to refer the installed binary instead of the binary at the build directory.

lipidity · 2023-01-04T05:03:22Z

Hi 👋

Running make clean after installing an extension breaks https://github.com/ohler55/ox because it removes the built artifacts which are needed at runtime.

simi · 2023-01-04T05:09:56Z

Hi wave

Running make clean after installing an extension breaks https://github.com/ohler55/ox because it removes the built artifacts which are needed at runtime.

#6205 (comment)

lipidity · 2023-01-04T05:12:51Z

@simi thanks, I should have linked to ohler55/ox#303 (it's still broken on macOS)

hsbt · 2023-01-04T05:35:19Z

@lipidity No. It works. Please confirm my comment

lipidity · 2023-01-04T22:26:44Z

I was wrong about ox still being broken. Confirmed that it works.

to support rubygems 3.4.0 ref: rubygems/rubygems#6133

eloyesp reviewed Dec 16, 2022

View reviewed changes

lib/rubygems/ext/builder.rb Outdated Show resolved Hide resolved

lib/rubygems/ext/ext_conf_builder.rb Outdated Show resolved Hide resolved

deivid-rodriguez force-pushed the clean_build_files_after_install_debug branch from 0afebd2 to 20e84f6 Compare December 16, 2022 10:41

eloyesp and others added 3 commits December 16, 2022 13:36

Fix tests checking intermediate files

ad52607

Some tests check that the shared objects are actually installed, but checking an intermediate build file instead of the installed one.

Use better name for variable

6f6681b

The installed file not always have the `.so` extension. Co-authored-by: Eloy Espinaco <eloyesp@gmail.com>

Cleanup intermediate artifacts after installing built extensions

98b6a95

deivid-rodriguez force-pushed the clean_build_files_after_install_debug branch from 20e84f6 to 98b6a95 Compare December 16, 2022 12:38

deivid-rodriguez marked this pull request as ready for review December 16, 2022 13:45

deivid-rodriguez added the rubygems: enhancement label Dec 16, 2022

deivid-rodriguez mentioned this pull request Dec 16, 2022

Clean built extensions after install #4017

Closed

4 tasks

deivid-rodriguez marked this pull request as draft December 16, 2022 13:46

deivid-rodriguez marked this pull request as ready for review December 16, 2022 14:10

simi self-requested a review December 16, 2022 14:15

simi merged commit 49c54da into master Dec 16, 2022

simi deleted the clean_build_files_after_install_debug branch December 16, 2022 16:18

This was referenced Dec 27, 2022

Fails to load with Rubygems 3.4 technion/ruby-argon2#56

Closed

Cleaning build artifact broke some of gems #6205

Closed

zunda added a commit to zunda/blurhash that referenced this pull request Dec 27, 2022

Load binary library from /lib/ instead of ../ext/blurhash/

7782e70

RubyGems started to clean built artifacts after building extensions since Ruby 3.2.0: rubygems/rubygems#6133 Related: rubygems/rubygems#6205

zunda mentioned this pull request Dec 27, 2022

Load binary library from /lib/ instead of ../ext/blurhash/ Gargron/blurhash#19

Closed

technicalpickles mentioned this pull request Jan 4, 2023

/opt/mastodon/vendor in docker image contains a lot of extra stuff mastodon/mastodon#21151

Open

tsroten mentioned this pull request Jan 6, 2023

Gem install fails for Ruby 3.2 in MacOS 12.6 zk-ruby/zookeeper#120

Closed

Shimokuni added a commit to Shimokuni/figure_set that referenced this pull request Mar 14, 2023

move exts to correct directory

7776e59

to support rubygems 3.4.0 ref: rubygems/rubygems#6133

Shimokuni added a commit to Shimokuni/figure_set that referenced this pull request Mar 14, 2023

move exts to correct directory

8f3965a

to support rubygems 3.4.0 ref: rubygems/rubygems#6133

Shimokuni added a commit to Shimokuni/figure_set that referenced this pull request Mar 14, 2023

move exts to correct directory

c630f4b

to support rubygems 3.4.0 ref: rubygems/rubygems#6133

Shimokuni mentioned this pull request Mar 14, 2023

Support rubygems 3.4.0 tsukasaoishi/figure_set#3

Merged

osyoyu mentioned this pull request Mar 16, 2023

Build .bundle into lib/ (RubyGems 3.4 compatibility) tomz/liblinear-ruby-swig#10

Merged

sudharsan-p mentioned this pull request Jul 10, 2023

Ruby 3.2 compatibility gverger/cbc-wrapper#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean built artifacts after building extensions #6133

Clean built artifacts after building extensions #6133

deivid-rodriguez commented Dec 15, 2022 •

edited

Loading

eloyesp left a comment

deivid-rodriguez commented Dec 16, 2022

deivid-rodriguez commented Dec 16, 2022 •

edited

Loading

eloyesp commented Dec 16, 2022

deivid-rodriguez commented Dec 16, 2022

simi commented Dec 16, 2022

simi commented Dec 16, 2022

lipidity commented Jan 4, 2023

simi commented Jan 4, 2023

lipidity commented Jan 4, 2023 •

edited

Loading

hsbt commented Jan 4, 2023

lipidity commented Jan 4, 2023

Clean built artifacts after building extensions #6133

Clean built artifacts after building extensions #6133

Conversation

deivid-rodriguez commented Dec 15, 2022 • edited Loading

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

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

Make sure the following tasks are checked

eloyesp left a comment

Choose a reason for hiding this comment

deivid-rodriguez commented Dec 16, 2022

deivid-rodriguez commented Dec 16, 2022 • edited Loading

eloyesp commented Dec 16, 2022

deivid-rodriguez commented Dec 16, 2022

simi commented Dec 16, 2022

simi commented Dec 16, 2022

lipidity commented Jan 4, 2023

simi commented Jan 4, 2023

lipidity commented Jan 4, 2023 • edited Loading

hsbt commented Jan 4, 2023

lipidity commented Jan 4, 2023

deivid-rodriguez commented Dec 15, 2022 •

edited

Loading

deivid-rodriguez commented Dec 16, 2022 •

edited

Loading

lipidity commented Jan 4, 2023 •

edited

Loading