MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.49k stars 1.27k forks source link

Shellcheck dev_setup.sh #3048

Closed krisgesling closed 2 years ago

krisgesling commented 2 years ago

Description

Hit an issue in a Github Actions install of Mycroft-core where it was unable to evaluate a conditional. This fixes that specific issue along with a range of suggestions from shellcheck.

How to test

run shellcheck dev_setup.sh install mycroft using dev_setup.sh

Contributor license agreement signed?

krisgesling commented 2 years ago

Also feel free to push additional fixes straight onto this branch

devops-mycroft commented 2 years ago

Voight Kampff Integration Test Succeeded (Results)

forslund commented 2 years ago

In reverse order: 4: The sed expression doesn't need to be spread out on three lines, with a single line this becomes nice and easy 3: Not sure what the best way is but it sounds like command -v should be the way to go 2: These seem to be due to using space separated string as commandline arguments, BashFAQ disapproves of this and recommends an array instead. 1: Easiest and nicest solution is in my opinion to split it into 2 lines

I pushed commits for 4,2 and 1. I suck at shellscripting so another pair of eyes on those changes might be a good idea.

3 seemed simple but my shellcheck didn't give me an error on that one so it might be best to be taken care of by someone seeing the warning. Edit: after @PureTryOut confirming this to be the best way I changed it to the suggested command -v

PureTryOut commented 2 years ago

which is indeed not standard. Debian is even going to drop it soon, there was quite a lively discussion about that recently. command -v is the only standard that can be relied upon to work the same across all distros and shells.

codecov-commenter commented 2 years ago

Codecov Report

Merging #3048 (8c32745) into dev (ae99974) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 8c32745 differs from pull request most recent head 55bc950. Consider uploading reports for the commit 55bc950 to get more accurate results Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #3048   +/-   ##
=======================================
  Coverage   53.11%   53.11%           
=======================================
  Files         123      123           
  Lines       11188    11188           
=======================================
  Hits         5942     5942           
  Misses       5246     5246           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ae99974...55bc950. Read the comment docs.

krisgesling commented 2 years ago

hmm doesn't look like you can add an individual file from outside the included directory. So I've dropped that commit for now.

I'll try find some time to do a clean up of the rest of the scripts since most of it is quite straight forward.

krisgesling commented 2 years ago

:facepalm: did a silly on the commit history - will fix it later today

forslund commented 2 years ago

May be worth looking at #3030 before doing the rest of the scripts. It adds some shellcheck setup which may be useful. I've been planning to go through the scripts folder but have been waiting for that PR to be reviewed.

krisgesling commented 2 years ago

Closing in favor of #3090