[Buildroot] [PATCH 0/5] check-package for SysV init scripts (including shellcheck)

[Buildroot] [PATCH 0/5] check-package for SysV init scripts (including shellcheck)

[Ricardo Martincoski]

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

-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 1/5] utils/check-package: prepare to run external tools

[Ricardo Martincoski]

Some file formats have well-established syntax checkers.
One example of this is the tool 'shellcheck' that can analyse shell
scripts for common mistakes.

There is no reason to reimplement such tools in check-package, when we
can just call them.

Add the ability to check-package to call external tools that will run
once for each file to be analysed.
For simplicity, when the tool generated one or more warnings, count it
as a single warning from check-package, that can display something like
this:

|$ ./utils/check-package  package/unscd/S46unscd
|package/unscd/S46unscd:0: run 'shellcheck' and fix the warnings
|25 lines processed
|1 warnings generated

|$ ./utils/check-package -vvvvvvvvvvvvvvvv package/unscd/S46unscd
|package/unscd/S46unscd:0: run 'shellcheck' and fix the warnings
|In package/unscd/S46unscd line 9:
|        printf "Starting ${NAME}: "
|               ^------------------^ SC2059: Don't use variables in the printf format string. Use printf "..%s.." "$foo".
|In package/unscd/S46unscd line 11:
|        [ $? -eq 0 ] && echo "OK" || echo "FAIL"
|          ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
|In package/unscd/S46unscd line 14:
|        printf "Stopping ${NAME}: "
|               ^------------------^ SC2059: Don't use variables in the printf format string. Use printf "..%s.." "$foo".
|In package/unscd/S46unscd line 16:
|        [ $? -eq 0 ] && echo "OK" || echo "FAIL"
|          ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
|For more information:
|  https://www.shellcheck.net/wiki/SC2059 -- Don't use variables in the printf...
|  https://www.shellcheck.net/wiki/SC2181 -- Check exit code directly with e.g...
|25 lines processed
|1 warnings generated

In this first commit, add only the ability for check-package to call
external tools and not an example of such tool, as adding each tool to
call may need update to the docker image and can lead to it's own
discussion on how to implement.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Please read the cover letter before applying.
---
 utils/check-package           | 34 ++++++++++++++++++++++++++++------
 utils/checkpackagelib/base.py |  8 ++++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/utils/check-package b/utils/check-package
index a959fef079..5fb430902d 100755
--- a/utils/check-package
+++ b/utils/check-package
@@ -8,6 +8,7 @@ import re
 import six
 import sys
 
+import checkpackagelib.base
 import checkpackagelib.lib_config
 import checkpackagelib.lib_hash
 import checkpackagelib.lib_mk
@@ -87,9 +88,7 @@ def get_lib_from_filename(fname):
     return None
 
 
-def is_a_check_function(m):
-    if not inspect.isclass(m):
-        return False
+def common_inspect_rules(m):
     # do not call the base class
     if m.__name__.startswith("_"):
         return False
@@ -100,6 +99,22 @@ def is_a_check_function(m):
     return True
 
 
+def is_a_check_function(m):
+    if not inspect.isclass(m):
+        return False
+    if not issubclass(m, checkpackagelib.base._CheckFunction):
+        return False
+    return common_inspect_rules(m)
+
+
+def is_external_tool(m):
+    if not inspect.isclass(m):
+        return False
+    if not issubclass(m, checkpackagelib.base._Tool):
+        return False
+    return common_inspect_rules(m)
+
+
 def print_warnings(warnings):
     # Avoid the need to use 'return []' at the end of every check function.
     if warnings is None:
@@ -121,14 +136,16 @@ def check_file_using_lib(fname):
         if flags.verbose >= VERBOSE_LEVEL_TO_SHOW_IGNORED_FILES:
             print("{}: ignored".format(fname))
         return nwarnings, nlines
-    classes = inspect.getmembers(lib, is_a_check_function)
+    internal_functions = inspect.getmembers(lib, is_a_check_function)
+    external_tools = inspect.getmembers(lib, is_external_tool)
+    all_checks = internal_functions + external_tools
 
     if flags.dry_run:
-        functions_to_run = [c[0] for c in classes]
+        functions_to_run = [c[0] for c in all_checks]
         print("{}: would run: {}".format(fname, functions_to_run))
         return nwarnings, nlines
 
-    objects = [c[1](fname, flags.manual_url) for c in classes]
+    objects = [c[1](fname, flags.manual_url) for c in internal_functions]
 
     for cf in objects:
         nwarnings += print_warnings(cf.before())
@@ -148,6 +165,11 @@ def check_file_using_lib(fname):
     for cf in objects:
         nwarnings += print_warnings(cf.after())
 
+    tools = [c[1](fname) for c in external_tools]
+
+    for tool in tools:
+        nwarnings += print_warnings(tool.run())
+
     return nwarnings, nlines
 
 
diff --git a/utils/checkpackagelib/base.py b/utils/checkpackagelib/base.py
index 9544a64e5a..73da925a03 100644
--- a/utils/checkpackagelib/base.py
+++ b/utils/checkpackagelib/base.py
@@ -16,3 +16,11 @@ class _CheckFunction(object):
 
     def after(self):
         pass
+
+
+class _Tool(object):
+    def __init__(self, filename):
+        self.filename = filename
+
+    def run(self):
+        pass
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 2/5] utils/checkpackagelib: warn about executable files

[Ricardo Martincoski]

Currently there are no .mk, Config.in, .patch or .hash files with
executable permissions in the tree.
But we don't want to have that.

So warn when a file checked by check-package has executable permission.

This check will be reused when testing SysV init scripts in the tree.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Please read the cover letter before applying.
---
 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/test_tool.py  | 41 +++++++++++++++++++++++++++++
 utils/checkpackagelib/tool.py       |  8 ++++++
 6 files changed, 53 insertions(+)
 create mode 100644 utils/checkpackagelib/test_tool.py
 create mode 100644 utils/checkpackagelib/tool.py

diff --git a/utils/checkpackagelib/lib_config.py b/utils/checkpackagelib/lib_config.py
index c348eec399..b05831f2c3 100644
--- a/utils/checkpackagelib/lib_config.py
+++ b/utils/checkpackagelib/lib_config.py
@@ -10,6 +10,7 @@ from checkpackagelib.lib import ConsecutiveEmptyLines  # noqa: F401
 from checkpackagelib.lib import EmptyLastLine          # noqa: F401
 from checkpackagelib.lib import NewlineAtEof           # noqa: F401
 from checkpackagelib.lib import TrailingSpace          # noqa: F401
+from checkpackagelib.tool import NotExecutable         # noqa: F401
 
 
 def _empty_or_comment(text):
diff --git a/utils/checkpackagelib/lib_hash.py b/utils/checkpackagelib/lib_hash.py
index 3e381119a5..c67046e16d 100644
--- a/utils/checkpackagelib/lib_hash.py
+++ b/utils/checkpackagelib/lib_hash.py
@@ -10,6 +10,7 @@ from checkpackagelib.lib import ConsecutiveEmptyLines  # noqa: F401
 from checkpackagelib.lib import EmptyLastLine          # noqa: F401
 from checkpackagelib.lib import NewlineAtEof           # noqa: F401
 from checkpackagelib.lib import TrailingSpace          # noqa: F401
+from checkpackagelib.tool import NotExecutable         # noqa: F401
 
 
 def _empty_line_or_comment(text):
diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
index 88e4b15c7c..153754b6f1 100644
--- a/utils/checkpackagelib/lib_mk.py
+++ b/utils/checkpackagelib/lib_mk.py
@@ -13,6 +13,7 @@ from checkpackagelib.lib import EmptyLastLine          # noqa: F401
 from checkpackagelib.lib import NewlineAtEof           # noqa: F401
 from checkpackagelib.lib import TrailingSpace          # noqa: F401
 from checkpackagelib.lib import Utf8Characters         # noqa: F401
+from checkpackagelib.tool import NotExecutable         # noqa: F401
 
 # used in more than one check
 start_conditional = ["ifdef", "ifeq", "ifndef", "ifneq"]
diff --git a/utils/checkpackagelib/lib_patch.py b/utils/checkpackagelib/lib_patch.py
index e4e914b7f0..caee36158f 100644
--- a/utils/checkpackagelib/lib_patch.py
+++ b/utils/checkpackagelib/lib_patch.py
@@ -8,6 +8,7 @@ import re
 
 from checkpackagelib.base import _CheckFunction
 from checkpackagelib.lib import NewlineAtEof           # noqa: F401
+from checkpackagelib.tool import NotExecutable         # noqa: F401
 
 
 class ApplyOrder(_CheckFunction):
diff --git a/utils/checkpackagelib/test_tool.py b/utils/checkpackagelib/test_tool.py
new file mode 100644
index 0000000000..e9e0838103
--- /dev/null
+++ b/utils/checkpackagelib/test_tool.py
@@ -0,0 +1,41 @@
+import os
+import pytest
+import re
+import tempfile
+import checkpackagelib.tool as m
+
+workdir = os.path.join(tempfile.mkdtemp(suffix='-checkpackagelib-test-tool'))
+workdir_regex = re.compile(r'/tmp/tmp[^/]*-checkpackagelib-test-tool')
+
+
+def check_file(tool, filename, string, permissions=None):
+    script = os.path.join(workdir, filename)
+    with open(script, 'wb') as f:
+        f.write(string.encode())
+    if permissions:
+        os.chmod(script, permissions)
+    obj = tool(script)
+    result = obj.run()
+    if result is None:
+        return []
+    return [workdir_regex.sub('dir', r) for r in result]
+
+
+NotExecutable = [
+    ('664',
+     'package.mk',
+     0o664,
+     '',
+     []),
+    ('775',
+     'package.mk',
+     0o775,
+     '',
+     ["dir/package.mk:0: This file does not need to be executable"]),
+    ]
+
+
+@pytest.mark.parametrize('testname,filename,permissions,string,expected', NotExecutable)
+def test_NotExecutable(testname, filename, permissions, string, expected):
+    warnings = check_file(m.NotExecutable, filename, string, permissions)
+    assert warnings == expected
diff --git a/utils/checkpackagelib/tool.py b/utils/checkpackagelib/tool.py
new file mode 100644
index 0000000000..f2007be1ff
--- /dev/null
+++ b/utils/checkpackagelib/tool.py
@@ -0,0 +1,8 @@
+import os
+from checkpackagelib.base import _Tool
+
+
+class NotExecutable(_Tool):
+    def run(self):
+        if os.access(self.filename, os.X_OK):
+            return ["{}:0: This file does not need to be executable".format(self.filename)]
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 3/5] utils/checkpackagelib/lib_sysv: check SysV init scripts

[Ricardo Martincoski]

Enable the common checks:
 - consecutive empty lines
 - empty last line
 - missing new line at end of file
 - trailing space
 - warn for executable files, with the hint to instead use
   '$(INSTALL) -D -m 0755' in the .mk file

Check indent with tabs:
 - add a simple check function to warn only when the indent is done
   using spaces or a mix of tabs and spaces. It does not check indenting
   levels, but it already makes the review easier, since it
   diferentiates spaces and tabs.

Check variables:
 - check DAEMON is defined
 - when DAEMON is defined, check the filename is in the form S01daemon
 - when PIDFILE is defined, expect it to be in /var/run and defined
   using $DAEMON.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Please read the cover letter before applying.
---
 utils/check-package                    |   5 +
 utils/checkpackagelib/base.py          |   3 +
 utils/checkpackagelib/lib_sysv.py      |  66 +++++++++++++
 utils/checkpackagelib/test_lib_sysv.py | 131 +++++++++++++++++++++++++
 utils/checkpackagelib/test_tool.py     |  25 +++++
 utils/checkpackagelib/tool.py          |   2 +-
 6 files changed, 231 insertions(+), 1 deletion(-)
 create mode 100644 utils/checkpackagelib/lib_sysv.py
 create mode 100644 utils/checkpackagelib/test_lib_sysv.py

diff --git a/utils/check-package b/utils/check-package
index 5fb430902d..f64daed84c 100755
--- a/utils/check-package
+++ b/utils/check-package
@@ -13,6 +13,7 @@ import checkpackagelib.lib_config
 import checkpackagelib.lib_hash
 import checkpackagelib.lib_mk
 import checkpackagelib.lib_patch
+import checkpackagelib.lib_sysv
 
 VERBOSE_LEVEL_TO_SHOW_IGNORED_FILES = 3
 flags = None  # Command line arguments.
@@ -66,6 +67,8 @@ DO_NOT_CHECK_INTREE = re.compile(r"|".join([
     r"toolchain/toolchain-external/pkg-toolchain-external\.mk$",
     ]))
 
+SYSV_INIT_SCRIPT_FILENAME = re.compile(r"/S\d\d[^/]+$")
+
 
 def get_lib_from_filename(fname):
     if flags.intree_only:
@@ -85,6 +88,8 @@ def get_lib_from_filename(fname):
         return checkpackagelib.lib_mk
     if fname.endswith(".patch"):
         return checkpackagelib.lib_patch
+    if SYSV_INIT_SCRIPT_FILENAME.search(fname):
+        return checkpackagelib.lib_sysv
     return None
 
 
diff --git a/utils/checkpackagelib/base.py b/utils/checkpackagelib/base.py
index 73da925a03..f666e4110b 100644
--- a/utils/checkpackagelib/base.py
+++ b/utils/checkpackagelib/base.py
@@ -24,3 +24,6 @@ class _Tool(object):
 
     def run(self):
         pass
+
+    def hint(self):
+        return ""
diff --git a/utils/checkpackagelib/lib_sysv.py b/utils/checkpackagelib/lib_sysv.py
new file mode 100644
index 0000000000..77e6283b25
--- /dev/null
+++ b/utils/checkpackagelib/lib_sysv.py
@@ -0,0 +1,66 @@
+import os
+import re
+
+from checkpackagelib.base import _CheckFunction
+from checkpackagelib.lib import ConsecutiveEmptyLines  # noqa: F401
+from checkpackagelib.lib import EmptyLastLine          # noqa: F401
+from checkpackagelib.lib import NewlineAtEof           # noqa: F401
+from checkpackagelib.lib import TrailingSpace          # noqa: F401
+from checkpackagelib.tool import NotExecutable as NotExecutable_base
+
+
+class Indent(_CheckFunction):
+    INDENTED_WITH_SPACES = re.compile(r"^[\t]* ")
+
+    def check_line(self, lineno, text):
+        if self.INDENTED_WITH_SPACES.search(text.rstrip()):
+            return ["{}:{}: should be indented with tabs, see package/busybox/S01syslogd".format(self.filename, lineno),
+                    text]
+
+
+class NotExecutable(NotExecutable_base):
+    def hint(self):
+        return ", just make sure you use '$(INSTALL) -D -m 0755' in the .mk file"
+
+
+# avoid check-package running both the parent and child classes. NOTE: NotExecutable_base.__name__ returns 'NotExecutable'
+del NotExecutable_base
+
+
+class Variables(_CheckFunction):
+    DAEMON_VAR = re.compile(r"^DAEMON=[\"']{0,1}([^\"']*)[\"']{0,1}")
+    PIDFILE_PATTERN = re.compile(r"/var/run/(\$DAEMON|\$\{DAEMON\}).pid")
+    PIDFILE_VAR = re.compile(r"^PIDFILE=[\"']{0,1}([^\"']*)[\"']{0,1}")
+
+    def before(self):
+        self.name = None
+
+    def check_line(self, lineno, text):
+        name_found = self.DAEMON_VAR.search(text.rstrip())
+        if name_found:
+            if self.name:
+                return ["{}:{}: DAEMON variable redefined, see package/busybox/S01syslogd".format(self.filename, lineno),
+                        text]
+            self.name = name_found.group(1)
+            if '/' in self.name:
+                self.name = os.path.basename(self.name)  # to be used in after() to check the expected filename
+                return ["{}:{}: Do not include path in DAEMON, see package/busybox/S01syslogd".format(self.filename, lineno),
+                        text,
+                        'DAEMON="{}"'.format(self.name)]
+            return
+
+        pidfile_found = self.PIDFILE_VAR.search(text.rstrip())
+        if pidfile_found:
+            pidfile = pidfile_found.group(1)
+            if not self.PIDFILE_PATTERN.match(pidfile):
+                return ["{}:{}: For PIDFILE use the same pattern found in package/busybox/S01syslogd".format(self.filename, lineno),
+                        text,
+                        'PIDFILE="/var/run/$DAEMON.pid"']
+
+    def after(self):
+        if self.name is None:
+            return ["{}:0: DAEMON variable not defined, see package/busybox/S01syslogd".format(self.filename)]
+        expected_filename = re.compile(r"S\d\d{}$".format(self.name))
+        if not expected_filename.match(os.path.basename(self.filename)):
+            return ["{}:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd".format(self.filename),
+                    "expecting S<number><number>{}".format(self.name)]
diff --git a/utils/checkpackagelib/test_lib_sysv.py b/utils/checkpackagelib/test_lib_sysv.py
new file mode 100644
index 0000000000..290539c0e4
--- /dev/null
+++ b/utils/checkpackagelib/test_lib_sysv.py
@@ -0,0 +1,131 @@
+import os
+import pytest
+import re
+import tempfile
+import checkpackagelib.test_util as util
+import checkpackagelib.lib_sysv as m
+from checkpackagelib.test_tool import check_file as tool_check_file
+
+workdir = os.path.join(tempfile.mkdtemp(suffix='-checkpackagelib-test-sysv'))
+workdir_regex = re.compile(r'/tmp/tmp[^/]*-checkpackagelib-test-sysv')
+
+
+Indent = [
+    ('empty file',
+     'any',
+     '',
+     []),
+    ('empty line',
+     'any',
+     '\n',
+     []),
+    ('ignore whitespace',
+     'any',
+     ' \n',
+     []),
+    ('spaces',
+     'any',
+     'case "$1" in\n'
+     '  start)',
+     [['any:2: should be indented with tabs, see package/busybox/S01syslogd',
+       '  start)']]),
+    ('tab',
+     'any',
+     'case "$1" in\n'
+     '\tstart)',
+     []),
+    ('tabs and spaces',
+     'any',
+     'case "$1" in\n'
+     '\t  start)',
+     [['any:2: should be indented with tabs, see package/busybox/S01syslogd',
+       '\t  start)']]),
+    ('spaces and tabs',
+     'any',
+     'case "$1" in\n'
+     '  \tstart)',
+     [['any:2: should be indented with tabs, see package/busybox/S01syslogd',
+       '  \tstart)']]),
+    ]
+
+
+@pytest.mark.parametrize('testname,filename,string,expected', Indent)
+def test_Indent(testname, filename, string, expected):
+    warnings = util.check_file(m.Indent, filename, string)
+    assert warnings == expected
+
+
+NotExecutable = [
+    ('SysV',
+     'sh-shebang.sh',
+     0o775,
+     '#!/bin/sh',
+     ["dir/sh-shebang.sh:0: This file does not need to be executable,"
+      " just make sure you use '$(INSTALL) -D -m 0755' in the .mk file"]),
+    ]
+
+
+@pytest.mark.parametrize('testname,filename,permissions,string,expected', NotExecutable)
+def test_NotExecutable(testname, filename, permissions, string, expected):
+    warnings = tool_check_file(m.NotExecutable, filename, string, permissions)
+    assert warnings == expected
+
+
+Variables = [
+    ('empty file',
+     'any',
+     '',
+     [['any:0: DAEMON variable not defined, see package/busybox/S01syslogd']]),
+    ('daemon and pidfile ok',
+     'package/busybox/S01syslogd',
+     'DAEMON="syslogd"\n'
+     'PIDFILE="/var/run/$DAEMON.pid"\n',
+     []),
+    ('wrong filename',
+     'package/busybox/S01syslog',
+     'DAEMON="syslogd"\n'
+     'PIDFILE="/var/run/${DAEMON}.pid"\n',
+     [['package/busybox/S01syslog:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd',
+       'expecting S<number><number>syslogd']]),
+    ('no pidfile ok',
+     'S99something',
+     'DAEMON="something"\n',
+     []),
+    ('hardcoded pidfile',
+     'S99something',
+     'DAEMON="something"\n'
+     'PIDFILE="/var/run/something.pid"\n',
+     [['S99something:2: For PIDFILE use the same pattern found in package/busybox/S01syslogd',
+       'PIDFILE="/var/run/something.pid"\n',
+       'PIDFILE="/var/run/$DAEMON.pid"']]),
+    ('redefined daemon',
+     'S50any',
+     'DAEMON="any"\n'
+     'DAEMON="other"\n',
+     [['S50any:2: DAEMON variable redefined, see package/busybox/S01syslogd',
+       'DAEMON="other"\n']]),
+    ('daemon name with dash',
+     'S82cups-browsed',
+     'DAEMON="cups-browsed"',
+     []),
+    ('daemon with path',
+     'S50avahi-daemon',
+     'DAEMON=/usr/sbin/avahi-daemon',
+     [['S50avahi-daemon:1: Do not include path in DAEMON, see package/busybox/S01syslogd',
+       'DAEMON=/usr/sbin/avahi-daemon',
+       'DAEMON="avahi-daemon"']]),
+    ('daemon with path and wrong filename',
+     'S50avahi',
+     'DAEMON=/usr/sbin/avahi-daemon',
+     [['S50avahi:1: Do not include path in DAEMON, see package/busybox/S01syslogd',
+       'DAEMON=/usr/sbin/avahi-daemon',
+       'DAEMON="avahi-daemon"'],
+      ['S50avahi:0: filename should be S<number><number><deamon name>, see package/busybox/S01syslogd',
+       'expecting S<number><number>avahi-daemon']]),
+    ]
+
+
+@pytest.mark.parametrize('testname,filename,string,expected', Variables)
+def test_Variables(testname, filename, string, expected):
+    warnings = util.check_file(m.Variables, filename, string)
+    assert warnings == expected
diff --git a/utils/checkpackagelib/test_tool.py b/utils/checkpackagelib/test_tool.py
index e9e0838103..aab7cb51c9 100644
--- a/utils/checkpackagelib/test_tool.py
+++ b/utils/checkpackagelib/test_tool.py
@@ -39,3 +39,28 @@ NotExecutable = [
 def test_NotExecutable(testname, filename, permissions, string, expected):
     warnings = check_file(m.NotExecutable, filename, string, permissions)
     assert warnings == expected
+
+
+NotExecutable_hint = [
+    ('no hint',
+     "",
+     'sh-shebang.sh',
+     0o775,
+     '#!/bin/sh',
+     ["dir/sh-shebang.sh:0: This file does not need to be executable"]),
+    ('hint',
+     ", very special hint",
+     'sh-shebang.sh',
+     0o775,
+     '#!/bin/sh',
+     ["dir/sh-shebang.sh:0: This file does not need to be executable, very special hint"]),
+    ]
+
+
+@pytest.mark.parametrize('testname,hint,filename,permissions,string,expected', NotExecutable_hint)
+def test_NotExecutable_hint(testname, hint, filename, permissions, string, expected):
+    class NotExecutable(m.NotExecutable):
+        def hint(self):
+            return hint
+    warnings = check_file(NotExecutable, filename, string, permissions)
+    assert warnings == expected
diff --git a/utils/checkpackagelib/tool.py b/utils/checkpackagelib/tool.py
index f2007be1ff..e931272554 100644
--- a/utils/checkpackagelib/tool.py
+++ b/utils/checkpackagelib/tool.py
@@ -5,4 +5,4 @@ from checkpackagelib.base import _Tool
 class NotExecutable(_Tool):
     def run(self):
         if os.access(self.filename, os.X_OK):
-            return ["{}:0: This file does not need to be executable".format(self.filename)]
+            return ["{}:0: This file does not need to be executable{}".format(self.filename, self.hint())]
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 4/5] support/docker: add shellcheck

[Ricardo Martincoski]

Allow developers to run check-package for init scripts, that call
shellcheck, without having to install the tool.

Since the docker have a fixed version of the tool, there will be no
difference between runs in different machines.

One can call:
$ utils/docker-run utils/check-package package/package/S*
$ utils/docker-run shellcheck package/package/S*

This change also allows to eventually run check-package for init scripts
in the GitLab CI.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Please read the cover letter before applying.
---
 support/docker/Dockerfile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile
index 792e70e292..2b14f0f7a7 100644
--- a/support/docker/Dockerfile
+++ b/support/docker/Dockerfile
@@ -44,6 +44,7 @@ RUN apt-get install -y --no-install-recommends \
         qemu-system-arm \
         qemu-system-x86 \
         rsync \
+        shellcheck \
         subversion \
         unzip \
         wget \
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 4/5] support/docker: add shellcheck

[Romain Naour]

Hello Ricardo,

Acked-by: Romain Naour <romain.naour@smile.fr>

Best regards,
Romain


Le 26/12/2021 à 19:49, Ricardo Martincoski a écrit :
> Allow developers to run check-package for init scripts, that call
> shellcheck, without having to install the tool.
> 
> Since the docker have a fixed version of the tool, there will be no
> difference between runs in different machines.
> 
> One can call:
> $ utils/docker-run utils/check-package package/package/S*
> $ utils/docker-run shellcheck package/package/S*
> 
> This change also allows to eventually run check-package for init scripts
> in the GitLab CI.
> 
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> ---
> Please read the cover letter before applying.
> ---
>  support/docker/Dockerfile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile
> index 792e70e292..2b14f0f7a7 100644
> --- a/support/docker/Dockerfile
> +++ b/support/docker/Dockerfile
> @@ -44,6 +44,7 @@ RUN apt-get install -y --no-install-recommends \
>          qemu-system-arm \
>          qemu-system-x86 \
>          rsync \
> +        shellcheck \
>          subversion \
>          unzip \
>          wget \
> 

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH 5/5] utils/checkpackagelib/lib_sysv: run shellcheck

[Ricardo Martincoski]

For simplicity, when shellcheck returns one or more warnings, count it
as a single check-package warning.
The developer can get the full output either by running shellcheck
directly or by calling check-package with enough -v.

Examples:

|$ ./utils/docker-run utils/check-package --include Shellcheck package/polkit/S50polkit
|package/polkit/S50polkit:0: run 'shellcheck' and fix the warnings
|51 lines processed
|1 warnings generated

|$ ./utils/docker-run utils/check-package --include Shellcheck -vvvvvvvvvvv package/polkit/S50polkit
|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.
|For more information:
|  https://www.shellcheck.net/wiki/SC2221 -- This pattern always overrides a l...
|  https://www.shellcheck.net/wiki/SC2222 -- This pattern never matches becaus...
|51 lines processed
|1 warnings generated

NOTICE: shellcheck results depends on the version of the tool.
This is why the examples above run inside the docker image.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Please read the cover letter before applying.
---
 utils/checkpackagelib/lib_sysv.py  |  1 +
 utils/checkpackagelib/test_tool.py | 46 ++++++++++++++++++++++++++++++
 utils/checkpackagelib/tool.py      | 15 ++++++++++
 3 files changed, 62 insertions(+)

diff --git a/utils/checkpackagelib/lib_sysv.py b/utils/checkpackagelib/lib_sysv.py
index 77e6283b25..786510badf 100644
--- a/utils/checkpackagelib/lib_sysv.py
+++ b/utils/checkpackagelib/lib_sysv.py
@@ -7,6 +7,7 @@ from checkpackagelib.lib import EmptyLastLine          # noqa: F401
 from checkpackagelib.lib import NewlineAtEof           # noqa: F401
 from checkpackagelib.lib import TrailingSpace          # noqa: F401
 from checkpackagelib.tool import NotExecutable as NotExecutable_base
+from checkpackagelib.tool import Shellcheck            # noqa: F401
 
 
 class Indent(_CheckFunction):
diff --git a/utils/checkpackagelib/test_tool.py b/utils/checkpackagelib/test_tool.py
index aab7cb51c9..c5c1d48dfe 100644
--- a/utils/checkpackagelib/test_tool.py
+++ b/utils/checkpackagelib/test_tool.py
@@ -64,3 +64,49 @@ def test_NotExecutable_hint(testname, hint, filename, permissions, string, expec
             return hint
     warnings = check_file(NotExecutable, filename, string, permissions)
     assert warnings == expected
+
+
+Shellcheck = [
+    ('missing shebang',
+     'empty.sh',
+     '',
+     ["dir/empty.sh:0: run 'shellcheck' and fix the warnings",
+      "In dir/empty.sh line 1:",
+      "^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.",
+      "For more information:",
+      "  https://www.shellcheck.net/wiki/SC2148 -- Tips depend on target shell and y..."]),
+    ('sh shebang',
+     'sh-shebang.sh',
+     '#!/bin/sh',
+     []),
+    ('bash shebang',
+     'bash-shebang.sh',
+     '#!/bin/bash',
+     []),
+    ('2 warnings',
+     'unused.sh',
+     'unused=""',
+     ["dir/unused.sh:0: run 'shellcheck' and fix the warnings",
+      "In dir/unused.sh line 1:",
+      'unused=""',
+      "^-- SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.",
+      "^----^ SC2034: unused appears unused. Verify use (or export if used externally).",
+      "For more information:",
+      "  https://www.shellcheck.net/wiki/SC2148 -- Tips depend on target shell and y...",
+      "  https://www.shellcheck.net/wiki/SC2034 -- unused appears unused. Verify use..."]),
+    ('tab',
+     'tab.sh',
+     '\t#!/bin/sh',
+     ["dir/tab.sh:0: run 'shellcheck' and fix the warnings",
+      "In dir/tab.sh line 1:",
+      '\t#!/bin/sh',
+      "^-- SC1114: Remove leading spaces before the shebang.",
+      "For more information:",
+      "  https://www.shellcheck.net/wiki/SC1114 -- Remove leading spaces before the ..."]),
+    ]
+
+
+@pytest.mark.parametrize('testname,filename,string,expected', Shellcheck)
+def test_Shellcheck(testname, filename, string, expected):
+    warnings = check_file(m.Shellcheck, filename, string)
+    assert warnings == expected
diff --git a/utils/checkpackagelib/tool.py b/utils/checkpackagelib/tool.py
index e931272554..47f9634486 100644
--- a/utils/checkpackagelib/tool.py
+++ b/utils/checkpackagelib/tool.py
@@ -1,4 +1,5 @@
 import os
+import subprocess
 from checkpackagelib.base import _Tool
 
 
@@ -6,3 +7,17 @@ class NotExecutable(_Tool):
     def run(self):
         if os.access(self.filename, os.X_OK):
             return ["{}:0: This file does not need to be executable{}".format(self.filename, self.hint())]
+
+
+class Shellcheck(_Tool):
+    def run(self):
+        cmd = ['shellcheck', self.filename]
+        try:
+            p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            stdout = p.communicate()[0]
+            processed_output = [str(line.decode().rstrip()) for line in stdout.splitlines() if line]
+            if p.returncode == 0:
+                return
+            return ["{}:0: run 'shellcheck' and fix the warnings".format(self.filename)] + processed_output
+        except FileNotFoundError:
+            return ["{}:0: failed to call 'shellcheck'".format(self.filename)]
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 0/5] check-package for SysV init scripts (including shellcheck)

[Arnout Vandecappelle]

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
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot