Skip to content

Conversation

@Jahorse
Copy link

@Jahorse Jahorse commented Jan 5, 2022

This change to install-gui.sh will skip the attempt to install the npm and nodejs packages through apt-get if the npm and node commands are already available.

Fixes #9727

@jack60612 jack60612 self-requested a review January 9, 2022 21:14
@jack60612 jack60612 self-assigned this Jan 9, 2022
@jack60612 jack60612 added the enhancement New feature or request label Jan 9, 2022
@jack60612
Copy link
Contributor

@Jahorse Could you try fixing the lint issues? Link
The link above should help you setup a linter. If it does not help let me know.

@Jahorse
Copy link
Author

Jahorse commented Jan 11, 2022

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.

@hoffmang9
Copy link
Member

This is functionally equivalent to the one run by SuperLinter and much easier to use:
https://www.shellcheck.net/

@Jahorse
Copy link
Author

Jahorse commented Jan 11, 2022

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.

@Jahorse
Copy link
Author

Jahorse commented Jan 11, 2022

I noticed that the "Test install scripts" job has failed to run install-gui.sh with the following error:

install-gui.sh: 36: Syntax error: "(" unexpected (expecting "fi")

That's a line I've updated, but I cannot replicate the error and it looks like valid bash to me.

@emlowe emlowe requested a review from ChiaMineJP January 26, 2022 20:53
@emlowe
Copy link
Contributor

emlowe commented Jan 26, 2022

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.
I'll leave this open until then
@ChiaMineJP Can you take a look at this, thanks

if ! [ -x "$(command -v node)" ]; then
program_list+=( "nodejs" )
fi
sudo apt-get install -y "${program_list[@]}"
Copy link
Contributor

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.

@ChiaMineJP
Copy link
Contributor

Hello.
As I wrote in this issue , the install scripts was overhauled by another PR.
Much appreciate your work and patience.

@ChiaMineJP ChiaMineJP closed this Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] install-gui.sh failing on Ubuntu 20.04

5 participants