From adb0b9a2ea786ade903bdafddf38fb08d753d17e Mon Sep 17 00:00:00 2001 From: bbhtt Date: Wed, 6 Nov 2024 23:15:08 +0530 Subject: [PATCH] modules: Restructure git source checks - Drop module-{module_name}-source-git-local-path, url is already mandatory - Add module-{module_name}-source-git-no-tag-commit-branch when url is present but not branch, commit or tag. Slight rename of `module-{module_name}-source-git-no-commit-or-tag` [release] --- flatpak_builder_lint/checks/modules.py | 32 ++++++++++--------- .../staticfiles/exceptions.json | 2 +- tests/manifests/modules.json | 21 ------------ tests/manifests/modules_git_disallowed.json | 14 ++++---- tests/test_manifest.py | 19 ++++++----- 5 files changed, 34 insertions(+), 54 deletions(-) diff --git a/flatpak_builder_lint/checks/modules.py b/flatpak_builder_lint/checks/modules.py index 6cd02fd0..09f15eff 100644 --- a/flatpak_builder_lint/checks/modules.py +++ b/flatpak_builder_lint/checks/modules.py @@ -1,6 +1,12 @@ +import re + from . import Check +def _is_git_commit_hash(s: str) -> bool: + return re.match(r"[a-f0-9]{4,40}", s) is not None + + class ModuleCheck(Check): def check_source(self, module_name: str, source: dict) -> None: source_type = source.get("type") @@ -19,26 +25,22 @@ def check_source(self, module_name: str, source: dict) -> None: commit = source.get("commit") branch = source.get("branch") tag = source.get("tag") - branch_committish = False - - if branch: - if len(branch) != 40: - self.errors.add(f"module-{module_name}-source-git-branch") - else: - branch_committish = True - - if not branch_committish and not commit and not tag: - self.errors.add(f"module-{module_name}-source-git-no-commit-or-tag") - - if source.get("path"): - self.errors.add(f"module-{module_name}-source-git-local-path") - url = source.get("url") + if not url: self.errors.add(f"module-{module_name}-source-git-no-url") - elif not url.startswith("https:") and not url.startswith("http:"): + return + + if url and not url.startswith(("http://", "https://")): self.errors.add(f"module-{module_name}-source-git-url-not-http") + if not any([commit, branch, tag]): + self.errors.add(f"module-{module_name}-source-git-no-tag-commit-branch") + return + + if branch and not _is_git_commit_hash(branch): + self.errors.add(f"module-{module_name}-source-git-branch") + def check_module(self, module: dict) -> None: name = module.get("name") buildsystem = module.get("buildsystem", "autotools") diff --git a/flatpak_builder_lint/staticfiles/exceptions.json b/flatpak_builder_lint/staticfiles/exceptions.json index 811541c7..fb604d49 100644 --- a/flatpak_builder_lint/staticfiles/exceptions.json +++ b/flatpak_builder_lint/staticfiles/exceptions.json @@ -1007,7 +1007,7 @@ "finish-args-flatpak-spawn-access": "application requires ptyxis-agent for PTY setup on host" }, "org.gnome.Totem.Devel": { - "module-totem-source-git-no-commit-or-tag": "This is the development version of Totem, tracking upstream", + "module-totem-source-git-no-tag-commit-branch": "This is the development version of Totem, tracking upstream", "appstream-id-mismatch-flatpak-id": "appstream-cid predates this linter rule" }, "org.gimp.GIMP": { diff --git a/tests/manifests/modules.json b/tests/manifests/modules.json index 6954af14..ea543eb8 100644 --- a/tests/manifests/modules.json +++ b/tests/manifests/modules.json @@ -8,27 +8,6 @@ "-DCMAKE_BUILD_TYPE=Debug" ], "sources": [ - { - "type": "git", - "path": "." - }, - { - "type": "git", - "url": "ssh://foobar.git", - "tag": "foo", - "dest-filename": "foo/bar" - }, - { - "type": "git", - "url": "ssh://foobar2.git", - "branch": "master", - "commit": "1234567890abcdef1234567890abcdef12345678" - }, - { - "type": "git", - "url": "ssh://foobar3.git", - "branch": "1234567890abcdef1234567890abcdef12345678" - }, { "type": "archive", "url": "https://example.com/foo.tar.gz", diff --git a/tests/manifests/modules_git_disallowed.json b/tests/manifests/modules_git_disallowed.json index 4cc58d6b..86af4aa1 100644 --- a/tests/manifests/modules_git_disallowed.json +++ b/tests/manifests/modules_git_disallowed.json @@ -5,8 +5,7 @@ "name": "module1", "sources": [ { - "type": "git", - "url": "https://foobar2.git" + "type": "git" } ] }, @@ -23,7 +22,9 @@ "name": "module3", "sources": [ { - "type": "git" + "type": "git", + "url": "ssh://foobar2.git", + "tag": "foo" } ] }, @@ -32,8 +33,7 @@ "sources": [ { "type": "git", - "url": "ssh://foobar2.git", - "commit": "8bdc272f381e2633b96d1846994c7cb0f90f079f" + "url": "https://example.org/foobar2.git" } ] }, @@ -42,8 +42,8 @@ "sources": [ { "type": "git", - "url": "https://foobar2.git", - "branch": "branch_name" + "url": "https://example.org/foobar2.git", + "branch": "foobarfoo" } ] } diff --git a/tests/test_manifest.py b/tests/test_manifest.py index 48b45e77..33c7dac5 100644 --- a/tests/test_manifest.py +++ b/tests/test_manifest.py @@ -224,13 +224,6 @@ def test_manifest_finish_args_empty() -> None: def test_manifest_modules() -> None: - errors = { - "module-module1-source-git-no-commit-or-tag", - "module-module1-source-git-local-path", - "module-module1-source-git-no-url", - "module-module1-source-git-url-not-http", - } - warnings = { "module-module1-buildsystem-is-plain-cmake", "module-module1-cmake-non-release-build", @@ -238,10 +231,8 @@ def test_manifest_modules() -> None: } ret = run_checks("tests/manifests/modules.json") - found_errors = set(ret["errors"]) found_warnings = set(ret["warnings"]) - assert errors.issubset(found_errors) assert warnings.issubset(found_warnings) @@ -253,8 +244,16 @@ def test_manifest_modules_git_allowed() -> None: def test_manifest_modules_git_disallowed() -> None: ret = run_checks("tests/manifests/modules_git_disallowed.json") + errors = { + "module-module1-source-git-no-url", + "module-module2-source-git-no-url", + "module-module3-source-git-url-not-http", + "module-module4-source-git-no-tag-commit-branch", + "module-module5-source-git-branch", + } found_errors = set(ret["errors"]) - assert [x for x in found_errors if x.startswith("module-")] + for e in errors: + assert e in found_errors def test_manifest_exceptions() -> None: