-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Set GEM_HOME only if needed, i.e., if Gem.default_dir is not writable #431
Conversation
I still need to adapt the tests, but it would be great to get some thoughts/reviews on this approach. |
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 |
c3f65f3
to
9f6ca13
Compare
I added tests, please review. Regarding documentation, I need a clarification as I don't want more than necessary conflicts with #419 (comment) |
9f6ca13
to
b24f696
Compare
Rebased on top of master. |
Probably should go into 1.0.0 because we're changing the behavior of where |
56e3c9d
to
816ae83
Compare
@postmodern I changed the base branch to |
It just occurred to me that RubyGem's "USER INSTALLATION DIRECTORY" (
How should |
By "support" I mean "add it to PATH": I think Using Yes, I think that should be a separate issue and we should discuss there whether it's worth fixing. |
Created issue #436. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you split the tests that expect the GEM_ROOT
to be read-only vs. writable into separate files? That way we can have different setUp
/tearDown
logic per-file.
6bcf13d
to
47dae0d
Compare
@postmodern Done. |
assertEquals "did not clear the path table" \ | ||
"" "$(hash)" | ||
fi | ||
chruby_reset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a missing chruby_reset
in the test here.
@postmodern Could you rebase the branch BTW, the CI seems to pass, except some existing failure on macOS which seems unrelated to this PR. |
@eregon rebased and force pushed. |
47dae0d
to
4b3b9da
Compare
Thanks, I rebased this PR on top of that. |
* Gems for a given Ruby will either be stored under that Ruby installation prefix (if writable) or into a directory containing the full Ruby name, which should achieve that gems are never shared between multiple Ruby versions, which needed the workarounds mentioned here.
* GEM_HOME/GEM_PATH are only set if GEM_ROOT is not writable.
4b3b9da
to
b4f540b
Compare
@postmodern Could you merge this? I think we've discussed plenty already, the RubyGems issue converged to the same approach, etc. |
@postmodern The CI failures are the same as in https://travis-ci.org/github/postmodern/chruby/jobs/729920136, and seem caused by 2ad3f73 or earlier (not by this PR). |
@eregon I'm considering going in a different direction and providing a command-line option to set or not set |
@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 |
Do you have something concrete? And is there an actual issue? My point of view is if after one year, and many discussions from #422, we figured this is a good solution, then delaying it further sounds to me like we'll never make any progress on this. As I think you know, the change I want is to not set |
@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 |
I didn't mean to do that, but yes I'm getting impatient and frustrated with how long this takes.
Isn't that exactly the typical user/superuser model on Unix? 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. |
Is this warnings such as |
@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 |
@eregon the difference between I will continue mulling over which approach or philosophy can handle setting |
Yes, that's what I meant.
I meant auto-
So now it seems clear what the guidance is from upstream rubygems maintainers.
It's what I'd think >95% Ruby developers do, and of course they want their gems under the $HOME somewhere. Not setting
OK, this will be my last comment for now. Let's check end of January maybe? |
While the rubygems change definitely helps users who are using system ruby, installed by the package manager into
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.
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. |
@postmodern Any more thoughts on this or work towards chruby 1.0.0?
The user can simply set |
@eregon work continues on chruby 1.0.0. Currently experimenting with removing |
After lengthy discussion, I believe #419 provides sufficient protection against sharing a (Also, 1.0.0 will support |
Thank for your decision, I think it's better to decline the PR than leave it undecided longer. 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 |
That is making a lot of assumptions without any evidence. This issue could be easily solved in RubyGems by instead of copying the built If using the the Ruby ABI version in |
See #422 (comment)
This is a cleaner alternative to #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 #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?