-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Only install NodeJS on Ubuntu if it's not already available #9728
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
Conversation
|
I was able to use the provided link to setup black, flake8, and mypy, but it looks like the bash linter that runs in the CI job is coming from GitHub's Super Linter, which needs Docker to setup locally, and I don't have it installed. I looked at the linting failures and they looked straightforward to fix, so I tried to just fix them without the linter. If it's still broken I'll get Docker set up and do it properly. |
|
This is functionally equivalent to the one run by SuperLinter and much easier to use: |
|
Oh thanks Hoffman! I ran Shellcheck without my update applied to confirm that it was reporting the issue the same way the CI job did, and then again with the update to confirm that it's now fixed. |
|
I noticed that the "Test install scripts" job has failed to run That's a line I've updated, but I cannot replicate the error and it looks like valid bash to me. |
|
We are making several related changes to the install script. Your PR is very much appreciated but likely will be included in a bigger PR where we fix several other items. |
| if ! [ -x "$(command -v node)" ]; then | ||
| program_list+=( "nodejs" ) | ||
| fi | ||
| sudo apt-get install -y "${program_list[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if nodejs is already installed, sudo apt-get tries to run whatever.
Ideally if nodejs/npm are installed already, apt-get don't need to be run at all.
By the way, we're planning a bunch of updates to the install-gui.sh.
I tried to rebase the upcoming PR onto your branch to express our-thanks but turned out number of conflicts are required to be resolved.
So I've decided to manage the updates to install-gui.sh in #9921
Much appreciate your work and your patience.
|
Hello. |
This change to
install-gui.shwill skip the attempt to install the npm and nodejs packages throughapt-getif thenpmandnodecommands are already available.Fixes #9727