| Message ID | 20211226184919.2753591-1-ricardo.martincoski@gmail.com |
|---|---|
| Headers | show |
| Series | check-package for SysV init scripts (including shellcheck) | expand |
On 26/12/2021 19:49, Ricardo Martincoski wrote: > Hello, > > This small series (called series2 below) is built on top of [1] (called series1) > specially for its patches 1 (check-package unit tests) and 4 (script > utils/docker-run that eases to run locally commands in the docker image in order > to have stable results across machines). > > So: > series1 1/4 utils/checkpackagelib: add unit tests > series1 2/4 support/docker: add python3-pytest > series1 3/4 utils/checkpackagelib: run unit tests on GitLab CI > series1 4/4 utils/docker-run: new script > series2 1/5 utils/check-package: prepare to run external tools > series2 2/5 utils/checkpackagelib: warn about executable files > series2 3/5 utils/checkpackagelib/lib_sysv: check SysV init scripts > series2 4/5 support/docker: add shellcheck > series2 5/5 utils/checkpackagelib/lib_sysv: run shellcheck Series applied to master. > > This series is also related to [2]. > Cc: Joachim Wiberg <troglobit@gmail.com> > See an example output for patch [2]: > |$ utils/docker-run utils/check-package -vvv package/inadyn/S70inadyn > |package/inadyn/S70inadyn:24: should be indented with tabs, see package/busybox/S01syslogd > |< tab >< tab >< tab > -- $INADYN_ARGS > |package/inadyn/S70inadyn:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file > |74 lines processed > |2 warnings generated > > This series is also related to [3]. > Perhaps if both series are accepted, we can add cross-reference between the doc > and the tool: > - check-package can point to a manual entry This I have done. > - the doc can mention to use check-package directly for SysV init script > - the doc can mention to use check-package inside docker image using > utils/docker-run These two I have not done. I've also made a few more changes, and I've put a new docker image to gitlab so it can be used by run-docker. With this tools thing in check-package, it would be nice if we could move flake8 in there as well. I have a post-commit hook that automatically runs check-package, but not flake8, and because of that it often happens that flake8 errors slip by (on this series as well...). Same for shellcheck, it would be nice to run it on all scripts (particularly in utils/ and support/), not just the init start script. Regards, Arnout > > What this series IS NOT: > - a complete rework of all SysV init scripts in the tree; > - an automated way to rework SysV init scripts; > - the ultimate checker for SysV init scripts that catches all possible errors; > - something to enable in GitLab CI in the short term. > > What this series IS (hopefully): > - a helper for developers willing to rework one SysV init script at a time; > - something that can be extended to check more common mistakes when reworking a > SysV init script with package/busybox/S01syslogd as base; > - a few years from now we can eventually enable the check in GitLab CI when all > SysV init script got reworked. > > In [4] one can see an example of all warnings that would be generated for all > scripts in the tree. > NOTICE that no warnings are generated for package/busybox/S01syslogd, as > expected. > At the end see some extracts from [4] with some interesting results (thanks to > shellcheck). > > NOTICE that as a consequence of using shellcheck, there will be cases that will > need shellcheck disables, just like S01syslogd already does: > | # shellcheck disable=SC2086 # we need the word splitting > > [1] http://patchwork.ozlabs.org/project/buildroot/list/?series=275236 > [2] http://patchwork.ozlabs.org/project/buildroot/patch/20211205102907.2836980-3-troglobit@gmail.com/ > [3] http://patchwork.ozlabs.org/project/buildroot/patch/20211205102010.2834942-1-troglobit@gmail.com/ > [4] https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/1921130201 > > Regards, > Ricardo > > Extracts from [4] with some interesting results: > |board/intel/galileo/rootfs_overlay/etc/init.d/S09modload: ignored > | > |package/pigpio/S50pigpio:4: For PIDFILE use the same pattern found in package/busybox/S01syslogd > |PIDFILE="/var/run/pigpio.pid" > |PIDFILE="/var/run/$DAEMON.pid" > | > |package/pigpio/S50pigpio:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd > |expecting S<number><number>pigpiod > | > |package/motion/S99motion:5: Do not include path in DAEMON, see package/busybox/S01syslogd > |DAEMON=/usr/bin/$NAME > |DAEMON="$NAME" > | > |package/motion/S99motion:23: should be indented with tabs, see package/busybox/S01syslogd > | start) > | > |package/motion/S99motion:0: run 'shellcheck' and fix the warnings > |In package/motion/S99motion line 13: > |< tab >printf "Stopping $NAME: " > | ^----------------^ SC2059: Don't use variables in the printf format string. Use printf "..%s.." "$foo". > | > |package/mender/S42mender:0: run 'shellcheck' and fix the warnings > |In package/mender/S42mender line 13: > |< tab > -a "$(readlink /var/lib/mender)" = "/var/run/mender" ] > | ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. > |In package/mender/S42mender line 29: > |< tab >[ $? = 0 ] && echo "OK" || echo "FAIL" > | ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. > | > |package/eudev/S10udev:45: consecutive empty lines > | > |package/mrp/S65mrp:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd > |expecting S<number><number>mrp_server > | > |package/bluez5_utils/S40bluetooth:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd > |expecting S<number><number>bluetoothd > | > |package/bluez5_utils/S40bluetooth:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file > | > |package/netatalk/S50netatalk:33: empty line at end of file > | > |package/iptables/S35iptables:0: run 'shellcheck' and fix the warnings > |In package/iptables/S35iptables line 5: > |IPTABLES_ARGS="" > |^-----------^ SC2034: IPTABLES_ARGS appears unused. Verify use (or export if used externally). > | > |package/rpcbind/S30rpcbind:38: empty line at end of file > | > |In package/dhcp/S80dhcp-relay line 31: > |DHCRELAYPID=/var/run/dhcrelay.pid > |^---------^ SC2034: DHCRELAYPID appears unused. Verify use (or export if used externally). > | > |package/busybox/S10mdev:9: consecutive empty lines > | > |package/busybox/S10mdev:0: run 'shellcheck' and fix the warnings > |In package/busybox/S10mdev line 11: > |< tab >echo -n "Starting $DAEMON... " > | ^-- SC2039: In POSIX sh, echo flags are undefined. > | > |In package/targetcli-fb/S50target line 7: > |< tab >local ret > | ^-------^ SC2039: In POSIX sh, 'local' is undefined. > | > |package/oracle-mysql/S97mysqld:0: run 'shellcheck' and fix the warnings > |In package/oracle-mysql/S97mysqld line 28: > |< tab >< tab >< tab >kill `cat /run/mysql/mysqld.pid` > | ^-------------------------^ SC2046: Quote this to prevent word splitting. > | ^-------------------------^ SC2006: Use $(...) notation instead of legacy backticked `...`. > |Did you mean: > |< tab >< tab >< tab >kill $(cat /run/mysql/mysqld.pid) > | > |package/dbus/S30dbus:0: run 'shellcheck' and fix the warnings > |In package/dbus/S30dbus line 58: > | if [ -f /var/lock/subsys/$servicename ]; then > | ^----------^ SC2154: servicename is referenced but not assigned. > | ^----------^ SC2086: Double quote to prevent globbing and word splitting. > |Did you mean: > | if [ -f /var/lock/subsys/"$servicename" ]; then > | > |package/smcroute/S41smcroute:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file > |package/c-icap/S96cicap:0: run 'shellcheck' and fix the warnings > |In package/c-icap/S96cicap line 12: > |< tab >[ $? == 0 ] && echo "OK" || echo "FAIL" > | ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. > | ^-- SC2039: In POSIX sh, == in place of = is undefined. > | > |package/polkit/S50polkit:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file > | > |package/polkit/S50polkit:0: run 'shellcheck' and fix the warnings > |In package/polkit/S50polkit line 43: > |< tab >start|stop|restart|reload) > | ^----^ SC2221: This pattern always overrides a later one on line 45. > |In package/polkit/S50polkit line 45: > |< tab >reload) > | ^----^ SC2222: This pattern never matches because of a previous pattern on line 43. > | > |package/restorecond/S02restorecond:0: run 'shellcheck' and fix the warnings > |In package/restorecond/S02restorecond line 52: > |< tab >< tab >echo $"Usage: $0 {start|stop|restart|reload}" > | ^-- SC2039: In POSIX sh, $".." is undefined. > | > |package/earlyoom/S02earlyoom:0: run 'shellcheck' and fix the warnings > |In package/earlyoom/S02earlyoom line 10: > |start() { > | ^-- SC1009: The mentioned syntax error was in this brace group. > |In package/earlyoom/S02earlyoom line 11: > |< tab >printf() 'Starting %s: ' "$DAEMON" > | ^-- SC1073: Couldn't parse this function. Fix to allow more checks. > | ^-- SC1064: Expected a { to open the function definition. > | ^-- SC1072: Fix any mentioned problems and try again. > | > |package/watchdogd/S01watchdogd:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file > | > |package/brltty/S10brltty:0: run 'shellcheck' and fix the warnings > |In package/brltty/S10brltty line 17: > |restart() { > |^-- SC2120: restart references arguments, but none are ever passed. > | > |package/audit/S02auditd:0: run 'shellcheck' and fix the warnings > |In package/audit/S02auditd line 13: > |CONFIG=/etc/audit/auditd.conf > |^----^ SC2034: CONFIG appears unused. Verify use (or export if used externally). > | > |package/network-manager/S45network-manager:40: consecutive empty lines > |package/network-manager/S45network-manager:41: consecutive empty lines > |package/network-manager/S45network-manager:41: empty line at end of file > | > |package/tftpd/S80tftpd-hpa:0: run 'shellcheck' and fix the warnings > |In package/tftpd/S80tftpd-hpa line 11: > |PIDFILE=/var/run/$NAME.pid > |^-----^ SC2034: PIDFILE appears unused. Verify use (or export if used externally). > | > |package/owfs/S60owfs:0: run 'shellcheck' and fix the warnings > |In package/owfs/S60owfs line 1: > |NAME="owfs" > |^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang. > | > |package/owfs/S55owserver:0: run 'shellcheck' and fix the warnings > |In package/owfs/S55owserver line 1: > |NAME="owserver" > |^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang. > | > |package/transmission/S92transmission:0: run 'shellcheck' and fix the warnings > |In package/transmission/S92transmission line 50: > |DAEMON=$(which $NAME) > | ^---^ SC2230: which is non-standard. Use builtin 'command -v' instead. > > Ricardo Martincoski (5): > utils/check-package: prepare to run external tools > utils/checkpackagelib: warn about executable files > utils/checkpackagelib/lib_sysv: check SysV init scripts > support/docker: add shellcheck > utils/checkpackagelib/lib_sysv: run shellcheck > > support/docker/Dockerfile | 1 + > utils/check-package | 39 ++++++-- > utils/checkpackagelib/base.py | 11 +++ > utils/checkpackagelib/lib_config.py | 1 + > utils/checkpackagelib/lib_hash.py | 1 + > utils/checkpackagelib/lib_mk.py | 1 + > utils/checkpackagelib/lib_patch.py | 1 + > utils/checkpackagelib/lib_sysv.py | 67 +++++++++++++ > utils/checkpackagelib/test_lib_sysv.py | 131 +++++++++++++++++++++++++ > utils/checkpackagelib/test_tool.py | 112 +++++++++++++++++++++ > utils/checkpackagelib/tool.py | 23 +++++ > 11 files changed, 382 insertions(+), 6 deletions(-) > create mode 100644 utils/checkpackagelib/lib_sysv.py > create mode 100644 utils/checkpackagelib/test_lib_sysv.py > create mode 100644 utils/checkpackagelib/test_tool.py > create mode 100644 utils/checkpackagelib/tool.py >
Hello, This small series (called series2 below) is built on top of [1] (called series1) specially for its patches 1 (check-package unit tests) and 4 (script utils/docker-run that eases to run locally commands in the docker image in order to have stable results across machines). So: series1 1/4 utils/checkpackagelib: add unit tests series1 2/4 support/docker: add python3-pytest series1 3/4 utils/checkpackagelib: run unit tests on GitLab CI series1 4/4 utils/docker-run: new script series2 1/5 utils/check-package: prepare to run external tools series2 2/5 utils/checkpackagelib: warn about executable files series2 3/5 utils/checkpackagelib/lib_sysv: check SysV init scripts series2 4/5 support/docker: add shellcheck series2 5/5 utils/checkpackagelib/lib_sysv: run shellcheck This series is also related to [2]. Cc: Joachim Wiberg <troglobit@gmail.com> See an example output for patch [2]: |$ utils/docker-run utils/check-package -vvv package/inadyn/S70inadyn |package/inadyn/S70inadyn:24: should be indented with tabs, see package/busybox/S01syslogd |< tab >< tab >< tab > -- $INADYN_ARGS |package/inadyn/S70inadyn:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file |74 lines processed |2 warnings generated This series is also related to [3]. Perhaps if both series are accepted, we can add cross-reference between the doc and the tool: - check-package can point to a manual entry - the doc can mention to use check-package directly for SysV init script - the doc can mention to use check-package inside docker image using utils/docker-run What this series IS NOT: - a complete rework of all SysV init scripts in the tree; - an automated way to rework SysV init scripts; - the ultimate checker for SysV init scripts that catches all possible errors; - something to enable in GitLab CI in the short term. What this series IS (hopefully): - a helper for developers willing to rework one SysV init script at a time; - something that can be extended to check more common mistakes when reworking a SysV init script with package/busybox/S01syslogd as base; - a few years from now we can eventually enable the check in GitLab CI when all SysV init script got reworked. In [4] one can see an example of all warnings that would be generated for all scripts in the tree. NOTICE that no warnings are generated for package/busybox/S01syslogd, as expected. At the end see some extracts from [4] with some interesting results (thanks to shellcheck). NOTICE that as a consequence of using shellcheck, there will be cases that will need shellcheck disables, just like S01syslogd already does: | # shellcheck disable=SC2086 # we need the word splitting [1] http://patchwork.ozlabs.org/project/buildroot/list/?series=275236 [2] http://patchwork.ozlabs.org/project/buildroot/patch/20211205102907.2836980-3-troglobit@gmail.com/ [3] http://patchwork.ozlabs.org/project/buildroot/patch/20211205102010.2834942-1-troglobit@gmail.com/ [4] https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/1921130201 Regards, Ricardo Extracts from [4] with some interesting results: |board/intel/galileo/rootfs_overlay/etc/init.d/S09modload: ignored | |package/pigpio/S50pigpio:4: For PIDFILE use the same pattern found in package/busybox/S01syslogd |PIDFILE="/var/run/pigpio.pid" |PIDFILE="/var/run/$DAEMON.pid" | |package/pigpio/S50pigpio:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd |expecting S<number><number>pigpiod | |package/motion/S99motion:5: Do not include path in DAEMON, see package/busybox/S01syslogd |DAEMON=/usr/bin/$NAME |DAEMON="$NAME" | |package/motion/S99motion:23: should be indented with tabs, see package/busybox/S01syslogd | start) | |package/motion/S99motion:0: run 'shellcheck' and fix the warnings |In package/motion/S99motion line 13: |< tab >printf "Stopping $NAME: " | ^----------------^ SC2059: Don't use variables in the printf format string. Use printf "..%s.." "$foo". | |package/mender/S42mender:0: run 'shellcheck' and fix the warnings |In package/mender/S42mender line 13: |< tab > -a "$(readlink /var/lib/mender)" = "/var/run/mender" ] | ^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. |In package/mender/S42mender line 29: |< tab >[ $? = 0 ] && echo "OK" || echo "FAIL" | ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. | |package/eudev/S10udev:45: consecutive empty lines | |package/mrp/S65mrp:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd |expecting S<number><number>mrp_server | |package/bluez5_utils/S40bluetooth:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd |expecting S<number><number>bluetoothd | |package/bluez5_utils/S40bluetooth:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file | |package/netatalk/S50netatalk:33: empty line at end of file | |package/iptables/S35iptables:0: run 'shellcheck' and fix the warnings |In package/iptables/S35iptables line 5: |IPTABLES_ARGS="" |^-----------^ SC2034: IPTABLES_ARGS appears unused. Verify use (or export if used externally). | |package/rpcbind/S30rpcbind:38: empty line at end of file | |In package/dhcp/S80dhcp-relay line 31: |DHCRELAYPID=/var/run/dhcrelay.pid |^---------^ SC2034: DHCRELAYPID appears unused. Verify use (or export if used externally). | |package/busybox/S10mdev:9: consecutive empty lines | |package/busybox/S10mdev:0: run 'shellcheck' and fix the warnings |In package/busybox/S10mdev line 11: |< tab >echo -n "Starting $DAEMON... " | ^-- SC2039: In POSIX sh, echo flags are undefined. | |In package/targetcli-fb/S50target line 7: |< tab >local ret | ^-------^ SC2039: In POSIX sh, 'local' is undefined. | |package/oracle-mysql/S97mysqld:0: run 'shellcheck' and fix the warnings |In package/oracle-mysql/S97mysqld line 28: |< tab >< tab >< tab >kill `cat /run/mysql/mysqld.pid` | ^-------------------------^ SC2046: Quote this to prevent word splitting. | ^-------------------------^ SC2006: Use $(...) notation instead of legacy backticked `...`. |Did you mean: |< tab >< tab >< tab >kill $(cat /run/mysql/mysqld.pid) | |package/dbus/S30dbus:0: run 'shellcheck' and fix the warnings |In package/dbus/S30dbus line 58: | if [ -f /var/lock/subsys/$servicename ]; then | ^----------^ SC2154: servicename is referenced but not assigned. | ^----------^ SC2086: Double quote to prevent globbing and word splitting. |Did you mean: | if [ -f /var/lock/subsys/"$servicename" ]; then | |package/smcroute/S41smcroute:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file |package/c-icap/S96cicap:0: run 'shellcheck' and fix the warnings |In package/c-icap/S96cicap line 12: |< tab >[ $? == 0 ] && echo "OK" || echo "FAIL" | ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. | ^-- SC2039: In POSIX sh, == in place of = is undefined. | |package/polkit/S50polkit:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file | |package/polkit/S50polkit:0: run 'shellcheck' and fix the warnings |In package/polkit/S50polkit line 43: |< tab >start|stop|restart|reload) | ^----^ SC2221: This pattern always overrides a later one on line 45. |In package/polkit/S50polkit line 45: |< tab >reload) | ^----^ SC2222: This pattern never matches because of a previous pattern on line 43. | |package/restorecond/S02restorecond:0: run 'shellcheck' and fix the warnings |In package/restorecond/S02restorecond line 52: |< tab >< tab >echo $"Usage: $0 {start|stop|restart|reload}" | ^-- SC2039: In POSIX sh, $".." is undefined. | |package/earlyoom/S02earlyoom:0: run 'shellcheck' and fix the warnings |In package/earlyoom/S02earlyoom line 10: |start() { | ^-- SC1009: The mentioned syntax error was in this brace group. |In package/earlyoom/S02earlyoom line 11: |< tab >printf() 'Starting %s: ' "$DAEMON" | ^-- SC1073: Couldn't parse this function. Fix to allow more checks. | ^-- SC1064: Expected a { to open the function definition. | ^-- SC1072: Fix any mentioned problems and try again. | |package/watchdogd/S01watchdogd:0: This file does not need to be executable, just make sure you use '$(INSTALL) -D -m 0755' in the .mk file | |package/brltty/S10brltty:0: run 'shellcheck' and fix the warnings |In package/brltty/S10brltty line 17: |restart() { |^-- SC2120: restart references arguments, but none are ever passed. | |package/audit/S02auditd:0: run 'shellcheck' and fix the warnings |In package/audit/S02auditd line 13: |CONFIG=/etc/audit/auditd.conf |^----^ SC2034: CONFIG appears unused. Verify use (or export if used externally). | |package/network-manager/S45network-manager:40: consecutive empty lines |package/network-manager/S45network-manager:41: consecutive empty lines |package/network-manager/S45network-manager:41: empty line at end of file | |package/tftpd/S80tftpd-hpa:0: run 'shellcheck' and fix the warnings |In package/tftpd/S80tftpd-hpa line 11: |PIDFILE=/var/run/$NAME.pid |^-----^ SC2034: PIDFILE appears unused. Verify use (or export if used externally). | |package/owfs/S60owfs:0: run 'shellcheck' and fix the warnings |In package/owfs/S60owfs line 1: |NAME="owfs" |^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang. | |package/owfs/S55owserver:0: run 'shellcheck' and fix the warnings |In package/owfs/S55owserver line 1: |NAME="owserver" |^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang. | |package/transmission/S92transmission:0: run 'shellcheck' and fix the warnings |In package/transmission/S92transmission line 50: |DAEMON=$(which $NAME) | ^---^ SC2230: which is non-standard. Use builtin 'command -v' instead. Ricardo Martincoski (5): utils/check-package: prepare to run external tools utils/checkpackagelib: warn about executable files utils/checkpackagelib/lib_sysv: check SysV init scripts support/docker: add shellcheck utils/checkpackagelib/lib_sysv: run shellcheck support/docker/Dockerfile | 1 + utils/check-package | 39 ++++++-- utils/checkpackagelib/base.py | 11 +++ utils/checkpackagelib/lib_config.py | 1 + utils/checkpackagelib/lib_hash.py | 1 + utils/checkpackagelib/lib_mk.py | 1 + utils/checkpackagelib/lib_patch.py | 1 + utils/checkpackagelib/lib_sysv.py | 67 +++++++++++++ utils/checkpackagelib/test_lib_sysv.py | 131 +++++++++++++++++++++++++ utils/checkpackagelib/test_tool.py | 112 +++++++++++++++++++++ utils/checkpackagelib/tool.py | 23 +++++ 11 files changed, 382 insertions(+), 6 deletions(-) create mode 100644 utils/checkpackagelib/lib_sysv.py create mode 100644 utils/checkpackagelib/test_lib_sysv.py create mode 100644 utils/checkpackagelib/test_tool.py create mode 100644 utils/checkpackagelib/tool.py