Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: Shellchecking bash scripts #184

Closed
wants to merge 975 commits into from

Conversation

KaushikMalapati
Copy link
Contributor

@KaushikMalapati KaushikMalapati commented May 6, 2024

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

wnwright and others added 30 commits February 23, 2023 18:07
Fully test the event masks and fix temp file name
Added extra bracket to fix issue with last elif statement
changes to run makepeds also in s3df
add spyder bash script and a line for finding duplicates was added to…
Co-authored-by: Zachary Lentz <ZLLentz@users.noreply.github.com>
ZLLentz and others added 26 commits September 5, 2024 17:46
ENH: add permission handling to ioc-deploy
ENH: check for github early to avoid long timeouts, decrease usage of git clone
ENH: allow in-script tagging in ioc-deploy
DOC: minor docstring errors from renamed cli args
Co-authored-by: Zachary Lentz <ZLLentz@users.noreply.github.com>
@KaushikMalapati KaushikMalapati changed the base branch from master to help-options-style September 29, 2024 20:50
@KaushikMalapati KaushikMalapati removed the request for review from janeliu-slac September 29, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.