MNT: Shellchecking bash scripts

[KaushikMalapati]

Description

Very minor edits that don't change anything important.

I shellchecked the bash scripts in /scripts (making edits based on warnings, errors and info messages that shellcheck gave). Some notes and questions I have are:

• Why is wc needed on line 319 of camviewer and what is this line supposed to do?
• Line 502 of makepeds_psana should be an elif unless xtcav_dark can change XTCAV?
• In line 196 of ioctool, the third if condition seemed incomplete and I tried to fix it by matching the regex to the string in question
• In these three places I arbitrarily chose to follow the rix soure, but I could choose different ones or just ignore the warnings stopdaq: # shellcheck source=/reg/g/pcds/dist/pds/rix/scripts/setup_env.sh restartdaq: # shellcheck source=/reg/g/pcds/dist/pds/rix/scripts/setup_env.sh checkcnf: # shellcheck source=/reg/g/pcds/dist/pds/rix/scripts/setup_env.sh
• I generally opted to use double brackets in if conditions to avoid word splitting warnings, but I could have quoted instead. I don't think we would have any portability concerns about this but I wanted to at least mention it
• I added the r switch to all instances of read to prevent backslash mangling in user input - I did not see a case in any of these scripts where someone would want to do this intentionally (SC2162 for context)
• Some of the most common edits - Double quoting variables, removing extraneous dollar signs in arithmetic expressions, fixing the order of backticks and braces for awk, replacing legacy backticks '' with $() for commands, using -gt instead of >, using grep -c or pgrep instead of grep | wc -l and ps | grep, and checking exit codes directly with if cmd; or if !cmd instead of cmd; if [$?]
• xpp_update_happi_line, grep_pv, ioctool, makepeds_psana, and motor-expert_screen all have warning SC2164 about cd failing. Should this be added to .shellcheckrc to always ignore it? Usually, the directories are sourced, or are constant paths that exist, or are checked explicitly beforehand
• configdb_readxtc, wherepsana, startami, motorInfo, restartdaq, makepeds, and motor-expert-screen (double-dipper) all have warning SC2034 about unused variables. I don't think this warning should be globally suppressed, but I'm also not sure when if the variables are used elsewhere and should exported or if they are no longer being used and can be removed.

I also found a tool called shfmt which we could add a precommit job for. Here is an example of the diff it would print for kinit_helper

-function kauth {
+function kauth
+{
     # If token doesn't exist, create it (will query for password)
-    if ! klist -s
-    then
-       while ! kinit -l 365d -r 365d
-       do
+    if ! klist -s; then
+        while ! kinit -l 365d -r 365d; do
             :
-       done
+        done
     else
-       kinit -R &> /dev/null
+        kinit -R &> /dev/null
     fi
     return 0
 }

-function afsauth {
+function afsauth
+{
     # need to be in g-pcds afs grup
-    if ! pts membership g-pcds 2>&1 | grep -q "$(whoami)"
-    then
+    if ! pts membership g-pcds 2>&1 | grep -q "$(whoami)"; then
         echo "You do not have permission to use afs. See https://confluence.slac.stanford.edu/display/PCDS/Onboarding+Staff+Members"
         return 1
     fi

     # afs should be used from psbuild servers
-    if [[ $(hostname) != psbuild-rhel* ]]
-    then
+    if [[ $(hostname) != psbuild-rhel* ]]; then
         echo "You must be on psbuild to create afs tokens"
-       return 1
+        return 1
     fi

     # If token doesn't exist, create it
-    if ! tokens | grep -q $UID
-    then
+    if ! tokens | grep -q $UID; then
         # afs needs kerberos token
-       kauth
-       aklog
+        kauth
+        aklog
     fi
     return 0
 }

 # if name == '__main__':
-if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then
+if [[ ${BASH_SOURCE[0]} == "${0}" ]]; then
     afsauth
 fi

It basically formats things nicely. I have the indenting set to four spaces right now, but it could be a different number or tabs instead. I haven't formatted anything yet because I don't want to add this unilaterally,

Motivation and Context

182

To standardize style and fix existing shellcheck issues so that future contributors only see pre-commit/workflow errors related to their changes instead of what was already there.

How Has This Been Tested?

Interactively

Where Has This Been Documented?

N/A

[KaushikMalapati]

I will turn this into a real pr once I test it a bit more.

[tangkong]

Is the way to go here adding shellcheck AND shellfmt to precommit?

[KaushikMalapati]

Is the way to go here adding shellcheck AND shellfmt to precommit?

Yes. There's some overlap but shellcheck would mainly be for catching errors and shfmt for consistent styling.

[ZLLentz]

Very minor edits

46 files

First glance looks great but we're going to need to review and test carefully

[ZLLentz]

I think shellfmt and similar sorts of auto-formatting tools are great As long as it doesn't disagree with shellcheck on anything we can include it