postmodern / chruby

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

Make chruby_reset undo only the changes made by chruby_use #379

Closed tomeon closed 6 years ago

tomeon commented 7 years ago

This PR alters how chruby handles environment variables in order to make sure that chruby_reset undoes only those changes that chruby itself has made. Right now, when chruby cleans up after itself, it does a pretty thorough reaving of PATH, GEM_HOME, etc., which can lead to situations like this:

$ source chruby.sh
$ export RUBIES=(/usr)
$ chruby usr
# ...much later...
$ chruby system
$ bash -c 'echo ":)"'
bash: bash: command not found
$ /usr/bin/bash -c 'echo ":("'
:(

I.e., chruby_reset scrubs all occurrences of /usr/bin from PATH, not just those added in chruby_use.

The code added in this PR caches the pre-chruby_use state of various environment variables, along with other values that represent the changes made by chruby_use. These values are used within chruby_reset to

  1. check whether any changes (e.g., additions to PATH or GEM_HOME) have occurred since chruby_use was last invoked, and
  2. restore the shell environment to its earlier state, if this is possible.

If chruby can't be sure that its cleanup logic will restore the shell environment to its earlier state, it will print a warning to that effect and fall back to default cleanup logic.


Because this PR violates the contributors' guide's prohibition on new environment variables, I will also open a PR for an alternate implementation that uses a function rather than the CHRUBY_CACHE global associative array as the oracle for assessing the correctness of the shell environment.

However, the function-based approach is not as reliable as the approach taken here: less state is maintained between chruby_use and chruby_reset, so the cleanup logic has to do some guesswork about the preexisting shell environment that isn't necessary when the relevant attributes of that previous environment are maintained in CHRUBY_CACHE.

I also think that CHRUBY_CACHE represents minimal environment pollution -- for instance, it won't show up in env/printenv nor be inherited by any non-bash/zsh child processes. Maybe it is okay to make an exception in this case?


Also included in this PR:

  1. An update to the Vagrantfile that adds Travis' own travis-ruby Docker machine, and
  2. Enhancements to the test setup script to automatically clean up test/opt/rubies in case the rvm download failed. Because the Makefile only runs the setup script if the test/opt/rubies target doesn't exist, an extant-but-empty directory doesn't trigger a build failure -- instead, the test target fails somewhere downstream when the absence of the test Ruby installation causes chruby to carp about an unknown Ruby.
postmodern commented 7 years ago

Note that we cannot use associated arrays, as they are a Bash 4 feature and OSX still ships Bash 3. Perhaps a better way would be to store the switching history as the arguments passed to chruby in a regular indexed Array?

I like the idea of undoing changes, but we lack a good way of display the change history. How far back should we track chruby switches? Also, is this feature really needed or will it add more cognitive overhead to the user?

The Vagrant changes should be a separate PR. I'm open to replacing Vagrant with schroot or shudder even Docker.