From f31bebc644b08f25b3f5d67209eec41449dd2df8 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 27 Jan 2019 16:32:04 +0100 Subject: [PATCH 01/14] Rework and add deprecated helpers usage and deprecated practices --- package_linter.py | 47 ++++++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/package_linter.py b/package_linter.py index a76e4a1..7745a06 100755 --- a/package_linter.py +++ b/package_linter.py @@ -251,13 +251,13 @@ def check_script(path, script_name, script_nbr): print(c.BOLD + c.HEADER + "\n>>>>", script_name.upper(), "SCRIPT <<<<" + c.END) - check_non_helpers_usage(read_file(script_path)) if script_nbr < 5: check_verifications_done_before_modifying_system(read_file(script_path)) check_set_usage(script_name, read_file(script_path)) check_helper_usage_dependencies(script_path, script_name) check_helper_usage_unix(script_path, script_name) check_helper_consistency(script_path, script_name) + check_deprecated_practices(script_path, script_name) #check_arg_retrieval(script.copy()) @@ -294,32 +294,6 @@ def check_verifications_done_before_modifying_system(script): "You should move this verification before any system modification." % (modify_cmd) , False) -def check_non_helpers_usage(script): - """ - check if deprecated commands are used and propose helpers: - - 'yunohost app setting' –> ynh_app_setting_(set,get,delete) - - 'exit' –> 'ynh_die' - """ - - ok = True - #TODO - #for line_nbr, cmd in script: - # if "yunohost app setting" in cmd: - # print_wrong("[YEP-2.11] Line {}: 'yunohost app setting' command is deprecated," - # " please use helpers ynh_app_setting_(set,get,delete)." - # .format(line_nbr + 1)) - # ok = False - - if not ok: - print("Helpers documentation: " - "https://yunohost.org/#/packaging_apps_helpers\n" - "code: https://github.com/YunoHost/yunohost/…helpers") - - if "exit" in script: - print_wrong("[YEP-2.4] 'exit' command shouldn't be used." - "Use 'ynh_die' helper instead.") - - def check_set_usage(script_name, script): present = False @@ -406,6 +380,25 @@ def check_helper_consistency(path, script_name): except FileNotFoundError: pass +def check_deprecated_practices(path, script_name): + + script = read_file(path) + + if "yunohost app setting" in script: + 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: + print_warning("'yunohost app checkurl' is deprecated. Please use 'ynh_webpath_register' instead.") + if "yunohost app checkport" in script: + print_warning("'yunohost app checkport' is deprecated. Please use 'ynh_find_port' instead.") + if "yunohost app initdb" in script: + print_warning("'yunohost app initdb' is deprecated. Please use 'ynh_mysql_setup_db' instead.") + if "exit" in script: + print_warning("'exit' command shouldn't be used. Please use 'ynh_die' instead.") + + if os.path.exists("%s/../conf/php-fpm.ini" % os.path.dirname(path)): + print_warning("Using a separate php-fpm.ini file is deprecated. Please merge your php-fpm directives directly in the pool file. (c.f. https://github.com/YunoHost-Apps/nextcloud_ynh/issues/138 )") + + if __name__ == '__main__': if len(sys.argv) != 2: print("Give one app package path.") From 8060598fed3bfc172603034de16f98e9ce729c4e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 28 Jan 2019 20:04:28 +0100 Subject: [PATCH 02/14] Ugh ... major bug, shlex actually split the whole file in words. If we look for 'rm -rf' occurrences for instances, those are two separate words ... used a raw read instead --- package_linter.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/package_linter.py b/package_linter.py index 7745a06..3361e22 100755 --- a/package_linter.py +++ b/package_linter.py @@ -93,6 +93,9 @@ def read_file(file_path): #file = filter(None, re.sub("#.*[^\n]", "", f.read()).splitlines()) return file +def read_file_raw(file_path): + return open(file_path).read() + def check_source_management(app_path): print(c.BOLD + c.HEADER + "\n>>>> SOURCES MANAGEMENT <<<<" + c.END) @@ -338,7 +341,7 @@ def check_helper_usage_dependencies(path, script_name): Detect usage of ynh_package_* & apt-get * and suggest herlpers ynh_install_app_dependencies and ynh_remove_app_dependencies """ - script = read_file(path) + script = read_file_raw(path) if "ynh_package_install" in script or "apt-get install" in script: print_warning("You should not use `ynh_package_install` or `apt-get install`, use `ynh_install_app_dependencies` instead") @@ -353,7 +356,7 @@ def check_helper_usage_unix(path, script_name): - rm → ynh_secure_remove - sed -i → ynh_replace_string """ - script = read_file(path) + script = read_file_raw(path) if "rm -rf" in script or "rm -Rf" in script: print_wrong("[YEP-2.12] You should avoid using `rm -rf`, please use `ynh_secure_remove` instead") @@ -369,12 +372,12 @@ def check_helper_consistency(path, script_name): check if ynh_install_app_dependencies is present in install/upgrade/restore so dependencies are up to date after restoration or upgrade """ - script = read_file(path) + script = read_file_raw(path) if script_name == "install" and "ynh_install_app_dependencies" in script: for name in ["upgrade", "restore"]: try: - script2 = read_file(os.path.dirname(path) + "/" + name) + script2 = read_file_raw(os.path.dirname(path) + "/" + name) if not "ynh_install_app_dependencies" in script2: print_warning("ynh_install_app_dependencies should also be in %s script" % name) except FileNotFoundError: @@ -382,7 +385,7 @@ def check_helper_consistency(path, script_name): def check_deprecated_practices(path, script_name): - script = read_file(path) + script = read_file_raw(path) if "yunohost app setting" in script: print_warning("'yunohost app setting' shouldn't be used directly. Please use 'ynh_app_setting_(set,get,delete)' instead.") From c7f40ca5662b01deb94c38579d3878b9ba8ebaf6 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 28 Jan 2019 20:05:38 +0100 Subject: [PATCH 03/14] Recommend ynh_string_random instead of 'dd' or 'openssl rand' --- package_linter.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/package_linter.py b/package_linter.py index 3361e22..4710163 100755 --- a/package_linter.py +++ b/package_linter.py @@ -398,6 +398,9 @@ def check_deprecated_practices(path, script_name): if "exit" in script: print_warning("'exit' command shouldn't be used. Please use 'ynh_die' instead.") + if "dd if=/dev/urandom" in script or "openssl rand" in script: + print_warning("Instead of 'dd if=/dev/urandom' or 'openssl rand', you might want to use ynh_string_random") + if os.path.exists("%s/../conf/php-fpm.ini" % os.path.dirname(path)): print_warning("Using a separate php-fpm.ini file is deprecated. Please merge your php-fpm directives directly in the pool file. (c.f. https://github.com/YunoHost-Apps/nextcloud_ynh/issues/138 )") From 5a45d7b7bb6b2b1490e4b99d209dcc1cb81625b4 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 28 Jan 2019 20:35:07 +0100 Subject: [PATCH 04/14] Remove commented lines when reading file in raw mode --- package_linter.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/package_linter.py b/package_linter.py index 4710163..454c778 100755 --- a/package_linter.py +++ b/package_linter.py @@ -87,14 +87,15 @@ def check_file_exist(file_path): def read_file(file_path): f = open(file_path) - # remove every comments and empty lines from the file content to avoid - # false positives file = shlex.shlex(f, False) - #file = filter(None, re.sub("#.*[^\n]", "", f.read()).splitlines()) return file def read_file_raw(file_path): - return open(file_path).read() + # 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 def check_source_management(app_path): From fcb9c17edc37919ae9674788949b0e4fdaa3a23b Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Mon, 28 Jan 2019 23:33:27 +0100 Subject: [PATCH 05/14] Big ugly refactoring commit. Boooh. Not cool :|. --- package_linter.py | 268 +++++++++++++++++++++++----------------------- 1 file changed, 136 insertions(+), 132 deletions(-) diff --git a/package_linter.py b/package_linter.py index 454c778..dcd6b01 100755 --- a/package_linter.py +++ b/package_linter.py @@ -13,8 +13,13 @@ reader = codecs.getreader("utf-8") return_code = 0 +# ############################################################################ +# Utilities +# ############################################################################ + + class c: - HEADER = '\033[95m' + HEADER = '\033[94m' OKBLUE = '\033[94m' OKGREEN = '\033[92m' WARNING = '\033[93m' @@ -25,14 +30,21 @@ class c: UNDERLINE = '\033[4m' -def header(app_path): - print(c.UNDERLINE + c.HEADER + c.BOLD + - "YUNOHOST APP PACKAGE LINTER\n", c.END, - "App packaging documentation: https://yunohost.org/#/packaging_apps\n", - "App package example: https://github.com/YunoHost/example_ynh\n", - "Official helpers: https://yunohost.org/#/packaging_apps_helpers_en\n", - "Experimental helpers: https://github.com/YunoHost-Apps/Experimental_helpers\n" - "Checking " + c.BOLD + app_path + c.END + " package\n") +def header(app): + print(""" + [{header}{bold}YunoHost App Package Linter{end}] + + App packaging documentation - https://yunohost.org/#/packaging_apps + App package example - https://github.com/YunoHost/example_ynh + Official helpers - https://yunohost.org/#/packaging_apps_helpers_en + Experimental helpers - https://github.com/YunoHost-Apps/Experimental_helpers + + Analyzing package {header}{app}{end}""" + .format(header=c.HEADER, bold=c.BOLD, end=c.END, app=app)) + + +def print_header(str): + print("\n [" + c.BOLD + c.HEADER + str.title() + c.END + "]\n") def print_right(str): @@ -62,33 +74,14 @@ def urlopen(url): return {'content': conn.read().decode('UTF8'), 'code': 200} -def check_files_exist(app_path): - """ - Check files exist - 'backup' and 'restore' scripts are mandatory - """ - - print(c.BOLD + c.HEADER + ">>>> MISSING FILES <<<<" + c.END) - fnames = ("manifest.json", "scripts/install", "scripts/remove", - "scripts/upgrade", "scripts/backup", "scripts/restore", "LICENSE", - "README.md") - - for nbr, fname in enumerate(fnames): - if not check_file_exist(app_path + "/" + fname): - if nbr != 4 and nbr != 5: - print_wrong(fname) - else: - print_warning(fname) +def file_exists(file_path): + return os.path.isfile(file_path) and os.stat(file_path).st_size > 0 -def check_file_exist(file_path): - return 1 if os.path.isfile(file_path) and os.stat(file_path).st_size > 0 else 0 - - -def read_file(file_path): +def read_file_shlex(file_path): f = open(file_path) - file = shlex.shlex(f, False) - return file + return shlex.shlex(f, False) + def read_file_raw(file_path): # remove every comments and empty lines from the file content to avoid @@ -98,29 +91,63 @@ def read_file_raw(file_path): return file +# ############################################################################ +# Actual high-level checks +# ############################################################################ + + +def check_files_exist(app_path): + """ + Check files exist + 'backup' and 'restore' scripts are not mandatory + """ + + print_header("MISSING FILES") + filenames = ("manifest.json", + "scripts/install", "scripts/remove", + "scripts/upgrade", + "scripts/backup", "scripts/restore", + "LICENSE", "README.md") + non_mandatory = ("script/backup", "script/restore") + + for filename in filenames: + if file_exists(app_path + "/" + filename): + continue + elif filename in non_mandatory: + print_warning(filename) + else: + print_wrong(filename) + + # Deprecated php-fpm.ini thing + if file_exists(app_path + "/conf/php-fpm.ini"): + print_warning("Using a separate php-fpm.ini file is deprecated. Please merge your php-fpm directives directly in the pool file. (c.f. https://github.com/YunoHost-Apps/nextcloud_ynh/issues/138 )") + + def check_source_management(app_path): - print(c.BOLD + c.HEADER + "\n>>>> SOURCES MANAGEMENT <<<<" + c.END) + print_header("SOURCES MANAGEMENT") DIR = os.path.join(app_path, "sources") # Check if there is more than six files on 'sources' folder - if os.path.exists(os.path.join(app_path, "sources")) and \ - len([name for name in os.listdir(DIR) if os.path.isfile(os.path.join(DIR, name))]) > 5: + if os.path.exists(os.path.join(app_path, "sources")) \ + and len([name for name in os.listdir(DIR) if os.path.isfile(os.path.join(DIR, name))]) > 5: print_warning("[YEP-3.3] Upstream app sources shouldn't be stored on this " "'sources' folder of this git repository as a copy/paste." "\nAt installation, the package should download sources " "from upstream via 'ynh_setup_source'.\nSee " "https://dev.yunohost.org/issues/201#Conclusion-chart") -def is_license_mention_in_readme(path): + +def license_mentionned_in_readme(path): readme_path = os.path.join(path, 'README.md') if os.path.isfile(readme_path): return "LICENSE" in open(readme_path).read() return False + def check_manifest(path): manifest = os.path.join(path, 'manifest.json') if not os.path.exists(manifest): return - print(c.BOLD + c.HEADER + "\n>>>> MANIFEST <<<<" + c.END) + print_header("MANIFEST") """ Check if there is no comma syntax issue """ @@ -158,7 +185,7 @@ def check_manifest(path): "'^[a-z1-9]((_|-)?[a-z1-9])+$'") if "name" in manifest: - if len(manifest["name"]) > 22 : + if len(manifest["name"]) > 22: print_warning("[YEP-1.1] The 'name' field shouldn't be too long to be" " able to be with one line in the app list. The most " "current bigger name is actually compound of 22 characters.") @@ -182,7 +209,7 @@ def check_manifest(path): " field is 'non-free' and not 'nonfree'") license = "non-free" if license in ["free", "non-free", "dep-non-free"]: - if not is_license_mention_in_readme(path): + if not license_mentionned_in_readme(path): print_warning("[YEP-1.3] The use of '%s' in license field implies to " "write something about the license in your " "README.md" % (license)) @@ -215,7 +242,8 @@ def check_manifest(path): if manifest["description"] == manifest["name"]: print_warning("[YEP-1.9] You should write a good description of the" "app (1 line is enough).") - #TODO test a specific template in README.md + + # TODO test a specific template in README.md # YEP 1.10 Garder un historique de version propre @@ -244,27 +272,6 @@ def check_manifest(path): print_wrong("[YEP-2.1] You should specify the type of the key with %s" % (typ)) - -def check_script(path, script_name, script_nbr): - - script_path = path + "/scripts/" + script_name - - if check_file_exist(script_path) == 0: - return - - print(c.BOLD + c.HEADER + "\n>>>>", - script_name.upper(), "SCRIPT <<<<" + c.END) - - if script_nbr < 5: - check_verifications_done_before_modifying_system(read_file(script_path)) - check_set_usage(script_name, read_file(script_path)) - check_helper_usage_dependencies(script_path, script_name) - check_helper_usage_unix(script_path, script_name) - check_helper_consistency(script_path, script_name) - check_deprecated_practices(script_path, script_name) - #check_arg_retrieval(script.copy()) - - def check_verifications_done_before_modifying_system(script): """ Check if verifications are done before modifying the system @@ -274,18 +281,18 @@ def check_verifications_done_before_modifying_system(script): cmds = ("cp", "mkdir", "rm", "chown", "chmod", "apt-get", "apt", "service", "find", "sed", "mysql", "swapon", "mount", "dd", "mkswap", "useradd") cmds_before_exit = [] - is_exit = False - for cmd in script: - if "ynh_die" == cmd or "exit " == cmd: - is_exit = True + has_exit = False + for cmd in script["shlex"]: + if cmd in ["ynh_die", "exit"]: + has_exit = True break cmds_before_exit.append(cmd) - if not is_exit: + if not has_exit: return for cmd in cmds_before_exit: - if "ynh_die" == cmd or "exit " == cmd: + if cmd in ["ynh_die", "exit"]: break if not ok or cmd in cmds: modify_cmd = cmd @@ -295,29 +302,28 @@ def check_verifications_done_before_modifying_system(script): if not ok: print_wrong("[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) + "You should move this verification before any system modification." % (modify_cmd), False) -def check_set_usage(script_name, script): +def check_set_usage(script): present = False - if script_name in ["backup", "remove"]: - present = "ynh_abort_if_errors" in script or "set -eu" in script + if script["name"] in ["backup", "remove"]: + present = "ynh_abort_if_errors" in script["raw"] or "set -eu" in script["raw"] else: - present = "ynh_abort_if_errors" in script + present = "ynh_abort_if_errors" in script["raw"] - if script_name == "remove": + if script["name"] == "remove": # Remove script shouldn't use set -eu or ynh_abort_if_errors if present: print_wrong("[YEP-2.4] set -eu or ynh_abort_if_errors is present. " - "If there is a crash it could put yunohost system in " - "invalidated states. For details, look at " - "https://dev.yunohost.org/issues/419") + "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: if not present: - print_wrong("[YEP-2.4] ynh_abort_if_errors is missing. For details," - "look at https://dev.yunohost.org/issues/419") - + print_wrong("[YEP-2.4] ynh_abort_if_errors is missing. For details, " + "look at https://github.com/YunoHost/issues/issues/419") def check_arg_retrieval(script): @@ -329,7 +335,7 @@ def check_arg_retrieval(script): present = False for cmd in script: - if cmd =='$' and script.get_token() in [str(x) for x in range(1, 10)]: + if cmd == '$' and script.get_token() in [str(x) for x in range(1, 10)]: present = True break @@ -337,99 +343,97 @@ def check_arg_retrieval(script): print_wrong("Argument retrieval from manifest with $1 is deprecated. You may use $YNH_APP_ARG_*") print_wrong("For more details see: https://yunohost.org/#/packaging_apps_arguments_management_en") -def check_helper_usage_dependencies(path, script_name): + +def check_helper_usage_dependencies(script): """ Detect usage of ynh_package_* & apt-get * and suggest herlpers ynh_install_app_dependencies and ynh_remove_app_dependencies """ - script = read_file_raw(path) - if "ynh_package_install" in script or "apt-get install" in script: + if "ynh_package_install" in script["shlex"] or "apt-get install" in script["raw"]: 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 or "apt-get remove" in script: + if "ynh_package_remove" in script["shlex"] or "apt-get remove" in script["raw"]: print_warning("You should not use `ynh_package_remove` or `apt-get removeè, use `ynh_remove_app_dependencies` instead") -def check_helper_usage_unix(path, script_name): - """ - Detect usage of unix commands with helper equivalents: - - sudo → ynh_exec_as - - rm → ynh_secure_remove - - sed -i → ynh_replace_string - """ - script = read_file_raw(path) - if "rm -rf" in script or "rm -Rf" in script: - print_wrong("[YEP-2.12] You should avoid using `rm -rf`, please use `ynh_secure_remove` instead") - - if "sed -i" in script: - print_warning("[YEP-2.12] You should avoid using `sed -i`, please use `ynh_replace_string` instead") - - if "sudo " in script: - 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`)") - -def check_helper_consistency(path, script_name): +def check_helper_consistency(script): """ check if ynh_install_app_dependencies is present in install/upgrade/restore so dependencies are up to date after restoration or upgrade """ - script = read_file_raw(path) - if script_name == "install" and "ynh_install_app_dependencies" in script: + if script["name"] == "install" and "ynh_install_app_dependencies" in script["shlex"]: for name in ["upgrade", "restore"]: try: - script2 = read_file_raw(os.path.dirname(path) + "/" + name) - if not "ynh_install_app_dependencies" in script2: + 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 -def check_deprecated_practices(path, script_name): - script = read_file_raw(path) +def check_deprecated_practices(script): - if "yunohost app setting" in script: + if "yunohost app setting" in script["raw"]: 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: + if "yunohost app checkurl" in script["raw"]: print_warning("'yunohost app checkurl' is deprecated. Please use 'ynh_webpath_register' instead.") - if "yunohost app checkport" in script: + if "yunohost app checkport" in script["raw"]: print_warning("'yunohost app checkport' is deprecated. Please use 'ynh_find_port' instead.") - if "yunohost app initdb" in script: + if "yunohost app initdb" in script["raw"]: print_warning("'yunohost app initdb' is deprecated. Please use 'ynh_mysql_setup_db' instead.") - if "exit" in script: + if "exit" in script["shlex"]: print_warning("'exit' command shouldn't be used. Please use 'ynh_die' instead.") - if "dd if=/dev/urandom" in script or "openssl rand" in script: + if "rm -rf" in script["raw"] or "rm -Rf" in script["raw"]: + print_wrong("[YEP-2.12] You should avoid using 'rm -rf', please use 'ynh_secure_remove' instead") + if "sed -i" in script["raw"]: + print_warning("[YEP-2.12] You should avoid using 'sed -i', please use 'ynh_replace_string' instead") + if "sudo " in script["raw"]: + 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"]: print_warning("Instead of 'dd if=/dev/urandom' or 'openssl rand', you might want to use ynh_string_random") - if os.path.exists("%s/../conf/php-fpm.ini" % os.path.dirname(path)): - print_warning("Using a separate php-fpm.ini file is deprecated. Please merge your php-fpm directives directly in the pool file. (c.f. https://github.com/YunoHost-Apps/nextcloud_ynh/issues/138 )") - -if __name__ == '__main__': +def main(): if len(sys.argv) != 2: print("Give one app package path.") exit() - # "or" trick to always be 1 if 1 is present: - # 1 or 0 = 1 - # 1 or 1 = 1 - # 0 or 1 = 1 - # 0 or 0 = 0 - app_path = sys.argv[1] header(app_path) + + # Global checks check_files_exist(app_path) check_source_management(app_path) - check_manifest(app_path) # + "/manifest.json") + check_manifest(app_path) + # Scripts checks scripts = ["install", "remove", "upgrade", "backup", "restore"] - for (dirpath, dirnames, filenames) in os.walk(os.path.join(app_path, "scripts")): - for filename in filenames: - if filename not in scripts and filename[-4:] != ".swp": - scripts.append(filename) + for script_name in scripts: - for script_nbr, script in enumerate(scripts): - check_script(app_path, script, script_nbr) + script = {"name": script_name, + "path": app_path + "/scripts/" + script_name} + + if not file_exists(script["path"]): + continue + + print_header(script["name"].upper() + " SCRIPT") + + script["raw"] = read_file_raw(script["path"]) + script["shlex"] = 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) + + +if __name__ == '__main__': + main() From 2e976bcdf3357709ab24a8c303a1bad03b47b1eb Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 29 Jan 2019 00:34:46 +0100 Subject: [PATCH 06/14] Shlex behavior is fucked up. --- package_linter.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/package_linter.py b/package_linter.py index dcd6b01..e83890c 100755 --- a/package_linter.py +++ b/package_linter.py @@ -260,7 +260,8 @@ def check_manifest(path): for service in manifest["services"]: if service not in services: - print_warning("[YEP-2.1]" + service + " service may not exist") + # FIXME : wtf is it supposed to mean ... + print_warning("[YEP-2.1] " + service + " service may not exist") if "install" in manifest["arguments"]: types = ("domain", "path", "password", "user", "admin") @@ -309,9 +310,9 @@ def check_set_usage(script): present = False if script["name"] in ["backup", "remove"]: - present = "ynh_abort_if_errors" in script["raw"] or "set -eu" in script["raw"] + present = "ynh_abort_if_errors" in script["shlex"] or "set -eu" in script["raw"] else: - present = "ynh_abort_if_errors" in script["raw"] + present = "ynh_abort_if_errors" in script["shlex"] if script["name"] == "remove": # Remove script shouldn't use set -eu or ynh_abort_if_errors @@ -423,7 +424,9 @@ def main(): print_header(script["name"].upper() + " SCRIPT") script["raw"] = read_file_raw(script["path"]) - script["shlex"] = read_file_shlex(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) From 171617cb8a01a946ec4ffadf3d15a80cd0ad9a7e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 29 Jan 2019 00:35:23 +0100 Subject: [PATCH 07/14] Complain about attempts to restart nginx --- package_linter.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/package_linter.py b/package_linter.py index e83890c..8ef7662 100755 --- a/package_linter.py +++ b/package_linter.py @@ -397,6 +397,8 @@ def check_deprecated_practices(script): if "dd if=/dev/urandom" in script["raw"] or "openssl rand" in script["raw"]: 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"]: + print_wrong("Restarting nginx is quite dangerous (especially for web installs) and should be avoided at all cost. Use 'reload' instead.") def main(): if len(sys.argv) != 2: From 657f30a94d6bd5c15acae918e61ffca10208dae3 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 29 Jan 2019 00:36:08 +0100 Subject: [PATCH 08/14] Complain if 'yunohost service add' is not matched with a 'yunohost service remove' --- package_linter.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/package_linter.py b/package_linter.py index 8ef7662..05b2294 100755 --- a/package_linter.py +++ b/package_linter.py @@ -373,6 +373,14 @@ def check_helper_consistency(script): except FileNotFoundError: pass + if script["name"] == "install" and "yunohost service add" in script["raw"]: + try: + script2 = read_file_raw(os.path.dirname(script["path"]) + "/remove") + if "yunohost service remove" not in script2: + print_wrong("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): From 0587845acbb57c30cbbcce9cc002c0ef792aa62e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 29 Jan 2019 00:44:33 +0100 Subject: [PATCH 09/14] print_wrong -> print_error --- package_linter.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/package_linter.py b/package_linter.py index 05b2294..3d46c38 100755 --- a/package_linter.py +++ b/package_linter.py @@ -55,7 +55,7 @@ def print_warning(str): print(c.WARNING + "!", str, c.END) -def print_wrong(str, reliable=True): +def print_error(str, reliable=True): if reliable: global return_code return_code = 1 @@ -116,7 +116,7 @@ def check_files_exist(app_path): elif filename in non_mandatory: print_warning(filename) else: - print_wrong(filename) + print_error(filename) # Deprecated php-fpm.ini thing if file_exists(app_path + "/conf/php-fpm.ini"): @@ -156,7 +156,7 @@ def check_manifest(path): with open(manifest, encoding='utf-8') as data_file: manifest = json.loads(data_file.read()) except: - print_wrong("[YEP-2.1] Syntax (comma) or encoding issue with manifest.json. " + print_error("[YEP-2.1] Syntax (comma) or encoding issue with manifest.json. " "Can't check file.") fields = ("name", "id", "packaging_format", "description", "url", "version", @@ -172,16 +172,16 @@ def check_manifest(path): """ if "packaging_format" not in manifest: - print_wrong("[YEP-2.1] \"packaging_format\" key is missing") + print_error("[YEP-2.1] \"packaging_format\" key is missing") elif not isinstance(manifest["packaging_format"], int): - print_wrong("[YEP-2.1] \"packaging_format\": value isn't an integer type") + print_error("[YEP-2.1] \"packaging_format\": value isn't an integer type") elif manifest["packaging_format"] != 1: - print_wrong("[YEP-2.1] \"packaging_format\" field: current format value is '1'") + print_error("[YEP-2.1] \"packaging_format\" field: current format value is '1'") # YEP 1.1 Name is app if "id" in manifest: if not re.match('^[a-z1-9]((_|-)?[a-z1-9])+$', manifest["id"]): - print_wrong("[YEP-1.1] 'id' field '%s' should respect this regex " + print_error("[YEP-1.1] 'id' field '%s' should respect this regex " "'^[a-z1-9]((_|-)?[a-z1-9])+$'") if "name" in manifest: @@ -251,7 +251,7 @@ def check_manifest(path): # YEP 2.1 if "multi_instance" in manifest and manifest["multi_instance"] != 1 and manifest["multi_instance"] != 0: - print_wrong( + print_error( "[YEP-2.1] \"multi_instance\" field must be boolean type values 'true' or 'false' and not string type") if "services" in manifest: @@ -270,7 +270,7 @@ def check_manifest(path): for install_arg in manifest["arguments"]["install"]: if typ == install_arg["name"]: if "type" not in install_arg: - print_wrong("[YEP-2.1] You should specify the type of the key with %s" % (typ)) + print_error("[YEP-2.1] You should specify the type of the key with %s" % (typ)) def check_verifications_done_before_modifying_system(script): @@ -301,7 +301,7 @@ def check_verifications_done_before_modifying_system(script): break if not ok: - print_wrong("[YEP-2.4] 'ynh_die' or 'exit' command is executed with system modification before (cmd '%s').\n" + 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) @@ -317,13 +317,13 @@ def check_set_usage(script): if script["name"] == "remove": # Remove script shouldn't use set -eu or ynh_abort_if_errors if present: - print_wrong("[YEP-2.4] set -eu or ynh_abort_if_errors is 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: if not present: - print_wrong("[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") @@ -341,8 +341,8 @@ def check_arg_retrieval(script): break if present: - print_wrong("Argument retrieval from manifest with $1 is deprecated. You may use $YNH_APP_ARG_*") - print_wrong("For more details see: https://yunohost.org/#/packaging_apps_arguments_management_en") + 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,7 +377,7 @@ def check_helper_consistency(script): try: script2 = read_file_raw(os.path.dirname(script["path"]) + "/remove") if "yunohost service remove" not in script2: - print_wrong("You used 'yunohost service add' in the install script, but not 'yunohost service remove' in the remove script.") + print_error("You used 'yunohost service add' in the install script, but not 'yunohost service remove' in the remove script.") except FileNotFoundError: pass @@ -396,7 +396,7 @@ def check_deprecated_practices(script): 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"]: - print_wrong("[YEP-2.12] You should avoid using 'rm -rf', please use 'ynh_secure_remove' instead") + print_error("[YEP-2.12] You should avoid using 'rm -rf', please use 'ynh_secure_remove' instead") if "sed -i" in script["raw"]: print_warning("[YEP-2.12] You should avoid using 'sed -i', please use 'ynh_replace_string' instead") if "sudo " in script["raw"]: @@ -406,7 +406,7 @@ def check_deprecated_practices(script): 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"]: - print_wrong("Restarting nginx is quite dangerous (especially for web installs) and should be avoided at all cost. Use 'reload' instead.") + print_error("Restarting nginx is quite dangerous (especially for web installs) and should be avoided at all cost. Use 'reload' instead.") def main(): if len(sys.argv) != 2: From a0c400f946d9d2742bcaac2586843eb50e126f34 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 29 Jan 2019 01:10:12 +0100 Subject: [PATCH 10/14] Small tip about the url field in manifest --- package_linter.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/package_linter.py b/package_linter.py index 3d46c38..d3e74bd 100755 --- a/package_linter.py +++ b/package_linter.py @@ -272,6 +272,9 @@ def check_manifest(path): if "type" not in install_arg: print_error("[YEP-2.1] You should specify the type of the key with %s" % (typ)) + 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...") + def check_verifications_done_before_modifying_system(script): """ From 7e4fecf6e6c8775bbbe34c5f3e42be94d976e96f Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 29 Jan 2019 01:36:53 +0100 Subject: [PATCH 11/14] Improve tip about the description --- package_linter.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/package_linter.py b/package_linter.py index d3e74bd..047615d 100755 --- a/package_linter.py +++ b/package_linter.py @@ -238,10 +238,16 @@ def check_manifest(path): # YEP 1.8 Publish test request # YEP 1.9 Document app - if "description" in manifest and "name" in manifest: - if manifest["description"] == manifest["name"]: - print_warning("[YEP-1.9] You should write a good description of the" - "app (1 line is enough).") + if "description" in manifest: + descr = manifest["description"] + if isinstance(descr, dict): + descr = descr.get("en", None) + + if descr is None or descr == manifest.get("name", None): + print_warning("[YEP-1.9] You should write a good description of the""app, at least in english (1 line is enough).") + + elif "for yunohost" in descr.lower(): + print_warning("[YEP-1.9] The 'description' should explain what the app actually does. No need to say that it is 'for YunoHost' - this is a YunoHost app so of course we know it is for YunoHost ;-).") # TODO test a specific template in README.md From 247c78c95ce221442fe14ccd3f8100000a8ba36a Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 29 Jan 2019 01:42:03 +0100 Subject: [PATCH 12/14] Fix broken link --- package_linter.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package_linter.py b/package_linter.py index 047615d..cfe6e3f 100755 --- a/package_linter.py +++ b/package_linter.py @@ -132,8 +132,9 @@ def check_source_management(app_path): print_warning("[YEP-3.3] Upstream app sources shouldn't be stored on this " "'sources' folder of this git repository as a copy/paste." "\nAt installation, the package should download sources " - "from upstream via 'ynh_setup_source'.\nSee " - "https://dev.yunohost.org/issues/201#Conclusion-chart") + "from upstream via 'ynh_setup_source'.\nSee the helper" + "documentation. Original discussion happened here : " + "https://github.com/YunoHost/issues/issues/201#issuecomment-391549262") def license_mentionned_in_readme(path): From a31bd52de27e2efb9f2d960fdd6167a508630c3e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 5 Feb 2019 17:01:13 +0100 Subject: [PATCH 13/14] Fix known service list --- package_linter.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package_linter.py b/package_linter.py index cfe6e3f..6ef8001 100755 --- a/package_linter.py +++ b/package_linter.py @@ -262,8 +262,9 @@ def check_manifest(path): "[YEP-2.1] \"multi_instance\" field must be boolean type values 'true' or 'false' and not string type") if "services" in manifest: - services = ("nginx", "php5-fpm", "mysql", "uwsgi", "metronome", - "postfix", "dovecot") # , "rspamd", "rmilter") + services = ("nginx", "mysql", "uwsgi", "metronome", + "php5-fpm", "php7.0-fpm", "php-fpm", + "postfix", "dovecot", "rspamd") for service in manifest["services"]: if service not in services: From 6bddff93d21a2d0bc9045f39f744ff1316a759ab Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 7 Feb 2019 15:06:28 +0100 Subject: [PATCH 14/14] Typo --- package_linter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package_linter.py b/package_linter.py index 6ef8001..69662f5 100755 --- a/package_linter.py +++ b/package_linter.py @@ -245,7 +245,7 @@ def check_manifest(path): descr = descr.get("en", None) if descr is None or descr == manifest.get("name", None): - print_warning("[YEP-1.9] You should write a good description of the""app, at least in english (1 line is enough).") + print_warning("[YEP-1.9] You should write a good description of the app, at least in english (1 line is enough).") elif "for yunohost" in descr.lower(): print_warning("[YEP-1.9] The 'description' should explain what the app actually does. No need to say that it is 'for YunoHost' - this is a YunoHost app so of course we know it is for YunoHost ;-).")