Improve code style in raspijamulus.sh #2489

ann0see · 2022-03-10T20:50:54Z

Short description of changes
Codacy complained that quoting is not right in raspijamulus.sh. This PR tries to fix it. I did not test it on a Raspberry Pi, but on my machine it doesn't seem to run.
This shouldn't touch the refactoring by @hoffie

CHANGELOG: Refactor raspijamulus.sh file

Context: Fixes an issue?
Not known.

Does this change need documentation? What needs to be documented and how?

Status of this Pull Request
Waiting for test.

What is missing until this pull request can be merged?
Needs testing on a raspberry pi.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

pljones · 2022-03-11T16:49:51Z

distributions/raspijamulus.sh

-  rm ${OPUS}.tar.gz
-  cd ${OPUS}
+  wget https://archive.mozilla.org/pub/opus/"${OPUS}".tar.gz
+  tar -xzf "${OPUS}".tar.gz


Style note... I'd prefer to see the quoting around the entire parameter, so here "${OPUS}.tar.gz" and later "-j${NCORES}" (alternatively, in this latter case, put a space between -j and its argument if that's valid).

I'm not sure if this ("-j${NCORES}") is valid (I can't really test it either since my laptop is intel based). At least the script fails on my machine.

If -j3 is valid and NCORES=3 then "-j${NCORES}" better be identical to -j3 or there's a bug in the shell.

distributions/raspijamulus.sh

softins

I don't think many of these items are needed. I don't know this Codacy, but it seems to be applying some rules without any knowledge of whether they should be applicable in this particular context.

For variables that have been set literally, one can know they don't contain spaces that are part of a pathname, and extra quoting is either unnecessary or even incorrect. It is only when a variable has been set by user input or globbing for filenames that extra quoting and possibly array syntax are necessary.

distributions/raspijamulus.sh

softins · 2022-03-12T12:23:49Z

distributions/raspijamulus.sh

 # install required packages
 pkgs='alsamixergui build-essential qtbase5-dev qttools5-dev-tools libasound2-dev cmake libglib2.0-dev'
-if ! dpkg -s $pkgs >/dev/null 2>&1; then
+if ! dpkg -s "${pkgs}" >/dev/null 2>&1; then


This doesn't work either. Same comment as on line 13.

Why do we even ask?

softins · 2022-03-12T12:25:02Z

distributions/raspijamulus.sh

+  wget https://archive.mozilla.org/pub/opus/"${OPUS}.tar.gz"
+  tar -xzf "${OPUS}.tar.gz"
+  rm "${OPUS}.tar.gz"
+  cd "${OPUS}"


Again, no need to quote ${OPUS}, because we know what is in it, from line 4

Isn't the point of populating a variable that you don't have to know its value to use it? In which case you should make sure you're using it safely. (And again, if this is a constant, use readonly to declare it.)

softins · 2022-03-12T12:25:33Z

distributions/raspijamulus.sh

  fi
  ./configure --enable-custom-modes --enable-fixed-point
-  make -j${NCORES}
+  make -j"${NCORES}"


And ${NCORES} is just a number. Again, quoting is unnecessary and cumbersome.

I wonder if Codacy understands readonly and then would work out that the expression is safe? Somehow I doubt it.

Using declare -ri NCORES=(whatever) might allow Codacy to realise it's an integer constant, too.

hoffie · 2022-03-12T12:43:01Z

I don't think many of these items are needed. I don't know this Codacy, but it seems to be applying some rules without any knowledge of whether they should be applicable in this particular context.

For variables that have been set literally, one can know they don't contain spaces that are part of a pathname, and extra quoting is either unnecessary or even incorrect. It is only when a variable has been set by user input or globbing for filenames that extra quoting and possibly array syntax are necessary.

In general, I agree that the quoting is optional in many places and quotes may even make it a bit harder to read. At the same time, depending on the script size it becomes harder and harder to keep in mind which things are safe to use without quotes and which are not. I think shellcheck (#2474) suggests doing the same -- quote or write it in a way which makes it obvious that expansion is wanted (i.e. use arrays). Maybe Codacy even uses Shellcheck, I don't know.

So, if we want some kind of static analysis, we either have to accept that everything has to be quoted or we have to sprinkle ignore comments into the scripts.
We'd not be the only ones who go a "quote everything" route (if we do):

https://tldp.org/LDP/abs/html/quotingvar.html

When referencing a variable, it is generally advisable to enclose its name in double quotes.

https://unix.stackexchange.com/questions/68694/when-is-double-quoting-necessary

Second, it is far easier to use double quotes all the time than to remember when they are needed

ann0see · 2022-03-12T20:07:19Z

I think codacy uses spellcheck to detect quoting, so we'd be notified either way.

ann0see · 2022-03-12T21:39:14Z

Please see my new try. I've now been able to test it in a Debian docker container and it seems to run (except for jack obviously). Couldn't check on real Pi yet - but I'll try in a VM.

softins

One comment so far. I'll try it on my Pi in the next day or two before approving.

softins · 2022-03-12T21:46:33Z

distributions/raspijamulus.sh

+  wget https://archive.mozilla.org/pub/opus/"${OPUS}.tar.gz"
+  tar -xzf "${OPUS}.tar.gz"
+  rm "${OPUS}.tar.gz"
+  cd "${OPUS}" || exit


If exiting due to an error, use a non-zero exit status, e.g. exit 1. Similarly below on line 66. Then the caller can tell the script failed, if necessary.

distributions/raspijamulus.sh

hoffie

Code looks good to me (haven't tested on a Pi).

pljones · 2022-03-12T21:58:17Z

distributions/raspijamulus.sh

+# Please run this script from the distributions/ folder
+
+readonly OPUS="opus-1.3.1"
 NCORES=$(nproc)


You can say declare -ri NCORES=$(nproc) to declare NCORES both read-only and containing an integer value.

(Assuming a version of bash from this millenium.)

I did that but spellcheck complained: https://www.shellcheck.net/

Oh dear... What did it complain about?

declare -ri NCORES=$(nproc) ^-- [SC2155](https://github.com/koalaman/shellcheck/wiki/SC2155) (warning): Declare and assign separately to avoid masking return values.

Oh dear, then it is pretty broken. You cannot declare something readonly and then assign to it... It's right about assignment masking the exit code:

~$ ( declare -ri NCORES=$(nproc) || { echo "foo"; exit 1; }; echo $NCORES ) 6 ~$ ( declare -ri NCORES=$(barbaz) || { echo "foo"; exit 1; }; echo $NCORES ) barbaz: command not found 0

Where the second go should have said "foo", not "0". So it should equally not like

readonly NCORES=$(nproc)

on that basis:

$ ( readonly NCORES=$(barbaz) || { echo "foo"; exit 1; }; echo $NCORES ) barbaz: command not found $

(no "foo" echoed). You end up with

tempvarnamehere=$(ncores) || exit 1 declare -ri NCORES="$tempvarnamehere"

which should keep it happy. Otherwise you end up having "...${NCORES}..." everywhere. Which is probably preferable.

ann0see · 2022-03-12T21:59:36Z

The Raspberry Pi VM is probably as slow as a real Raspberry Pi 1st edition, so I'm going to abort it soon. 32 Bit Intel worked (although I had the feeling that the script did "too much").

pljones · 2022-03-12T21:59:54Z

distributions/raspijamulus.sh

+  tar -xzf "${OPUS}.tar.gz"
+  rm "${OPUS}.tar.gz"
+  cd "${OPUS}" || exit 1
  if [ ${OPUS} == "opus-1.3.1" ]; then


"${OPUS}"

pljones · 2022-03-12T22:01:31Z

distributions/raspijamulus.sh

  git checkout v1.9.20
-  ./waf configure --alsa --prefix=/usr/local --libdir=$(pwd)/build
-  ./waf -j${NCORES}
+  ./waf configure --alsa --prefix=/usr/local --libdir="$(pwd)"/build


"--libdir=$(pwd)/build" is nicer (or --libdir="$(pwd)/build" - but splitting the /build off doesn't look as nice.)

pljones · 2022-03-12T22:02:36Z

distributions/raspijamulus.sh

 # start Jack2 and Jamulus in headless mode
 export LD_LIBRARY_PATH="distributions/${OPUS}/.libs:distributions/jack2/build:distributions/jack2/build/common"
-distributions/jack2/build/jackd -R -T --silent -P70 -p16 -t2000 -d alsa -dhw:${ADEVICE} -p 128 -n 3 -r 48000 -s &
+distributions/jack2/build/jackd -R -T --silent -P70 -p16 -t2000 -d alsa -dhw:"${ADEVICE}" -p 128 -n 3 -r 48000 -s &


Again, I'd do "-dhw:${ADEVICE}" if it's a single argument.

ann0see · 2022-03-12T22:17:04Z

Messed a bit with the rebase while trying to move the commits around. It's now all squashed and I hope I didn't loose anything.

hoffie · 2022-03-12T22:25:27Z

Messed a bit with the rebase while trying to move the commits around. It's now all squashed and I hope I didn't loose anything.

The diff between your force push and the state before looks ok to me:

Diff

$ git diff f86d21..8795988
diff --git a/distributions/raspijamulus.sh b/distributions/raspijamulus.sh
index 5dabcfc3..71f777db 100755
--- a/distributions/raspijamulus.sh
+++ b/distributions/raspijamulus.sh
@@ -1,6 +1,8 @@
 #!/bin/bash
 
 # This script is intended to setup a clean Raspberry Pi system for running Jamulus
+# This needs to be run from the distributions/ folder
+
 readonly OPUS="opus-1.3.1"
 NCORES=$(nproc)
 readonly NCORES
@@ -66,7 +68,7 @@ else
   cd jack2 || exit 1
   git checkout v1.9.20
   ./waf configure --alsa --prefix=/usr/local "--libdir=$(pwd)/build"
-  ./waf -j${NCORES}
+  ./waf "-j${NCORES}"
   mkdir build/jack
   cp build/*.so build/jack
   cp build/common/*.so build/jack

softins

Tried it on my Pi4 with a fresh image of Raspbian 11, and it works ok.

I'm not sure anyone other than Volker actually uses the script. I see that after building (if necessary), it automatically starts a headless client, connecting to anygenre3.jamulus.io (which actually ends up on the directory server for Any Genre 1, because no custom port is given), with the username Raspi raspberryp (where the raspberryp part comes from the default hostname of the Pi, truncated).

ann0see · 2022-03-14T19:28:43Z

Yes, I think as soon as we have ARM builds, this script should be (re)-moved

pljones reviewed Mar 11, 2022

View reviewed changes

hoffie requested changes Mar 11, 2022

View reviewed changes

distributions/raspijamulus.sh Outdated Show resolved Hide resolved

softins requested changes Mar 12, 2022

View reviewed changes

ann0see marked this pull request as draft March 12, 2022 20:08

ann0see force-pushed the patch/fixCodeStyleRaspi branch 2 times, most recently from 8c11975 to b88d9f3 Compare March 12, 2022 20:59

ann0see requested review from hoffie and softins March 12, 2022 21:40

softins reviewed Mar 12, 2022

View reviewed changes

hoffie reviewed Mar 12, 2022

View reviewed changes

distributions/raspijamulus.sh Outdated Show resolved Hide resolved

ann0see force-pushed the patch/fixCodeStyleRaspi branch 2 times, most recently from de27a24 to 6c37b8e Compare March 12, 2022 21:56

hoffie approved these changes Mar 12, 2022

View reviewed changes

ann0see marked this pull request as ready for review March 12, 2022 21:57

pljones reviewed Mar 12, 2022

View reviewed changes

ann0see force-pushed the patch/fixCodeStyleRaspi branch from 6c37b8e to f86d21e Compare March 12, 2022 22:08

Improve code style of raspijamulus.sh

8795988

ann0see force-pushed the patch/fixCodeStyleRaspi branch from f86d21e to 8795988 Compare March 12, 2022 22:15

ann0see changed the title ~~Improve quoting for raspijamulus.sh~~ Improve code style in raspijamulus.sh Mar 12, 2022

softins approved these changes Mar 14, 2022

View reviewed changes

ann0see merged commit b452550 into jamulussoftware:master Mar 14, 2022

ann0see deleted the patch/fixCodeStyleRaspi branch March 14, 2022 19:28

pljones added this to the Release 3.9.0 milestone Mar 14, 2022

Improve code style in raspijamulus.sh #2489

Improve code style in raspijamulus.sh #2489

Uh oh!

Conversation

ann0see commented Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

pljones Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

softins left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoffie commented Mar 12, 2022

Uh oh!

ann0see commented Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ann0see commented Mar 12, 2022

Uh oh!

softins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoffie left a comment

Choose a reason for hiding this comment

Uh oh!

pljones Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pljones Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see commented Mar 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pljones Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see commented Mar 10, 2022 •

edited

Loading

pljones Mar 11, 2022 •

edited

Loading

ann0see Mar 11, 2022 •

edited

Loading

softins left a comment •

edited

Loading

ann0see commented Mar 12, 2022 •

edited

Loading

pljones Mar 12, 2022 •

edited

Loading

pljones Mar 13, 2022 •

edited

Loading

pljones Mar 12, 2022 •

edited

Loading