Skip to content
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

Add brew integration description #415

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gonzalom
Copy link

Just adding a brew integration description.

#### brew

``` bash
RUBIES+=(/usr/local/Cellar/ruby/*)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's worth showing brew --cellar to accommodate alternative brew prefixes?

RUBIES+=("$(brew --cellar ruby)"/*)

On the other hand, the hardcoded /usr/local version is prettier.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think about that option.

Not sure if it could be a custom path for it. However, as it's documentation, not a script, it could have both options documented.

In my case, I rather set the path, as this would be one command less to execute every time I start a terminal session.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are good points. I think it's fine to hard code to the brew prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to add engine-version to RUBIES rather than just version. With Ruby 2.6.3 for example, it'd currently be 2.6.3 instead of ruby-2.6.3.

I also wonder about other Ruby engines that brew has packages for. Is it worth adding those too, or maybe just separate examples for Ruby and JRuby?

for ruby_engine in jruby mruby ruby; do
	for ruby in /usr/local/Cellar/$ruby_engine/*; do
		RUBIES+=("${ruby%/*}/$ruby_engine-${ruby##/*/}")
	done
done

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't tell about the jruby ones, as I just use homebrew for it. Sounds good to me, but as I can't test it, I wouldn't dear :D

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to propose you this:

RUBIES+=(
  /usr/local/Cellar/jruby/*
  /usr/local/Cellar/mruby/*
  /usr/local/Cellar/ruby/*
)

But here is my experience...

First of all, I installed (throw homebrew) jruby and mruby to check it out.

The binary directories would be then:

  • /usr/local/Cellar/ruby/2.6.5/bin/
  • /usr/local/Cellar/jruby/9.2.9.0/bin/
  • /usr/local/Cellar/mruby/2.0.1/bin/

If I run your loop, it would check for this:

/usr/local/Cellar/jruby/jruby-9.2.9.0
/usr/local/Cellar/mruby/mruby-2.0.1
/usr/local/Cellar/ruby/ruby-2.6.5

Which is not right, as you can see, so you can just run it like:

for ruby_engine in jruby mruby ruby; do
	for ruby in /usr/local/Cellar/$ruby_engine/*; do
		echo "${ruby%/*}/${ruby##/*/}"
	done
done

But what would be the point? It would do the same as what I propose:

RUBIES+=(
  /usr/local/Cellar/jruby/*
  /usr/local/Cellar/mruby/*
  /usr/local/Cellar/ruby/*
)

But there is a problem here:

> chruby
   2.6.5
   9.2.9.0
   2.0.1
> chruby 2.6.5
> ruby --version
ruby 2.6.5p114 (2019-10-01 revision 67812) [x86_64-darwin19]
> chruby 2.0.1
chruby: /usr/local/Cellar/mruby/2.0.1/bin/ruby not executable
> chruby 9.2.9.0
chruby: /usr/local/Cellar/jruby/9.2.9.0/bin/ruby not executable

You see what it's doing? and why it fails? is because it is trying to get the {path}/bin/ruby binary, but in the case of jruby and mruby, the binaries are called jruby and mruby, so it fails.

In conclusion:

  1. I don't see the point of adding a for loop, I think that it would be better to use just the * wildcard.
  2. It will not work anyways, unless there is a way to tell chruby the name of the binary (I don't now if there is a way or not), in which case, this is not the purpose of this PR, I think.

So I suggest to get stick with the current proposal.

@gonzalom
Copy link
Author

Merged with master in order to pass the tests.

In any case, this PR is just documentation, it does not touch the code really, but it looks better this way ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants