Merge pull request #54 from YunoHost/rework-script-parsing

Rework script parsing : line-per-line (still using shlex)

package_linter.py

@@ -78,19 +78,6 @@ def file_exists(file_path):
     return os.path.isfile(file_path) and os.stat(file_path).st_size > 0
 
 
-def read_file_shlex(file_path):
-    f = open(file_path)
-    return shlex.shlex(f, False)
-
-
-def read_file_raw(file_path):
-    # remove every comments and empty lines from the file content to avoid
-    # false positives
-    f = open(file_path)
-    file = "\n".join(filter(None, re.sub("#.*[^\n]", "", f.read()).splitlines()))
-    return file
-
-
 # ############################################################################
 #   Actual high-level checks
 # ############################################################################
@@ -125,7 +112,7 @@ def check_files_exist(app_path):
     for filename in os.listdir(app_path + "/conf"):
         if not os.path.isfile(filename):
             continue
-        content = read_file_raw(app_path + "/conf/" + filename)
+        content = open(app_path + "/conf/" + filename).read()
         if "location" in content and "add_header" in content:
             print_warning("Do not use 'add_header' in the nginx conf. Use 'more_set_headers' instead. (See https://www.peterbe.com/plog/be-very-careful-with-your-add_header-in-nginx and https://github.com/openresty/headers-more-nginx-module#more_set_headers )")
 
@@ -283,18 +270,17 @@ def check_manifest(path):
         recognized_types = ("domain", "path", "boolean", "app", "password", "user", "string")
 
         for argument in manifest["arguments"]["install"]:
-            if not "type" in argument.keys():
+            if "type" not in argument.keys():
                 print_warning("[YEP-2.1] You should specify the type of the argument '%s'. You can use : %s." % (argument["name"], ', '.join(recognized_types)))
             elif argument["type"] not in recognized_types:
                 print_warning("[YEP-2.1] The type '%s' for argument '%s' is not recognized... it probably doesn't behave as you expect ? Choose among those instead : %s" % (argument["type"], argument["name"], ', '.join(recognized_types)))
 
             if "choices" in argument.keys():
-                choices = [ c.lower() for c in argument["choices"] ]
+                choices = [c.lower() for c in argument["choices"]]
                 if len(choices) == 2:
                     if ("true" in choices and "false" in choices) or ("yes" in choices and "no" in choices):
                         print_warning("Argument %s : you might want to simply use a boolean-type argument. No need to specify the choices list yourself." % argument["name"])
 
-
     if "url" in manifest and manifest["url"].endswith("_ynh"):
         print_warning("'url' is not meant to be the url of the yunohost package, but rather the website or repo of the upstream app itself...")
 
@@ -303,72 +289,50 @@ def check_verifications_done_before_modifying_system(script):
     """
     Check if verifications are done before modifying the system
     """
-    ok = True
+
-    modify_cmd = ''
+    if not script.contains("ynh_die") and not script.contains("exit"):
-    cmds = ("cp", "mkdir", "rm", "chown", "chmod", "apt-get", "apt", "service",
+        return
-           "find", "sed", "mysql", "swapon", "mount", "dd", "mkswap", "useradd")
+
+    # FIXME : this really looks like a very small subset of command that
+    # can be used ... also packagers are not supposed to use apt or service
+    # anymore ...
+    modifying_cmds = ("cp", "mkdir", "rm", "chown", "chmod", "apt-get", "apt",
+                      "service", "find", "sed", "mysql", "swapon", "mount",
+                      "dd", "mkswap", "useradd")
     cmds_before_exit = []
-    has_exit = False
+    for cmd in script.lines:
-    for cmd in script["shlex"]:
+        cmd = " ".join(cmd)
-        if cmd in ["ynh_die", "exit"]:
+
-            has_exit = True
+        if "ynh_die" in cmd or "exit" in cmd:
             break
         cmds_before_exit.append(cmd)
 
-    if not has_exit:
+    for modifying_cmd in modifying_cmds:
-        return
+        if any(modifying_cmd in cmd for cmd in cmds_before_exit):
-
+            print_error("[YEP-2.4] 'ynh_die' or 'exit' command is executed with system modification before (cmd '%s').\n"
-    for cmd in cmds_before_exit:
+                        "This system modification is an issue if a verification exit the script.\n"
-        if cmd in ["ynh_die", "exit"]:
+                        "You should move this verification before any system modification." % modifying_cmd, False)
-            break
+            return
-        if not ok or cmd in cmds:
-            modify_cmd = cmd
-            ok = False
-            break
-
-    if not ok:
-        print_error("[YEP-2.4] 'ynh_die' or 'exit' command is executed with system modification before (cmd '%s').\n"
-        "This system modification is an issue if a verification exit the script.\n"
-        "You should move this verification before any system modification." % (modify_cmd), False)
 
 
 def check_set_usage(script):
     present = False
 
-    if script["name"] in ["backup", "remove"]:
+    if script.name in ["backup", "remove"]:
-        present = "ynh_abort_if_errors" in script["shlex"] or "set -eu" in script["raw"]
+        present = script.contains("ynh_abort_if_errors") or script.contains("set -eu")
     else:
-        present = "ynh_abort_if_errors" in script["shlex"]
+        present = script.contains("ynh_abort_if_errors")
 
-    if script["name"] == "remove":
+    if script.name == "remove":
         # Remove script shouldn't use set -eu or ynh_abort_if_errors
         if present:
             print_error("[YEP-2.4] set -eu or ynh_abort_if_errors is present. "
                         "If there is a crash, it could put yunohost system in "
                         "a broken state. For details, look at "
                         "https://github.com/YunoHost/issues/issues/419")
-    else:
+    elif not present:
-        if not present:
+        print_error("[YEP-2.4] ynh_abort_if_errors is missing. For details, "
-            print_error("[YEP-2.4] ynh_abort_if_errors is missing. For details, "
+                    "look at https://github.com/YunoHost/issues/issues/419")
-                        "look at https://github.com/YunoHost/issues/issues/419")
-
-
-def check_arg_retrieval(script):
-    """
-    Check arguments retrival from manifest is done with env var $YNH_APP_ARG_* and not with arg $1
-    env var was found till line ~30 on scripts. Stop file checking at L30: This could avoid wrong positives
-    Check only from '$1' to '$10' as 10 arg retrieval is already a lot.
-    """
-    present = False
-
-    for cmd in script:
-        if cmd == '$' and script.get_token() in [str(x) for x in range(1, 10)]:
-            present = True
-            break
-
-    if present:
-        print_error("Argument retrieval from manifest with $1 is deprecated. You may use $YNH_APP_ARG_*")
-        print_error("For more details see: https://yunohost.org/#/packaging_apps_arguments_management_en")
 
 
 def check_helper_usage_dependencies(script):
@@ -377,10 +341,10 @@ def check_helper_usage_dependencies(script):
     and suggest herlpers ynh_install_app_dependencies and ynh_remove_app_dependencies
     """
 
-    if "ynh_package_install" in script["shlex"] or "apt-get install" in script["raw"]:
+    if script.contains("ynh_package_install") or script.contains("apt-get install"):
         print_warning("You should not use `ynh_package_install` or `apt-get install`, use `ynh_install_app_dependencies` instead")
 
-    if "ynh_package_remove" in script["shlex"] or "apt-get remove" in script["raw"]:
+    if script.contains("ynh_package_remove") or script.contains("apt-get remove"):
         print_warning("You should not use `ynh_package_remove` or `apt-get remove`, use `ynh_remove_app_dependencies` instead")
 
 
@@ -390,53 +354,97 @@ def check_helper_consistency(script):
     so dependencies are up to date after restoration or upgrade
     """
 
-    if script["name"] == "install" and "ynh_install_app_dependencies" in script["shlex"]:
+    if script.name == "install" and script.contains("ynh_install_app_dependencies"):
-        for name in ["upgrade", "restore"]:
-            try:
-                script2 = read_file_raw(os.path.dirname(script["path"] + "/" + name))
-                if "ynh_install_app_dependencies" not in script2:
-                    print_warning("ynh_install_app_dependencies should also be in %s script" % name)
-            except FileNotFoundError:
-                pass
 
-    if script["name"] == "install" and "yunohost service add" in script["raw"]:
+        for name in ["upgrade", "restore"]:
-        try:
+            script2 = Script(name, os.path.dirname(script.path))
-            script2 = read_file_raw(os.path.dirname(script["path"]) + "/remove")
+            if script2.exists and not script2.contains("ynh_install_app_dependencies"):
-            if "yunohost service remove" not in script2:
+                print_warning("ynh_install_app_dependencies should also be in %s script" % name)
+
+    if script.name == "install" and script.contains("yunohost service add"):
+        script2 = Script("remove", os.path.dirname(script.path))
+        if script2.exists and not script2.contains("yunohost service remove"):
                 print_error("You used 'yunohost service add' in the install script, but not 'yunohost service remove' in the remove script.")
-        except FileNotFoundError:
-            pass
 
 
 def check_deprecated_practices(script):
 
-    if "yunohost app setting" in script["raw"]:
+    if script.contains("yunohost app setting"):
         print_warning("'yunohost app setting' shouldn't be used directly. Please use 'ynh_app_setting_(set,get,delete)' instead.")
-    if "yunohost app checkurl" in script["raw"]:
+    if script.contains("yunohost app checkurl"):
         print_warning("'yunohost app checkurl' is deprecated. Please use 'ynh_webpath_register' instead.")
-    if "yunohost app checkport" in script["raw"]:
+    if script.contains("yunohost app checkport"):
         print_warning("'yunohost app checkport' is deprecated. Please use 'ynh_find_port' instead.")
-    if "yunohost app initdb" in script["raw"]:
+    if script.contains("yunohost app initdb"):
         print_warning("'yunohost app initdb' is deprecated. Please use 'ynh_mysql_setup_db' instead.")
-    if "exit" in script["shlex"]:
+    if script.contains("exit"):
         print_warning("'exit' command shouldn't be used. Please use 'ynh_die' instead.")
 
-    if "rm -rf" in script["raw"] or "rm -Rf" in script["raw"]:
+    if script.contains("rm -rf"):
         print_error("[YEP-2.12] You should avoid using 'rm -rf', please use 'ynh_secure_remove' instead")
-    if "sed -i" in script["raw"]:
+    if script.contains("sed -i"):
         print_warning("[YEP-2.12] You should avoid using 'sed -i', please use 'ynh_replace_string' instead")
-    if "sudo " in script["raw"]:
+    if script.contains("sudo"):
         print_warning("[YEP-2.12] You should not need to use 'sudo', the script is being run as root. (If you need to run a command using a specific user, use 'ynh_exec_as')")
 
-    if "dd if=/dev/urandom" in script["raw"] or "openssl rand" in script["raw"]:
+    if script.contains("dd if=/dev/urandom") or script.contains("openssl rand"):
         print_warning("Instead of 'dd if=/dev/urandom' or 'openssl rand', you might want to use ynh_string_random")
 
-    if "systemctl restart nginx" in script["raw"] or "service nginx restart" in script["raw"]:
+    if script.contains("systemctl restart nginx") or script.contains("service nginx restart"):
         print_error("Restarting nginx is quite dangerous (especially for web installs) and should be avoided at all cost. Use 'reload' instead.")
 
-    if script["name"] == "install" and "ynh_print_info" not in script["shlex"] and "ynh_script_progression" not in script["shlex"]:
+    if script.name == "install" and not script.contains("ynh_print_info") and not script.contains("ynh_script_progression"):
         print_warning("Please add a few messages for the user, to explain what is going on (in friendly, not-too-technical terms) during the installation. You can use 'ynh_print_info' or 'ynh_script_progression' for this.")
 
+class Script():
+
+    def __init__(self, app_path, name):
+        self.name = name
+        self.path = app_path + "/scripts/" + name
+        self.exists = file_exists(self.path)
+        if not self.exists:
+            return
+
+    def read_file(self):
+        with open(self.path) as f:
+            lines = f.readlines()
+
+        # Remove trailing spaces, empty lines and comment lines
+        lines = [line.strip() for line in lines]
+        lines = [line for line in lines if line and not line.startswith('#')]
+
+        # Merge lines when ending with \
+        lines = '\n'.join(lines).replace("\\\n", "").split("\n")
+
+        for line in lines:
+            try:
+                line = shlex.split(line, True)
+                yield line
+            except Exception as e:
+                print_warning("Could not parse this line (%s) : %s" % (e, line))
+
+    def contains(self, command):
+        """
+        Iterate on lines to check if command is contained in line
+
+        For instance, "app setting" is contained in "yunohost app setting $app ..."
+        """
+        return any(command in line
+                   for line in [ ' '.join(line) for line in self.lines])
+
+
+    def analyze(self):
+
+        print_header(self.name.upper() + " SCRIPT")
+        self.lines = list(self.read_file())
+
+        check_verifications_done_before_modifying_system(self)
+        check_set_usage(self)
+        check_helper_usage_dependencies(self)
+        check_helper_consistency(self)
+        check_deprecated_practices(self)
+
+
 def main():
     if len(sys.argv) != 2:
         print("Give one app package path.")
@@ -454,25 +462,9 @@ def main():
     scripts = ["install", "remove", "upgrade", "backup", "restore"]
     for script_name in scripts:
 
-        script = {"name": script_name,
+        script = Script(app_path, script_name)
-                  "path": app_path + "/scripts/" + script_name}
+        if script.exists:
-
+            script.analyze()
-        if not file_exists(script["path"]):
-            continue
-
-        print_header(script["name"].upper() + " SCRIPT")
-
-        script["raw"] = read_file_raw(script["path"])
-        # We transform the shlex thing into a list because the original
-        # object has completely fucked-up behaviors :|.
-        script["shlex"] = [ l for l in read_file_shlex(script["path"]) ]
-
-        check_verifications_done_before_modifying_system(script)
-        check_set_usage(script)
-        check_helper_usage_dependencies(script)
-        check_helper_consistency(script)
-        check_deprecated_practices(script)
-        # check_arg_retrieval(script)
 
     sys.exit(return_code)