ci: add Shellcheck workflow #569
Conversation
|
the new shellcheck workflow is failing. I need to fix more scripts. |
autogen.sh
Outdated
| set -x | ||
|
|
||
| cd $srcdir | ||
| cd "$srcdir" || exit |
There was a problem hiding this comment.
i'd argue that just exiting without any error message or exit code is worse than running into the aclocal error that would happen when this fails.
There was a problem hiding this comment.
@rofl0r wrote:
i'd argue that just exiting without any error message or exit code is worse than running into the aclocal error that would happen when this fails.
Agreed. I changed to have a message along with exit.
|
is there any specific reason you're pursuing this? |
|
@rofl0r wrote:
I simply wanted to add shellcheck to the CI. Without breaking ci right away, it requires adjusting some scripts. i think i never used autogen.sh, instead always using "autoreconf -i". `autogen.sh is at least used in ci workflows. I have been using it as well. as for adding double quotes everywhere my personal opinion is that nobody should use spaces in filenames/paths, and if he/she does, the punishment is just... ;) |
obnoxxx
force-pushed
the
shellcheck
branch
2 times, most recently
from
9d87141 to
d79ce30
Compare
tests/scripts/run_tests.sh
Outdated
| @@ -1,4 +1,4 @@ | |||
| #!/bin/sh | |||
| #!/bin/env bash | |||
There was a problem hiding this comment.
no particular reason.
iirc, it seemed to fix something.
Now changing it back to sh.
tests/scripts/run_tests.sh
Outdated
| kill $pid | ||
| if test "x$?" = "x0" ; then | ||
| printf "killing tinyproxy..." | ||
| local pid |
There was a problem hiding this comment.
fwiw, even dash supports local
There was a problem hiding this comment.
yeah, I wasn't removing local. just changing the way it is used. shellcheck said to separate declaration from assignment.
obnoxxx
force-pushed
the
shellcheck
branch
2 times, most recently
from
a3ac5d8 to
5c53187
Compare
The following error types are addressed: https://www.shellcheck.net/wiki/SC2164 -- Use 'cd ... || exit' or 'cd ... || return' in case cd fails. https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing and word splitting. https://www.shellcheck.net/wiki/SC2006 -- Use $(...) notation instead of legacy backticks `...`. Signed-off-by: Michael Adam <obnox@samba.org>
|
I have changed the PR to introduce a I have also split the shellcheck fixes into multiple commits, making sure at each step that the scripts still work as expected (in particular |
obnoxxx
force-pushed
the
shellcheck
branch
2 times, most recently
from
279ba85 to
5c3abd5
Compare
|
congrats on getting it working. |
|
@rofl0r wrote:
👍🏼 does the github workflow still enforce the shellcheck to succeed ? yes, CI would fail if future PRs introduced shellcheck issue. The main point with the new PR verion is thatthe new
While I am generally in favor of atomic commits, I do agree in this case that there is no value in having multiple commits per file for fixing different shellcheck issues. I plan to push an update later today with the following changes:
|
This fixes several instances of the following shellcheck issues: https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitting. https://shellcheck.net/wiki/SC3037 (warning): In POSIX sh, echo flags are undefined. https://shellchelleck.net/wiki/SC2268 (style): Avoid x-prefix in comparisons as it no longer serves a purpose. https://shellcheck.net/wiki/SC2009 (info): Consider using pgrep instead of grepping ps output. https://shellcheck.net/wiki/SC3028 (warning): In POSIX sh, SECONDS is undefined. SC2059 (info): Don't use variables in the printf format string COUNT appears unused. Verify use (or export if used externally). https://shellcheck.net/wiki/SC2162 (info): read without -r will mangle backslashes. https://shellcheck.net/wiki/SC2034 (warning): READ appears unused. Verify use (or export if used externally). https://shellcheck.net/wiki/SC2317 (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly). https://shellcheck.net/wiki/SC2086 (info): Double quote to prevent globbing and word splitting. Signed-off-by: Michael Adam <obnox@samba.org> tests: fix syntax errors in run_tests.sh Signed-off-by: Michael Adam <obnox@samba.org>
This fixes instances of: https://shellcheck.net/wiki/SC2086 (info): Double quote to prevent globbing and word splitting. https://shellcheck.net/wiki/SC2034 (warning): BASEDIR appears unused. Signed-off-by: Michael Adam <obnox@samba.org>
This can be used to lint shell scripts for syntactic correctness and style. It requires shellcheck to be installed on the host. Signed-off-by: Michael Adam <obnox@samba.org>
Signed-off-by: Michael Adam <obnox@samba.org>
Signed-off-by: Michael Adam <obnox@samba.org>
2 participants
This adds shellcheck linting to the CI.
The purpose of this is to make sure the shell scripts in this repo don't have
syntactic or style issues known to shellcheck and to prevent future PRs from introducing regressions.
It additionally fixes issues in existing scripts while at it to prevent this from breaking the CI right away.