Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

Shellcheck dev_setup.sh #3048

Closed
wants to merge 11 commits into from
Closed

Shellcheck dev_setup.sh #3048

wants to merge 11 commits into from

Conversation

krisgesling
Copy link
Contributor

@krisgesling krisgesling commented Dec 8, 2021

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?

  • CLA

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Dec 8, 2021
@krisgesling
Copy link
Contributor Author

Also feel free to push additional fixes straight onto this branch

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator

forslund commented Dec 8, 2021

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
Copy link
Contributor

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
Copy link

Codecov Report

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

❗ 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.

forslund and others added 11 commits December 9, 2021 08:59
This replaces save_*_path with usage of the xdg_*_home when handling
config files. This means the config folders will not be created unless
actually written to.

The check for whether a directory needs to be created is handled behind
a lock to avoid race conditions
read will mangle backslashes without it. These are not expected
characters so not explicitly necessary, but also won't hurt.
Simplify sed-expression to be a single line only removing the need for
the offending / and linefeed
Splits the check into to files to be able to quote the expressions
in a good way.
This converts the variable APT_PACKAGE_LIST from a string to an array.
This is a safer way to handle arguments according to BashFAQ
(http://mywiki.wooledge.org/BashFAQ/050)
@krisgesling krisgesling force-pushed the bugfix/shellcheck-dev-setup branch 2 times, most recently from feecf13 to 9355bbe Compare December 8, 2021 23:41
@krisgesling
Copy link
Contributor Author

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 krisgesling added the Status: Work in progress PR being actively worked on, not yet ready for review. label Dec 8, 2021
@krisgesling
Copy link
Contributor Author

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

@forslund
Copy link
Collaborator

forslund commented Dec 9, 2021

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
Copy link
Contributor Author

Closing in favor of #3090

@krisgesling krisgesling closed this Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Status: Work in progress PR being actively worked on, not yet ready for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants