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

Allow to skip NVM installation in setup-local-env.sh #16248

Closed
wants to merge 4 commits into from
Closed

Allow to skip NVM installation in setup-local-env.sh #16248

wants to merge 4 commits into from

Conversation

desaiuditd
Copy link
Member

@desaiuditd desaiuditd commented Jun 23, 2019

Description

Related to #9120.

I'm using nodenv as Node Version Manager. So it's conflicting with the NVM setup script in ./setup-local-env.sh.

This PR introduces a flag option for the setup-local-env.sh script to skip NVM installation and setup, if needed. By default, it will behave as is.

How has this been tested?

  • Running ./bin/setup-local-env.sh --skip-nvm-setup on the local machine will skip all the steps related to NVM, and directly install npm packages.
  • So far, I've only tested this on my Mac environment. I've used getopts which seems to be in-built command in bash. So hopefully, it also works in Windows/Linux. Happy to listen from other OS users.
  • I've also updated the Getting Started doc for dev contributors.

Types of changes

  • Local Environment Setup script.
  • Documentation to let people know about the new flag to skip NVM installation.

Checklist:

  • My code is tested (Only on MacOS so far. Need help in testing this on other OS).
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@desaiuditd
Copy link
Member Author

Reviewers' Note: Hide whitespace changes while reviewing. It will help to understand the new change in better way.

case "${opt}" in
-)
case "${OPTARG}" in
skip-nvm-setup)
Copy link
Member Author

Choose a reason for hiding this comment

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

Open to suggestions on renaming this flag name.

@gziolo gziolo added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement. labels Jun 24, 2019
@gziolo
Copy link
Member

gziolo commented Jun 24, 2019

This change breaks Travis jobs causing them to run forever, see:
https://travis-ci.com/WordPress/gutenberg/builds/116557184

Screen Shot 2019-06-24 at 09 39 44

This was not needed, as each script is sourced explicitly in the current shell. So it will take its own arguments.
This will also fix the Travis CI timeout.
@desaiuditd
Copy link
Member Author

First commit was indeed a genuine mistake from my side and the build was failing because of that.

But last 2 builds doesn't look like relevant failures. In first, an e2e test failed and in second, a unit test failed.

I am not able to conclude a straight forward reason, why they would fail due to my changes, which are essentially targeted towards build tooling and local development.

Can someone have a second look? 👀

@desaiuditd
Copy link
Member Author

Merged latest master into my branch and now tests are passing without any changes from my end. Should be good to review now.

@senadir
Copy link
Contributor

senadir commented Aug 16, 2019

cc @pento does this sound like something we might want to have?
I have concerns about possible problems if we don't have a way to check for the node version used

@youknowriad
Copy link
Contributor

nvm is not really a standard to check or install node versions, I'd personally just remove all nvm related scripts... and just check if the node version is not right, ask the person to install the right one with its preferred method.

@pento
Copy link
Member

pento commented Aug 19, 2019

Yah, NVM was an interesting experiment in trying to ensure the correct version of Node was installed, but I don't think it was a successful as I would've liked.

#17004 will remove all of these scripts, so I'm going to close this PR in favour of that one.

@pento pento closed this Aug 19, 2019
@desaiuditd desaiuditd deleted the add/allow-to-skip-nvm-installation-in-local-env-setup branch August 19, 2019 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Build Tooling Issues or PRs related to build tooling [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants