postmodern / chruby

Changes the current Ruby
MIT License
2.85k stars 190 forks source link

Use the Ruby directory basename to decide the GEM_HOME #419

Closed eregon closed 4 years ago

eregon commented 4 years ago

This is the better version of https://github.com/postmodern/chruby/pull/410, discussed in that PR starting from https://github.com/postmodern/chruby/pull/410#issuecomment-509155061

I believe this is the right fix for always setting GEM_HOME correctly to a separate directory per Ruby installation.

I added an entry in the changelog along with upgrade notes. I also updated the README.

Fixes https://github.com/oracle/truffleruby/issues/1723.

@postmodern @havenwood What do you think? Could we merge this?

eregon commented 4 years ago

Tests passing now, I had to adapt one expectation.

postmodern commented 4 years ago

Also, if we're going to change chruby's gem home directory schema, perhaps we should move it out of ~/.gem/ entirely so as not to contaminate RubyGems ~/.gem/ and to make cleanup of chruby easier? Maybe ~/.gems/ or even ~/.local/share?

postmodern commented 4 years ago

It might be possible to write an ugly migration script to rename chruby's gem dirs based on the #!/path/to/$ruby_dir/bin/ruby of any bin stubs within $gem_dir/bin/.

postmodern commented 4 years ago

Added a 1.0.0 milestone and separate branch.

eregon commented 4 years ago

@postmodern Thank you for the great review, I'll take a look at addressing it soon.

eregon commented 4 years ago

Also, if we're going to change chruby's gem home directory schema, perhaps we should move it out of ~/.gem/ entirely so as not to contaminate RubyGems ~/.gem/ and to make cleanup of chruby easier? Maybe ~/.gems/ or even ~/.local/share?

I would keep it under .gem, that directory already exists and it's very easy to find and I find it intuitive.

eregon commented 4 years ago

Added a 1.0.0 milestone and separate branch.

Thanks, should I target this PR to the 1.0.0 branch then?

eregon commented 4 years ago

3. Probably need a safer way of cleaning up chruby gem homes inside ~/.gem/ without deleting gems installed outside of chruby (aka system ruby).

That seems difficult, because gem install --user-install would install to e.g. ~/gem/ruby/2.6.0, i.e., potentially mixing the gems with chruby < 1.0's gems.

Maybe we should just mention to look at directories under ~/.gem and consider removing them to save disk space? For many cases, I'd think rm -rf ~/.gem would be correct though. If one is using chruby then one likely doesn't install gems on the system ruby.

eregon commented 4 years ago

@postmodern I updated the Upgrade Notes with some of the discussion here. Now waiting from your feedback.

postmodern commented 4 years ago

Currently the 1.0.0 branch is identical to master. This will be merged into the 1.0.0 branch, so probably no need to rebase it.

havenwood commented 4 years ago

The 0.4.0 branch does have the addition of a manpage. If we want to add the manpage in 1.0.0, it would just need updating to reflect these changes.

eregon commented 4 years ago

@postmodern I updated the documentation, I think it's both safe and accurate now. Could we merge this?

I updated the PR to target the 1.0.0 branch.

eregon commented 4 years ago

@postmodern @havenwood is there anything left to do before merging this?

postmodern commented 4 years ago

Finally merged something :stuck_out_tongue:

eregon commented 4 years ago

@postmodern Awesome, thanks! (I reacted with a :tada: before but might as well write it in words).

Any plan as to when to release 1.0.0, and if other changes should make it for 1.0.0? I'd like a way to solve #422 for 1.0.0.

eregon commented 4 years ago

@postmodern What's your branching model? Shouldn't this change be on master?

Or should https://github.com/postmodern/chruby/pull/431 target the 1.0.0 branch? But that needs test fixes from master.

postmodern commented 4 years ago

@eregon once I get ready to release 1.0.0, I merge the 1.0.0 branch into master and hit release. I need to start doing the release engineering work to get 1.0.0 ready (ChangeLog, README, man pages, homebrew recipe, etc).

eregon commented 4 years ago

OK, should I retarget https://github.com/postmodern/chruby/pull/431 against the 1.0.0 branch then?

Could you merge master to 1.0.0, or rebase 1.0.0 on top of master? https://github.com/postmodern/chruby/pull/431 depends on test fixes in master.

postmodern commented 4 years ago

@eregon fixes and tests can go into master, and we can always rebase other branches.

eregon commented 4 years ago

I realized, while this works well for releases, it works less nicely for -dev/-head builds which always have the same directory basename. https://github.com/postmodern/chruby/pull/431 will fix that though.

eregon commented 3 years ago

I realized, while this works well for releases, it works less nicely for -dev/-head builds which always have the same directory basename.

431 will fix that though.

@postmodern I forgot about this, but the point above seems to make #431 (that is not setting GEM_HOME) the only solution to reliably have a gem home per Ruby build, also when having local Rubies or head Rubies installed (assuming these Rubies are rm -rf before a reinstall, which seems very much recommended for head Rubies).

eregon commented 2 years ago

I just realized, this approach is not good enough for any head Ruby, e.g. ruby-head (while ruby-install doesn't support it, chruby doesn't say anything about head Rubies AFAIK), because the ABI then changes but the name does not if reinstalled. A workaround is users manually clean the GEM_HOME, but this feels impractical and easy to forget. There is work in CRuby to actually give a proper ABI version for CRuby dev builds.

So a combination of ruby-name + ABI version (RbConfig::CONFIG["ruby_version"]) would work, or of course simply not setting GEM_HOME: https://github.com/postmodern/chruby/pull/431

postmodern commented 2 years ago

If a user is running and updating ruby-head, then it should be the user's responsibility to run gem pristin --extensions to rebuild any extensions that were compiled against a previous ABI. Or, rubygems could better handle edge cases where C extensions were previously built, but against the an older ABI or static vs. dynamic libruby, and print an informative error message. There is a limit to how much chruby can protect users from themselves, and when rubygems should consider how they could better handle exceptional edge-cases.

Not setting GEM_HOME is simply not an option, as that would not work if rubies default installation directory is not writable. I believe I have pointed this out previously. Many users, including myself, have rubies installed into /opt/rubies/ which is a perfectly acceptable option, even for regular users not in a server environment. There are benefits to installing software outside of your home directory, such as preventing accidental modification/deletion, separating your user data from user installed software, and better organization of user installed software.

eregon commented 2 years ago

With https://github.com/ruby/ruby/pull/5474, CRuby would raise LoadError if the ABI doesn't match for dev versions, so that should be helpful and be much clearer than a segfault or so later on.

I understand your point of view, I just don't think people are easily aware they need to both rm -r ruby-head-prefix && rm -r ~/.gem/ruby-head before reinstalling ruby-head (or recompile all extensions as you said instead of second rm -r).

With the current chruby release + CRuby head reporting RUBY_VERSION as the next release (e.g., RUBY_VERSION is 3.2.0 on ruby-master currently), dev builds and the 3.2.0 release when it comes out share the same gem home, so that is rather unfortunate, and CRuby releases will probably still not warn on ABI mismatch for releases.

IMHO it's hard for RubyGems to detect ABI mismatches (and it doesn't currently), that should be the job of the Ruby implementation and e.g. TruffleRuby always checks the ABI matches. RubyGems could try to better separate per ABI version, but the current design when GEM_HOME is set is RubyGems just uses that path and assumes it's correct whether it is or not. Hence the responsibility is moved to whatever sets GEM_HOME as things stand.

eregon commented 1 year ago

Linking things together, I think https://github.com/postmodern/chruby/pull/410 is actually better than this merged PR, because it also works e.g. for ~/.rubies/ruby-dev being reinstalled and changing its ABI without changing the basename. More details in https://github.com/postmodern/chruby/issues/490#issuecomment-1455164350