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

Properly detect NVM path on macOS using homebrew #5786

Merged
merged 7 commits into from
Mar 26, 2018

Conversation

gravityrail
Copy link
Contributor

@gravityrail gravityrail commented Mar 25, 2018

Description

When attempting to build the plugin on macOS, I encountered the following error:

(path to plugins)/gutenberg (master)$ ./bin/setup-local-env.sh
./bin/install-node-nvm.sh: line 13: /Users/me/.nvm/nvm.sh: No such file or directory

By detecting homebrew and using the --prefix command, we can find the true path to nvm.sh on macOS systems.

How Has This Been Tested?

By running ./bin/setup-local-env.sh and seeing that it completes without error. Hopefully my else clause allows this to work properly on Linux machines, which I have NOT tested.

I also tried with nvm uninstalled, ensuring that brew was used to install nvm on systems with homebrew available.

Types of changes

Check for brew command and use to find nvm.sh, and to install nvm if necessary.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@pento
Copy link
Member

pento commented Mar 26, 2018

NVM explicitly doesn't support installing with homebrew, so we definitely don't want to be using homebrew to install NVM.

I'd be okay with the Load NVM logic to look something like:

  • Check if $NVM_DIR is not null.
    • Check if $NVM_DIR/nvm.sh exists.
      • If it exists, load it.
      • If it doesn't exist, check if command_exists brew is true.
        • If brew exists, check if $(brew --prefix nvm)/nvm.sh exists.
          • If it exists:
            • Load it.
            • echo -e $(warning_message "NVM is installed using homebrew, which is not supported by the NVM maintainers. If you run into problems with NVM, it's recommended that you remove it using the following command:" )
            • echo -e $(warning_message "$(action_format "brew uninstall nvm")" )
            • echo -e $(warning_message "After that, re-run the setup script to install NVM correctly." )

@gravityrail
Copy link
Contributor Author

gravityrail commented Mar 26, 2018

That seems reasonable. I didn't realise nvm wasn't supported via homebrew.

I think rather than warning people who use nvm via homebrew, let's just silently try to use it if it's there and the regular way of running nvm fails. The reason being, if they have installed it via homebrew and aren't having problems (as I have, with zero issues) then I don't see a need to rattle their cage over this issue.

I also pushed up a change to my logic so rather than first checking for homebrew, it first checks for the standard location of nvm.sh, inside the .nvm dir, before trying homebrew as a fallback.

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

I'm not particularly fussed about whether to include the warning about using homebrew, let's see if we get more reports as we get more people using the script.

. "$NVM_DIR/nvm.sh" --no-use
if [ -f "$NVM_DIR/nvm.sh" ]; then
. "$NVM_DIR/nvm.sh" --no-use
elif [ -n "$(command -v brew)" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Use command_exists here. eg:

elif command_exists "brew"; then

Copy link
Member

Choose a reason for hiding this comment

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

(Side note: command_exists is a helper function we use: https://github.com/WordPress/gutenberg/blob/master/bin/includes.sh#L123-L134)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, fixed

@@ -9,8 +9,12 @@ set -e

# Load NVM
if [ -n "$NVM_DIR" ]; then
# The --no-use option ensures loading NVM doesn't switch the current version.
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for deleting this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User error ;) fixed.

. "$NVM_DIR/nvm.sh" --no-use
elif [ -n "$(command -v brew)" ]; then
# use homebrew that to find path to nvm.sh
. "$(brew --prefix nvm)/nvm.sh" --no-use
Copy link
Member

Choose a reason for hiding this comment

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

Need to check that the file exists before including it: it could've been installed with some other package manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

👍🏻

@pento pento merged commit 1522503 into WordPress:master Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants