Fix loading of nvm for brew-installed nvm

[brianphillips]

Loading a brew-installed nvm was previously broken. This PR fixes it.

Description

This reverts to the original (working) version from 2017. During the most recent refactor to use the _bash_it_homebrew_check helper method, it was overlooked that ${BASH_IT_HOMEBREW_PREFIX}/nvm.sh is not the equivalent to $(brew --prefix nvm)/nvm.sh since BASH_IT_HOMEBREW_PREFIX is set from brew --prefix instead of brew --prefix nvm.

Motivation and Context

This fixes a regression caused by 65ef8e2.

How Has This Been Tested?

I've tested this on my local instance where I have nvm installed via brew.

Screenshots (if appropriate):

N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• If my change requires a change to the documentation, I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
• I have added tests to cover my changes, and all the new and existing tests pass.

[davidpfarrell]

TIL some formula can have unique prefixes, which is probably why the formula name is an optional argument to the --prefix option:
from manpage

--prefix [--unbrewed] [--installed] [formula ...]
       Display Homebrew´s install path. Default:

       •   macOS Intel: /usr/local

       •   macOS ARM: /opt/homebrew

       •   Linux: /home/linuxbrew/.linuxbrew

       If formula is provided, display the location where formula is or would be installed.

       --unbrewed
              List files in Homebrew´s prefix not installed by Homebrew.

       --installed
              Outputs nothing and returns a failing status code if formula is not installed.

My guess is that this bug pretty much fully invalidates BASH_IT_HOMEBREW_PREFIX and shows that we should be using something like $(brew --prefix --installed $forumula) in its place.

There might be a way to introduce a convenience function like _bash_it_homebrew_prefix that also does the _bash_it_homebrew_check and possibly simplify the call-sites. (I haven't thought this out though and may be wrong)

[brianphillips]

@davidpfarrell What's the next step here?

[davidpfarrell]

When we were using a variable that almost-certainly existed with a valid value, the reference was fine.

BUT using the result of a sub-shell that may not return a valid value seems more risky.

ie. When brew is not managing the nvm install, you get a msg on stderr:

Error: No available formula with the name "nvm".

But the test continues, essentially checking for "/nvm.sh".

I think we need to isolate the fetching of the prefix call and check it if it's valid before using it.

Also, invoking brew twice seems a bit heavy.

It also feels like it runs the risk of triggering brew's random update check, although I haven't researched if it can be triggered during a --prefix check ...

[NoahGorny]

This is only reverting to the old approach. I think that we should probably merge this fix, and then improve BASH_IT_HOMEBREW_PREFIX as a further improvement

[brianphillips]

@davidpfarrell I've attempted a slightly different approach. Can you take a look please?

[brianphillips]

Also, I've verified that brew --prefix ... is actually quite a lightweight operation and won't trigger auto-updates: https://github.com/Homebrew/brew/blob/8421b702c5bdc0861b4c794af342d8705e6bc622/Library/Homebrew/brew.sh#L99-L101

[davidpfarrell]

Hey!

Glad to see progress on this. I'm totally fine punting on a global brew_prefix_installed function and just checking it explicitly here.

I do have a few notes, but I think its close!

[davidpfarrell]

Since $NVM_BREW_PREFIX already contains the result of $(brew --prefix nvm), shouldn't this be:

source "${NVM_BREW_PREFIX}/nvm.sh"

[brianphillips]

fair point, fixed

[davidpfarrell]

In my (now old) message, I mentioned:

$(brew --prefix --installed $forumula)

With the --installed option, it only returns a value if the formula is installed.

Without it, when the formula is not installed, it would return where the formula would be installed (vs returning nothing)

Maybe you can test this logic locally to confirm, but I think we want the --installed flag here.

[brianphillips]

brew --prefix --installed $formula runs about 30x slower (over a second on my machine) than brew --prefix $formula (30ms) because it has to exec a ruby script instead of just staying in the bash portion of brew. And since we're checking the existence of the script just below, I don't think we should care about the specifics of brew --prefix --installed nvm vs just brew --prefix nvm

[seefood]

seemsw legit.