From 807f9aa7003838086163d04a2be3b4196a2e8b58 Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Wed, 8 Apr 2026 00:39:16 +0200 Subject: [PATCH 1/2] fedora-review: allow skipping checks This patch adds support for completely skipping some checks for all packages. For example, there is no reason to ever run `CheckNoNameConflict`. It also adds support for user-defined list of checks that should be skipped only for the package that defines them. Some packages has FESCo-approved exceptions and can violate some things that are normally a MUST. We also need to be prepared for false-positives or bugs in the checks. Users can put their configuration in `fedora-ci.toml`, e.g.: [tools.fedora-review.toml] exclude = "CheckCodeAndContent,CheckBundledLibs,CheckPythonBuildRequires" --- plans/fedora-review/main.fmf | 8 ++ tests/fedora-review/main.fmf | 2 + tests/fedora-review/run-fedora-review.py | 103 ++++++++++++++++++++--- tests/fedora-review/viewer.html | 4 +- 4 files changed, 104 insertions(+), 13 deletions(-) diff --git a/plans/fedora-review/main.fmf b/plans/fedora-review/main.fmf index 43a4dfd..2a1c260 100644 --- a/plans/fedora-review/main.fmf +++ b/plans/fedora-review/main.fmf @@ -16,3 +16,11 @@ provision: discover: how: fmf test: /tests/fedora-review + +# TODO Remove the prepare section once this PR +# https://forge.fedoraproject.org/packaging/FedoraReview/pulls/551 +# is merged and released +prepare: + how: artifact + provide: + - copr.build:10883634:fedora-44-x86_64 diff --git a/tests/fedora-review/main.fmf b/tests/fedora-review/main.fmf index 6c233c2..e07e9d1 100644 --- a/tests/fedora-review/main.fmf +++ b/tests/fedora-review/main.fmf @@ -9,6 +9,8 @@ require: - koji - git + - python3-ruamel-yaml + - python3-tomli-w test: python3 ./distgit-prepare.py adjust: # TODO: For now this is only available for distgit commits diff --git a/tests/fedora-review/run-fedora-review.py b/tests/fedora-review/run-fedora-review.py index 6a606cb..942a1a1 100644 --- a/tests/fedora-review/run-fedora-review.py +++ b/tests/fedora-review/run-fedora-review.py @@ -9,6 +9,10 @@ from enum import Enum import json import yaml +import utils +import tomli_w + +CI_CONFIG_SECTION = "fedora-review" # Expose these to the users FEDORA_REVIEW_RESULTS = [ @@ -27,7 +31,7 @@ class Result(Enum): PASS = "pass" -def dump_results_yaml(issues: int): +def dump_results_yaml(issues: int, skipped: int): """ https://tmt.readthedocs.io/en/stable/spec/results.html """ @@ -36,8 +40,11 @@ def dump_results_yaml(issues: int): { "name": "/", "result": result.value, - "note": [f"{issues} issues"], - "log": ["viewer.html"] + FEDORA_REVIEW_RESULTS, + "note": [ + f"{skipped} skipped", + f"{issues} issues", + ], + "log": ["viewer.html", "fedora-review.toml"] + FEDORA_REVIEW_RESULTS, } ] path = os.path.join(os.environ.get("TMT_TEST_DATA"), "results.yaml") @@ -72,6 +79,19 @@ def copy_viewer_html(): shutil.copy(viewer, Path(os.environ["TMT_TEST_DATA"]) / viewer) +def copy_mock_fedora_ci_toml(): + """ + Copy a mock fedora-ci.toml to the plan data directory + This is only for development purposes. In production a package either has + a fedora-ci.toml configuration in its repository or it doesn't. Either way, + we don't want to copy it from anywhere else. + """ + filename = "fedora-ci.toml" + print(f"Copying {filename} to the plan data") + dst = Path(os.environ["TMT_PLAN_DATA"]) / "dist-git" / filename + shutil.copy(filename, dst) + + def copy_data_into_data(): """ There is a weird bug that we discovered with @LecrisUT. For some reason, @@ -96,9 +116,20 @@ def fedora_review(spec_file, workdir): env["REVIEW_NO_MOCKGROUP_CHECK"] = "true" name = Path(spec_file).stem - cmd = ["fedora-review", "--prebuilt", "-n", name] + config = str(args.workdir / "fedora-review.toml") + cmd = ["fedora-review", "--config", config, "--prebuilt", "-n", name] print(f"Running: {" ".join(cmd)}") - subprocess.run(cmd, cwd=workdir, env=env, check=True) + proc = subprocess.run( + cmd, + cwd=workdir, + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + print(proc.stdout.decode("utf-8")) + print(proc.stderr.decode("utf-8")) + if proc.returncode: + raise RuntimeError("The fedora-review command failed") path = os.path.join(workdir, "review-" + name, "review.json") if not os.path.exists(path): @@ -110,9 +141,46 @@ def fedora_review(spec_file, workdir): return review -def count_issues(review): - issues = review.get("issues", []) - return len(issues) +def skip_checks(config): + skip_for_all = [ + # A package with this name obviously already exists in the Fedora + # repositories and this is that package. Check for a name conflict only + # makes sense during the initial Package Review Process, but it does't + # make any sense for CI on existing packages. + "CheckNoNameConflict", + # The licensecheck implementation within the `fedora-review` tool is + # not up to modern standards and produces far to many false-positives + # which would be too annoying for our users. We discussed this with + # @msuchy and agreed that it would be better to have a dedicate service + # for checking licenses. It should be based around ScanCode Toolkit, + # FOSSology, or anything that succeeds them. + "CheckLicensInDoc", + "CheckLicenseField", + ] + skip_for_package = [] + if exclude := config.get("exclude"): + skip_for_package = [x.strip() for x in exclude.split(",")] + skip_for_package = [x for x in skip_for_package if x] + return skip_for_all + skip_for_package + + +def parse_fedora_review_toml(workdir: Path): + """ + Parse the fedora-review.toml out of the fedora-ci.toml + """ + dist_git_path = args.workdir / "dist-git" + if config := utils.get_config(dist_git_path, CI_CONFIG_SECTION): + return config["toml"] + return {} + + +def dump_fedora_review_config(fedora_review_config): + name = "fedora-review.toml" + path: Path = args.workdir / name + with path.open("wb") as fp: + tomli_w.dump(fedora_review_config, fp) + print(f"Copying {name} to the test results") + shutil.copy(path, Path(os.environ["TMT_TEST_DATA"]) / name) def main(args: argparse.Namespace) -> None: @@ -129,14 +197,27 @@ def main(args: argparse.Namespace) -> None: # we just need to copy the .spec next to them shutil.copy(args.spec_file, args.workdir) + # Uncomment if needed for development purposes + # copy_mock_fedora_ci_toml() + + # Parse the `fedora-review config` aout of the `fedora-ci.toml`, update + # the list of excluded checks and save it as `fedora-review.toml`. + config = parse_fedora_review_toml(args.workdir) + skip = skip_checks(config) + config["exclude"] = ",".join(skip) + dump_fedora_review_config(config) + print(f"Skipping these checks: {skip}") + review = fedora_review(args.spec_file, args.workdir) - issues = count_issues(review) - dump_results_yaml(issues) + issues = review.get("issues", []) + + dump_results_yaml(len(issues), len(skip)) copy_fedora_review_results(args.spec_file, args.workdir) copy_viewer_html() copy_data_into_data() - print(f"Found {issues} issues") + print(f"Skipped {len(skip)} issues") + print(f"Found {len(issues)} issues") if issues: sys.exit(1) diff --git a/tests/fedora-review/viewer.html b/tests/fedora-review/viewer.html index 033f66e..9388df6 100644 --- a/tests/fedora-review/viewer.html +++ b/tests/fedora-review/viewer.html @@ -70,7 +70,7 @@

} from "https://cdn.jsdelivr.net/gh/lit/dist@2/core/lit-core.min.js"; function getReviewIssues(report) { - return report["issues"]; + return report["issues"] ?? []; } function renderIssue(issue) { @@ -87,7 +87,7 @@

const issues = getReviewIssues(report); const summary = []; - if (issues?.length === 0) { + if (issues.length === 0) { summary.push(html`
No issues were found
`); } else { issues.forEach((issue) => { From 6a5504213f9348027a644d9f8d360e0d54b1f90d Mon Sep 17 00:00:00 2001 From: Jakub Kadlcik Date: Thu, 20 Aug 2026 22:09:11 +0200 Subject: [PATCH 2/2] fedora-review: drop temporary workaround for missing viewer.html Seems like the bug was resolved, so we don't need the hack anymore --- tests/fedora-review/run-fedora-review.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/tests/fedora-review/run-fedora-review.py b/tests/fedora-review/run-fedora-review.py index 942a1a1..7078f5a 100644 --- a/tests/fedora-review/run-fedora-review.py +++ b/tests/fedora-review/run-fedora-review.py @@ -92,22 +92,6 @@ def copy_mock_fedora_ci_toml(): shutil.copy(filename, dst) -def copy_data_into_data(): - """ - There is a weird bug that we discovered with @LecrisUT. For some reason, - when a plan has `result: custom`, the `viewer.html` stops rendering in - Testing Farm. It is because for some reason, Oculus starts looking for it - in `data/data/viewer.html` instead of just `data/viewer.html`. - This is IMHO a bug but either way, until it gets resolved, we can copy the - data there as well. - See https://gitlab.com/testing-farm/general/-/work_items/111 - """ - shutil.copytree( - Path(os.environ["TMT_TEST_DATA"]), - Path(os.environ["TMT_TEST_DATA"]) / "data", - ) - - def fedora_review(spec_file, workdir): """ Run fedora-review @@ -214,7 +198,6 @@ def main(args: argparse.Namespace) -> None: dump_results_yaml(len(issues), len(skip)) copy_fedora_review_results(args.spec_file, args.workdir) copy_viewer_html() - copy_data_into_data() print(f"Skipped {len(skip)} issues") print(f"Found {len(issues)} issues")