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

Shell linter #3242

Closed
jngrad opened this issue Oct 8, 2019 · 1 comment · Fixed by #3326
Closed

Shell linter #3242

jngrad opened this issue Oct 8, 2019 · 1 comment · Fixed by #3326

Comments

@jngrad
Copy link
Member

jngrad commented Oct 8, 2019

We've recently had a few issues troubleshooting bash scripts that relied on the inherent flexibility of the shell syntax, resulting in silent bugs that went undetected, verbose bugs that took a while to correctly diagnose, and shell scripts that are generally hard to maintain (#3238, #2932, #2998).

Our shell scripts could be made less brittle by following some of the recommendations in the Google Shell Style Guide and using Dash with POSIX syntax in simple scripts that don't require Bash-specific features. The task of modernizing all our shell scripts can be assisted by the shellcheck linter. It can also detect common POSIX violations when using option --shell=dash and looking for rule SC2039; Bash scripts with few to no violations can be candidates for a downgrade to Dash. Below are a few rules that are relevant to the shell scripts in the 4.1 release. shellcheck does not have a whitelisting feature though, which would be helpful to prevent the re-introduction of specific errors in CI.

Dangerous errors:

  • SC2148: Missing shebang
  • SC2061: Quote the parameter to -name, e.g. find . -name *.so to find . -name '*.so'
  • SC2166: Change if [ a -o b ] to if [ a ] || [ b ]: POSIX standard for utilities test and [ is unspecified when called with >4 arguments (test/OPERANDS in IEEE Standard 1003.1)

Readability:

  • SC2116: Useless echo, e.g. f=$(echo "/path/${filename}") to f="/path/${filename}"
  • SC2002: Useless cat, e.g. lines=$(cat a.log | wc -l) to lines=$(wc -l < a.log)
  • SC2006: Legacy subshell syntax, change var=`..` to var=$(..)

Miscellaneous:

  • SC2086: Double quote to prevent globbing and word splitting, e.g. rm $file to rm "$file", unfortunately has many false positives (variables often contain whitespaces on purpose, e.g. list of compiler options) and has a few false negatives (e.g. the bug in maintainer: Escape module python in wrapper script #3238 is not detected)
  • SC2050: conditional with constant expression, unfortunately has many false positives in files with CMake placeholder variables like if [ "@MY_VAR@" = "inplace" ]
@fweik
Copy link
Contributor

fweik commented Oct 14, 2019

I think we should also check if some of the shell scripts can be replaced by python. We have shell scripts with non-trivial logic, e.g. the CI script, for which I think python would be way more maintainable. We should discuss if it wouldn't be a more sustainable solution to replace shell scripts altogether instead putting effort into a linting solution which then has to be maintained on top of the scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants