utils/checkpackagelib/lib_sysv: check SysV init scripts

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.

Also add unit test for this.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
[Arnout: avoid 'del NotExecutable_base' by importing the module instead
 of the class; refer to manual in warnings]
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

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
 
 

utils/checkpackagelib/base.py

@@ -24,3 +24,6 @@ class _Tool(object):
 
     def run(self):
         pass
+
+    def hint(self):
+        return ""

utils/checkpackagelib/lib_sysv.py

@@ -0,0 +1,68 @@
+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
+import checkpackagelib.tool
+
+
+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 ({}#adding-packages-start-script)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text]
+
+
+class NotExecutable(checkpackagelib.tool.NotExecutable):
+    def hint(self):
+        return ", just make sure you use '$(INSTALL) -D -m 0755' in the .mk file"
+
+
+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 ({}#adding-packages-start-script)"
+                        .format(self.filename, lineno, self.url_to_manual),
+                        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 ({}#adding-packages-start-script)"
+                        .format(self.filename, lineno, self.url_to_manual),
+                        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 ["{}:{}: Incorrect PIDFILE value  ({}#adding-packages-start-script)"
+                        .format(self.filename, lineno, self.url_to_manual),
+                        text,
+                        'PIDFILE="/var/run/$DAEMON.pid"']
+
+    def after(self):
+        if self.name is None:
+            return ["{}:0: DAEMON variable not defined ({}#adding-packages-start-script)"
+                    .format(self.filename, self.url_to_manual)]
+        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><daemon name> ({}#adding-packages-start-script)"
+                    .format(self.filename, self.url_to_manual),
+                    "expecting S<number><number>{}".format(self.name)]

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 (url#adding-packages-start-script)',
+       '  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 (url#adding-packages-start-script)',
+       '\t  start)']]),
+    ('spaces and tabs',
+     'any',
+     'case "$1" in\n'
+     '  \tstart)',
+     [['any:2: should be indented with tabs (url#adding-packages-start-script)',
+       '  \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 (url#adding-packages-start-script)']]),
+    ('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><daemon name> (url#adding-packages-start-script)',
+       '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: Incorrect PIDFILE value  (url#adding-packages-start-script)',
+       '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 (url#adding-packages-start-script)',
+       '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 (url#adding-packages-start-script)',
+       '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 (url#adding-packages-start-script)',
+       'DAEMON=/usr/sbin/avahi-daemon',
+       'DAEMON="avahi-daemon"'],
+      ['S50avahi:0: filename should be S<number><number><daemon name> (url#adding-packages-start-script)',
+       '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

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

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())]