Rewrite chruby-fish in fish

[bouk]

Hi, I rewrote chruby.fish and auto.fish from scratch, so it doesn't rely on the bash chruby. This makes things faster and fixes #31, closes #37, closes #36

I ran the tests and they pass, except for the one about sourcing .bash_profile of course, so I removed that one.

[bouk]

I'm not going to try and keep debugging this travis mess!

[bouk]

oh sweet it worked

[JeanMertz]

Amazing work @bouk and thanks for getting the tests to pass.

Interestingly enough, the original version (5869c05) of this library was actually written in pure Fish. It was later rewritten as a wrapper script in 3c94b5a to make it easier to keep up-to-date with the official chruby scripts.

Having said that, I think chruby itself has matured over the years, and there have been no breaking changes for several years now. Similarly, Fish has also matured a lot and has gained many built-in functions that make it easier and more performant to use a pure-Fish implementation.

So I'd say now would be a good time to switch back to a pure Fish implementation.

I left a few remarks, but overall this looks good.

I propose we release this as 1.0.0-beta.1 so that we can get some testing going, before we release the final 1.0.0 version which basically makes this library "done", other than bug fixes we might fix going forward.

[JeanMertz]

It's probably a good idea to add a note to the README about the minimum supported Fish version, given that we're now using some built-in functions that only became available in later versions of Fish.

I'm fine with setting the minimum supported version to the latest stable release of Fish. If others find that things work on their version, I'd accept PRs reducing the minimum supported version based on their feedback.

[JeanMertz]

I don't think we want auto to always load in our tests. We want to test auto-loading separate from the regular setup, so that we know both behave as expected.

[bouk]

I've addressed your comments @JeanMertz ! Thanks for the review

[JeanMertz]

@bouk thanks so much for the effort you put in.

I've been using the Fish version for a while since I've had to work on a Ruby project for the first time in a long while.

I'm going to keep this PR open for a while, but so far, things look promising 👍 I do notice some slowdowns in certain situations, but haven't had enough time to dig in yet. I suspect it has something to do with auto-detection, but I'm not sure yet.

[bouk]

I'm very curious where it's slower! I wonder if you could try fish --profile to dig into it.

[waynehoover]

This is great, is there an easy way to test this now?

[monfresh]

Hello. Any updates on this? Is there anything we can do to allow this to be merged?

[lazyatom]

This is great, is there an easy way to test this now?

I'm testing it by:

1. uninstalling the existing version (I installed it using homebrew, so brew uninstall chruby-fish),
2. cloning this fork
3. checking out the branch in this PR (rewrite-fish)
4. running make install
5. removing lines in my fish config that sourced the old version (e.g. source /usr/local/share/chruby/chruby.sh); this approach doesn't need them

[monfresh]

@JeanMertz Do you still have time to maintain this repo? If not, would it make sense to add other maintainers? It would be really great to have this PR merged and released. Thanks!

[bouk]

I would gladly take over maintenance

[ioquatix]

Any update on this?

[ioquatix]

My made my own fork wit these changes and can confirm they work as expected.

[todd-a-jacobs]

In my personal experience, RVM is very Bourne-centric. As a result, it doesn't play well with Fish. One way around this would be to use RVM to install a given Ruby, but then call a clean environment for the actual chruby testing. I haven't spent much time on this yet, but it seems like leveraging RVM to create the environment and then using another stage or add-on to ensure that we're actually in Fish without RVM (but with chruby installed) would be useful.

After looking at the code, it looks like the Travis CI YAML is more or less doing that already. If that's the case, what's the actual problem that still needs to be solved here?

[ioquatix]

@bouk @JeanMertz any update on where we are at with this?

I've been testing my fork on lots of machines and it is working well. https://github.com/ioquatix/chruby-fish

[tenderlove]

Hi,

Sorry to be another noisy +1 on this PR, but I also ran in to #31 and this PR fixes it. My use case is that I'm on an M1 now, but I want to have a Terminal that opens under Rosetta and one that does not. Then have my Fish config change PATH depending on my architecture. Specifically I have two installations of homebrew, one for x86_64 and one for arm64. I want the brew command to pick the right one depending on what arch returns so I've got this in my config.fish:

if test (arch) = "i386"
  echo "Intel"
  set HOMEBREW_PREFIX /usr/local
else
  echo "M1"
  set HOMEBREW_PREFIX /opt/homebrew
end

fish_add_path $HOMEBREW_PREFIX/bin

Unfortunately the path gets messed up due to #31, and this PR fixes it.

I'm currently testing by doing this in my config.fish:

set fish_function_path $fish_function_path $HOME/git/chruby-fish/share/fish/vendor_functions.d

It's totally a hack, but I didn't want to make install until something more official happens with this PR.

Anyway, thanks for your time and I hope my context makes sense!

[ioquatix]

@tenderlove just use my fork it merged all the patches.

[tenderlove]

@tenderlove just use my fork it merged all the patches.

yep that's what I'm doing! 😄

[ioquatix]

A subset of the engineers at my current work are using it with great success, so @JeanMertz it's well tested.

[ioquatix]

@tenderlove feel free to submit a PR to the README on my repo if there is some interesting use case affecting M1, it's really helpful for people to document some "typical" use cases otherwise you can spend hours trying to figure out how to do something. Splitting M1/Rosetta is something a lot of Ruby engineers are wrangling with, so any insight you have would be most welcome.

[dalizard]

@JeanMertz Any capacity to take a look at those changes again? 😅

[JeanMertz]

Unfortunately, I don't use Ruby at my day job any more, but I'd be happy to add contributors to this repository to keep it maintained. If anyone in this discussion wants to take on that role, let me know.

[bouk]

@JeanMertz I'll gladly take over maintainership of this project!

[JeanMertz]

@JeanMertz I'll gladly take over maintainership of this project!

Awesome. I sent you an invite.

[ioquatix]

@JeanMertz can you please add me as an admin/maintainer too?

[JeanMertz]

@JeanMertz can you please add me as an admin/maintainer too?

Done. Between the two of you, I hope we can keep the project in a good state, to everyone's benefit. Thank you so much for making the effort to help everyone @bouk and @ioquatix ❤️

[bouk]

Excellent, thank you @JeanMertz !

outdated review

[bouk]

I've replaced the Travis config with GitHub Actions. @ioquatix this is now ready for review.

@JeanMertz could you disable Travis for this repo, or at least remove the required check? I can't access the settings.

[ioquatix]

I was largely happy with these changes and already have them in "production" so to speak, so I'm going to merge it and then follow up with any missing bits (if any) from my repo.