postmodern / chruby

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

Set GEM_HOME only if needed, i.e., if Gem.default_dir is not writable #431

Closed eregon closed 3 years ago

eregon commented 4 years ago

See https://github.com/postmodern/chruby/issues/422#issuecomment-557379476 This is a cleaner alternative to https://github.com/postmodern/chruby/pull/423.

This solution feels both very nice and simple. With this PR, chruby only sets and uses non-default configuration when needed, i.e., when the default gem directory is not writable.

If the default gem directory is writable, then it just use the default configuration and does not set GEM_HOME, which has multiple advantages as described in https://github.com/postmodern/chruby/issues/422.

The only drawback I know is it causes user-installed gems to be together with pre-installed gems. In the rare cases where one wants to remove all user-installed gems, one simple solution (and the safest in general) is to reinstall that Ruby.

@postmodern @havenwood Would it be fine to merge this?

eregon commented 4 years ago

I still need to adapt the tests, but it would be great to get some thoughts/reviews on this approach.

postmodern commented 4 years ago

Obviously needs tests. Now that we have a test/fixtures.sh file, it should be possible to create a writable gem-dir and a non-writable gem dir and test which one chruby picks.

While normally I like to keep user-stuff separate from the ruby, even when the ruby was installed by myself (makes deleting user-installed gems easier), I do like the idea of being able to run path/to/ruby -S ... sans chruby and still having access to all of the additional user installed gems.

eregon commented 4 years ago

I added tests, please review. This PR is rebased on top of https://github.com/postmodern/chruby/pull/435 which would be good to merge first.

Regarding documentation, I need a clarification as I don't want more than necessary conflicts with https://github.com/postmodern/chruby/pull/419#issuecomment-605475964 Should this PR be targeting master (then we need #419 in master) or 1.0.0 (then we need to merge master to 1.0.0 or rebase 1.0.0 on top of master)?

eregon commented 4 years ago

Rebased on top of master.

postmodern commented 4 years ago

Probably should go into 1.0.0 because we're changing the behavior of where GEM_HOME points?

eregon commented 4 years ago

@postmodern I changed the base branch to 1.0.0 add added documentation and a ChangeLog entry. Could you review? I think it's ready to go now.

postmodern commented 4 years ago

It just occurred to me that RubyGem's "USER INSTALLATION DIRECTORY" ($HOME/.gem/$RUBY_ENGINE/$RUBY_API_VERSION) needs to be added to GEM_PATH and PATH, so that gems installed with gem install --user-install are accessible:

$ chruby ruby-2.5
$ which ruby
~/.rubies/ruby-2.5.5/bin/ruby
$ echo $GEM_HOME
/home/postmodern/.gem/ruby/2.5.5
$ echo $GEM_PATH
/home/postmodern/.gem/ruby/2.5.5:/home/postmodern/.rubies/ruby-2.5.5/lib/ruby/gems/2.5.0
$ gem env | grep "USER INSTALLATION DIRECTORY"
  - USER INSTALLATION DIRECTORY: /home/postmodern/.gem/ruby/2.5.0
$ gem install --verbose --user-install rake
...
/home/postmodern/.gem/ruby/2.5.0/gems/rake-13.0.1/rake.gemspec
/home/postmodern/.gem/ruby/2.5.0/bin/rake
Successfully installed rake-13.0.1
Parsing documentation for rake-13.0.1
Parsing sources...
100% [43/43]  lib/rake/win32.rb                        
Done installing documentation for rake after 0 seconds
1 gem installed
$ rake --version
rake, version 12.3.0
$ ruby -rrake -e "puts Rake::VERSION"
12.3.0

How should --user-install fit into the new ~/.gem/ directory structure and GEM_HOME behavior? Should we make a separate issue for this edge-case?

eregon commented 4 years ago

By "support" I mean "add it to PATH": I think chruby never supported --user-install and I think very few people use it because it's essentially a workaround useful only when GEM_HOME is not set and the default gem home is not writable. Such a condition was never the case in chruby, and will not be with this PR either.

I think rbenv doesn't support it either, but I'm not sure.

Using --user-install is actually harmful when e.g. there is a Ruby 2.5.1 and a Ruby 2.5.2 installed but with different configuration flags. It breaks the clean separation of gem directories essentially, due to using the ABI version (which is not enough to characterize all variants to build a Ruby which can affect binary compatibility). A concrete example is having 2.5.1 with --enable-shared and 2.5.2 without which will cause unwanted RubyGems warnings due to unintentional sharing of the gem directory.

Yes, I think that should be a separate issue and we should discuss there whether it's worth fixing. In any case, I think it's completely orthogonal to this PR, i.e. I think this PR doesn't change anything about that (except the small fact that the old logic would "accidentally" pick up --user-install binaries for Ruby versions ending in .0).

postmodern commented 4 years ago

Created issue #436.

eregon commented 4 years ago

@postmodern Done. However I couldn't run the tests locally to verify, I need the fixes in master. Could you rebase the branch 1.0.0 on top of current master?

eregon commented 4 years ago

@postmodern Could you rebase the branch 1.0.0 on top of current master?

BTW, the CI seems to pass, except some existing failure on macOS which seems unrelated to this PR.

postmodern commented 4 years ago

@eregon rebased and force pushed.

eregon commented 4 years ago

Thanks, I rebased this PR on top of that. Could you review this PR and merge it?

eregon commented 4 years ago

@postmodern Ping, could you merge this? I think it's ready.

postmodern commented 4 years ago

@eregon for some reason the CI is still failing on macOS: https://travis-ci.org/github/postmodern/chruby/jobs/684039143 Not sure how I feel about merging this before CI is fixed. Starting to suspect it's a TravisCI issue. Tempted to look into GitHub Actions as a replacement.

eregon commented 4 years ago

Right, GitHub Actions didn't work very well with shunit2 last time I tried though: https://github.com/postmodern/chruby/pull/435#issuecomment-609042549

Could you maybe rerun the CI on master to see if this error happens there too, or if it's specific to this PR (I don't see how, but one can never be sure about those) ?

postmodern commented 3 years ago

@eregon master indeed has the same problem on macOS. I'm holding off on merging this until there's consensus on rubygems/rubygems#2847. Since the issue overlaps with many of the same edge cases discussed here, I'm interested in what the guidance is from upstream rubygems and ruby maintainers.

eregon commented 3 years ago

@postmodern That's not very nice. We already extensively discussed this in the issue, and I made multiple PRs to try to find something you would accept. It's exhausting. Obviously this is a step in the right direction, and chruby needs to handle it for previous versions so whatever RubyGems defaults do is basically irrelevant and this approach would work just fine with it.

You're basically now forcing me to make a PR to RubyGems which might be a lot of work to get in (the discussion will likely not progress much until somebody makes an actual PR). There might not be consensus there before a very long time.

postmodern commented 3 years ago

@eregon no one is forcing you to do anything; however you choose to link the RubyGems --user-install discussion back to this issue which caught my attention ;) 1.0.0 breaking changes should not be rushed through.

I am concerned that RubyGems may change their GEM_HOME/GEM_PATH behavior after we have changed chruby's GEM_HOME/GEM_PATH behavior, which could cause --user-install to conflict with chruby or vice versa. I would prefer that chruby tries to mirror the behavior of RubyGems, while also safely supporting multiple isolated gem dirs and multiple rubies. If the RubyGems or Ruby Core teams can agree on an elegant way to isolate --user-install gem dirs of multiple Rubies, I'd prefer adapting their solution.

eregon commented 3 years ago

I am concerned that RubyGems may change their GEM_HOME/GEM_PATH behavior after we have changed chruby's GEM_HOME/GEM_PATH behavior,

I don't think so, I believe GEM_HOME/GEM_PATH will always override as they already do, it would be too incompatible to change that.

I think it might takes years (if it happens) to reach consensus there, the issue https://github.com/rubygems/rubygems/issues/1394 was created 5 years ago FWIW, and the problem exists since RubyGems exists (or with a broader view, since package managers and permissions exist).

So I think we should not prevent improvements in chruby while waiting for a potentially very long time for RubyGems. Can you decide a deadline by how long to wait for RubyGems? And at the deadline, decide to merge or reject this PR?

eregon commented 3 years ago

@postmodern I believe we reached consensus on the RubyGems PR, can you merge this?

eregon commented 3 years ago

@postmodern Could you merge this? It's been soon a year since I started this PR & related PR #423.

I think we've discussed plenty already, the RubyGems issue converged to the same approach, etc.

eregon commented 3 years ago

@postmodern The CI failures are the same as in https://travis-ci.org/github/postmodern/chruby/jobs/729920136, and seem caused by https://github.com/postmodern/chruby/commit/2ad3f73a2e2c4497936071e6d5a3cd2f71f0eb35 or earlier (not by this PR).

postmodern commented 3 years ago

@eregon I'm considering going in a different direction and providing a command-line option to set or not set GEM_HOME. Now I'm thinking that implicit variable behavior can lead to confusion, and that it's better to just give the user an option flag and let them decide. Still thinking about this.

postmodern commented 3 years ago

@eregon also I have switched from TravisCI to GitHub Actions. However, they are failing on macOS due to not being able to download an appropriate test ruby from https://rvm.io/binaries/osx/. CI passes on ubuntu-latest though.

eregon commented 3 years ago

@eregon I'm considering going in a different direction and providing a command-line option to set or not set GEM_HOME. Now I'm thinking that implicit variable behavior can lead to confusion, and that it's better to just give the user an option flag and let them decide. Still thinking about this.

Do you have something concrete? And is there an actual issue? Otherwise I'd like to suggest to merge this and refine if needed. You can of course still change your mind before the release if you want.

My point of view is if after one year, and many discussions from https://github.com/postmodern/chruby/issues/422, we figured this is a good solution, then delaying it further sounds to me like we'll never make any progress on this. https://github.com/postmodern/chruby/pull/423 was a sort of flag and you said no, so it feels we're going in circles.

As I think you know, the change I want is to not set GEM_HOME/GEM_PATH on chruby someruby (when Ruby is installed in a writable location, the vast majority of cases for developer machines), because that cause many subtle bugs (e.g. warning about not compiled extensions) when using another Ruby executable with its full path (e.g., because it's not added to chruby, and chruby is used to set a default recent Ruby rather than using an old/impractical system Ruby), or when contributing to a Ruby implementation.

postmodern commented 3 years ago

@eregon please don't attempt to pester or coerce me. I will release 1.0.0 when I am satisfied with the 1.0.0 branch and it's ready to be merged.

I am currently debating whether to add a --gem-home [dir] option and/or a --no-gem-home option, and how to do so without adding significant complexity to the chruby function. Instead of bifurcating the logic based on whether or not the GEM_ROOT is writable, thus possibly installing gems into two different places which makes debugging or clean up Rubies slightly more difficult, I'd prefer having a reasonable default (a GEM_HOME for the users gems, since it is the user who decides to switch rubies) and maybe some options for the user to override that default (something like --gem-home [dir] or -no-gem-home). Still writing prototypes for those options, but not satisfied with the code yet.

eregon commented 3 years ago

I didn't mean to do that, but yes I'm getting impatient and frustrated with how long this takes. (this PR has been opened for 10 months, we did multiple iterations on it, I addressed all your feedback and it's still not merged)

Instead of bifurcating the logic based on whether or not the GEM_ROOT is writable, thus possibly installing gems into two different places which makes debugging or clean up Rubies slightly more difficult

Isn't that exactly the typical user/superuser model on Unix? I.e., either the Ruby is installed in a system dir, then typically no user can add gems in it, only root can install gems directly in it. Or the Ruby is under some user home, and then only that user should be able to access it, no issue. Third possibility is Ruby is installed in a system dir, and some users can write to it, then they have similar rights to the the superuser regarding gems. It's the same behavior as --user-install, only when needed, so it maximizes sharing and convenience.

To me that seems the best behavior for all three cases, and it follows the permissions model so it's naturally simple to implement and support.

dentarg commented 3 years ago

warning about not compiled extensions

Is this warnings such as Ignoring bcrypt-3.1.16 because its extensions are not built. Try: gem pristine bcrypt --version 3.1.16? (I get a full screen of them each time a open a new tmux window (using macOS and zsh)

postmodern commented 3 years ago

@dentarg could you submit a separate issue with all important information (see scripts/bug_report.sh) so as not to clutter the comments on this PR?

That issue sounds more like the GEM_HOME sharing issue which #419 solves and has already been merged into the 1.0.0 branch, pending the release of 1.0.0.

postmodern commented 3 years ago

@eregon the difference between --user-install and your PR, is a user has to explicitly specify --user-install where as with your PR it's entirely implicit and dependent on whether the directory is writable or not. Currently chruby sets GEM_HOME based on whether the user is a superuser or not, thus allowing the superuser to install/update gems within the installed Ruby. The UNIX permission model and FHS lays out a model where installed software is kept separate from additionally installed user data/configuration/plugins/etc (/usr vs /usr/local vs /opt vs /home). Installing software into one's home directory (not explicitly supported by UNIX or FHS) complicates this assumption. XDG attempts to address some issues by defining specific directories where app data and configurations should be installed. Thus I am starting to reconsider automagically setting the GEM_HOME based on writability or the user's permissions.

I will continue mulling over which approach or philosophy can handle setting GEM_HOME, and how to best support things like updating built-in gems. Trying to get me to merge your PR by continued debate is not going to work.

eregon commented 3 years ago

Is this warnings such as Ignoring bcrypt-3.1.16 because its extensions are not built. Try: gem pristine bcrypt --version 3.1.16? (I get a full screen of them each time a open a new tmux window (using macOS and zsh)

Yes, that's what I meant.

@eregon the difference between --user-install and your PR, is a user has to explicitly specify --user-install where as with your PR it's entirely implicit and dependent on whether the directory is writable or not

I meant auto---user-install when writable, which is what the RubyGems discussion on that PR converged to. Earlier in this PR you said:

Since the issue overlaps with many of the same edge cases discussed here, I'm interested in what the guidance is from upstream rubygems and ruby maintainers.

So now it seems clear what the guidance is from upstream rubygems maintainers. I'm a CRuby committer and TruffleRuby maintainer and I believe this is the best approach considering multiple Ruby implementations and the ability to install head versions.

Installing software into one's home directory

It's what I'd think >95% Ruby developers do, and of course they want their gems under the $HOME somewhere. Not setting GEM_HOME make it safer and more reliable, while still keeping gems under $HOME for that case and for the system not-writable case. I think we want the same thing in the end, to keep it simple and convenient for users. And of course users don't want to specify chruby --no-gem-home 2.7.2 every time.

I will continue mulling over which approach or philosophy can handle setting GEM_HOME, and how to best support things like updating built-in gems. Trying to get me to merge your PR by continued debate is not going to work.

OK, this will be my last comment for now. Let's check end of January maybe?

postmodern commented 3 years ago

So now it seems clear what the guidance is from upstream rubygems maintainers.

While the rubygems change definitely helps users who are using system ruby, installed by the package manager into /usr, and just want to run gem install rails without using sudo, I would like to support additional use-cases. Such as, updating gems as root into a Ruby's GEM_ROOT as well as installing gems as root into ~/.gem/. I also want to support setting custom GEM_HOMEs for testing purposes, such as for TruffleRuby development/testing. I believe both common and uncommon use-cases can be supported without sacrificing one or the other.

It's what I'd think >95% Ruby developers do

Do you have statistics for this? Even if a majority of users prefer a certain configuration, does not mean we can't also support other configurations without inconveniencing the majority of users.

And of course users don't want to specify chruby --no-gem-home 2.7.2 every time.

That's why I said "reasonable default". I don't want to pick a default that is too auto-magical or variable, but not too overly explicit or strict. This is why I am still prototyping new chruby 1.0.0 APIs and throwing them away because I'm not happy with them. Allow me to work on 1.0.0 and I'm sure I can come up with something that balances everyone's needs/wants.

eregon commented 3 years ago

@postmodern Any more thoughts on this or work towards chruby 1.0.0? I can rebase this PR if you tell me you will merge it. I'm sad to see this is not yet merged after over a year, and a fair amount of work and feedback. It's also literally a 2-lines changed, so I think it's very safe and it would help many users.

I also want to support setting custom GEM_HOMEs for testing purposes, such as for TruffleRuby development/testing.

The user can simply set GEM_HOME themselves for this purpose, it's very easy and appropriate for testing. I don't think chruby needs any shortcut for that. Keeping chruby minimalist is good, isn't it?

postmodern commented 3 years ago

@eregon work continues on chruby 1.0.0. Currently experimenting with removing RUBIES entirely, XDG directory support, and adding more function hooks to allow for customization. What is primarily holding me back is the fact that GitHub Actions breaks shunit2 on it's macOS image. It appears to be cwd related or some file system change issue. We are effectively flying blind wrt macOS support. Due to the fact Apple still ships an extremely old version of Bash and an old-er version of Zsh than Linux, it is essential that we somehow test how chruby performs in macOS'es environment.

postmodern commented 3 years ago

After lengthy discussion, I believe #419 provides sufficient protection against sharing a GEM_HOME between rubies. Additional edge-cases appear to be related to RubyGem's poor isolation of compiled C extensions and when a Ruby is upgraded in place such that it's directory name does not change but it's ABI does (ex: installing the most recent truffleruby nightly release). Also, if the user installs a nightly build of ruby or compiles ruby from HEAD, then I think it is the user's responsibility to run gem pristin --extensions to rebuild any C extensions which may have broken due to ABI changes and an in-place upgrade; although I would prefer RubyGems handled this automatically for users. Also, this pull request does not handle the edge-case where a nightly build of truffleruby is installed into /opt/rubies/ and is not writable to a regular user, which then causes the GEM_HOME to be ~/.gem/truffleruby-dev/ and we're back to having potential ABI breakages between in-place upgrades of a ruby. These RubyGems ABI edge-cases also can occur if a user does not use chruby, but explicitly sets GEM_HOME and manually upgrades a nightly version of truffleruby. Thus it is not specifically a problem in chruby, and chruby does have an explicit policy of not accepting OS or Ruby specific workarounds (which includes TruffleRuby and RubyGems). I believe it is better to fix the problem in RubyGems so that everyone benefits, not just chruby users or truffleruby users.

(Also, 1.0.0 will support chruby_gem_home functions which will allow users such as yourself to configure the specific behavior of how chruby sets GEM_HOME.)

eregon commented 3 years ago

Thank for your decision, I think it's better to decline the PR than leave it undecided longer. We disagree on how much a Ruby switcher should protect against invalid sharing (I'd say as much as possible) and whether the Ruby switcher should even set GEM_HOME (I argue much safer to not set it), but that's fine. I will probably end up creating my own minimal Ruby switcher inspired by chruby, never setting GEM_HOME. And/or maybe just migrate to rbenv.

You have a point that RubyGems could still use a sub-folder when GEM_HOME is set to ensure having the ABI as part of the gem path even when GEM_HOME is set and does not include ABI. This is actually what Bundler does for bundle config --local path vendor/bundle (e.g., vendor/bundle/truffleruby/2.7.2.1), so that always works safely and nicely. I'm somewhat doubtful this can be solved in RubyGems, because it would likely cause huge compatibility issues (or significant complexity), IMHO it is much harder to solve than at chruby level, where it's more expected since chruby sets GEM_HOME. It might still be worth filing an issue to let them know though. Unless maybe you have some ideas how RubyGems could ensure using ABI and not cause any breaking change?

postmodern commented 3 years ago

I'm somewhat doubtful this can be solved in RubyGems, because it would likely cause huge compatibility issues (or significant complexity)

That is making a lot of assumptions without any evidence. This issue could be easily solved in RubyGems by instead of copying the built .so/.dylib file into lib/, but adding the extensions/ sub directory to $LOAD_PATH so that require "my_ext" still works. This might break some obscure ruby C bindings that explicitly load their extensions using require_relative or an absolute path to the built extensions file in lib/; although that practice seems to be rare based on the C extensions I have checked so far (unf_ext, nokogiri, bcrypt, sqlite3). It's still worth suggesting to the RubyGems project, since users can still technically trigger the bug when explicitly setting their own GEM_HOME and upgrading rubies. Nothing will be improved if we don't at least try. I can submit a feature suggestion / enhancement issue if you don't want to.

If using the the Ruby ABI version in GEM_HOME works for bundler, would it also work for chruby? Or are there potential issues with static vs dynamically linked rubies potentially sharing previously built C extensions from the same lib/ directory?