fedora-review: allow skipping checks by FrostyX · Pull Request #34 · packit/tmt-plans

FrostyX · 2026-05-03T14:12:14Z

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.

FrostyX · 2026-08-20T21:45:29Z

Users can now put their configuration in fedora-ci.toml, e.g.:

[tools.fedora-review.toml]
exclude = "CheckCodeAndContent,CheckBundledLibs,CheckPythonBuildRequires"

I don't have any real package using the configuration, so I am testing locally by uncommenting the copy_mock_fedora_ci_toml() line and it seems to work fine. In TF we are successfully skipping and rendering that information as well.

$ testing-farm request \
    --git-url https://github.com/FrostyX/tmt-plans.git \
    --git-ref fedora-review-3 \
    --plan /plans/fedora-review \
    -e KOJI_TASK_ID=149283758 \
    --context initiator=fedora-ci \
    --context trigger=commit \
    --compose Fedora-latest 

📦 repository https://github.com/FrostyX/tmt-plans.git ref fedora-review-3 test-type fmf
💻 Fedora-latest on x86_64
🔎 api https://api.dev.testing-farm.io/v0.1/requests/69f56a46-7ecf-4e60-90b9-4e5adace1f9a
💡 waiting for request to finish, use ctrl+c to skip
👶 request is waiting to be queued
🚀 request is running
🚢 artifacts https://artifacts.dev.testing-farm.io/69f56a46-7ecf-4e60-90b9-4e5adace1f9a
❌ tests failed
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━┓
┃ id                                   ┃ state    ┃ artifacts                                                                 ┃ overall ┃ arches_requested ┃ errored_plans ┃ failed_plans ┃ skipped_plans ┃ passed_plans ┃ incompleted_plans ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━┩
│ 69f56a46-7ecf-4e60-90b9-4e5adace1f9a │ complete │ https://artifacts.dev.testing-farm.io/69f56a46-7ecf-4e60-90b9-4e5adace1f… │ failed  │ x86_64           │ 0             │ 1            │ 0             │ 0            │ 0                 │
└──────────────────────────────────────┴──────────┴───────────────────────────────────────────────────────────────────────────┴─────────┴──────────────────┴───────────────┴──────────────┴───────────────┴──────────────┴───────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━┓
┃ plan                 ┃ x86_64 ┃
┡━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━┩
│ /plans/fedora-review │ fail   │
└──────────────────────┴────────┘

We still need to wait until the fedora-review PR with config support is merged and released
https://forge.fedoraproject.org/packaging/FedoraReview/pulls/551

But I think this is ready for a review.

LecrisUT · 2026-08-20T21:53:57Z

Quick note, can you look at the failures in #40 (comment). If it's an easy fix we can squeeze it in, otherwise let's track it.

LecrisUT · 2026-08-21T01:45:58Z

-    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")


Why this change? It seems to do the same thing as before, but with extra steps. You could just try-catch if what you want a different exception message.

Because with the previous version, the logs show

Test invocation process spawned with pid 1713057. stdout: INFO: Processing local files: hatch stdout: INFO: Getting .spec and .srpm Urls from : Local files in /var/tmp/tmt/run-021/plans/fedora-review/data stdout: INFO: --> SRPM url: file:///var/tmp/tmt/run-021/plans/fedora-review/data/hatch-1.18.0-1.eln159.src.rpm stdout: INFO: --> Spec url: file:///var/tmp/tmt/run-021/plans/fedora-review/data/hatch.spec stdout: INFO: Using review directory: /var/tmp/tmt/run-021/plans/fedora-review/data/review-hatch stdout: WARNING: No disttag found in prebuilt packages stdout: INFO: Use --define DISTTAG to set proper dist. e. g. --define DISTTAG fc21. stdout: ERROR: 'No disttag in package and no DISTTAG flag. Use --define DISTTAG to set proper dist e. g., --define DISTTAG=fc21.' (logs in /root/.cache/fedora-review.log) stdout: Copying fedora-review.toml to the test results stdout: Skipping these checks: ['CheckNoNameConflict', 'CheckLicensInDoc', 'CheckLicenseField'] stdout: Running: fedora-review --config /var/tmp/tmt/run-021/plans/fedora-review/data/fedora-review.toml --prebuilt -n hatch stdout: Traceback (most recent call last): stdout: File "/var/tmp/tmt/run-021/plans/fedora-review/discover/default-0/tests/tests/fedora-review/./run-fedora-review.py", line 238, in <module> stdout: main(args) stdout: ~~~~^^^^^^ stdout: File "/var/tmp/tmt/run-021/plans/fedora-review/discover/default-0/tests/tests/fedora-review/./run-fedora-review.py", line 196, in main stdout: review = fedora_review(args.spec_file, args.workdir) stdout: File "/var/tmp/tmt/run-021/plans/fedora-review/discover/default-0/tests/tests/fedora-review/./run-fedora-review.py", line 117, in fedora_review stdout: subprocess.run(cmd, cwd=workdir, env=env, check=True) stdout: ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ stdout: File "/usr/lib64/python3.14/subprocess.py", line 577, in run stdout: raise CalledProcessError(retcode, process.args, stdout: output=stdout, stderr=stderr) stdout: subprocess.CalledProcessError: Command '['fedora-review', '--config', '/var/tmp/tmt/run-021/plans/fedora-review/data/fedora-review.toml', '--prebuilt', '-n', 'hatch']' returned non-zero exit status 1. stdout: Shared connection to 127.0.0.1 closed.^M Command returned '1' (failure).

with the ugly traceback, and now the logs show just:

Test invocation process spawned with pid 1685990. stdout: The fedora-review command failed stdout: Copying fedora-review.toml to the test results stdout: Skipping these checks: ['CheckNoNameConflict', 'CheckLicensInDoc', 'CheckLicenseField'] stdout: Running: fedora-review --config /var/tmp/tmt/run-020/plans/fedora-review/data/fedora-review.toml --prebuilt -n hatch stdout: stdout: INFO: Processing local files: hatch stdout: INFO: Getting .spec and .srpm Urls from : Local files in /var/tmp/tmt/run-020/plans/fedora-review/data stdout: INFO: --> SRPM url: file:///var/tmp/tmt/run-020/plans/fedora-review/data/hatch-1.18.0-1.eln159.src.rpm stdout: INFO: --> Spec url: file:///var/tmp/tmt/run-020/plans/fedora-review/data/hatch.spec stdout: INFO: Using review directory: /var/tmp/tmt/run-020/plans/fedora-review/data/review-hatch stdout: WARNING: No disttag found in prebuilt packages stdout: INFO: Use --define DISTTAG to set proper dist. e. g. --define DISTTAG fc21. stdout: ERROR: 'No disttag in package and no DISTTAG flag. Use --define DISTTAG to set proper dist e. g., --define DISTTAG=fc21.' (logs in /root/.cache/fedora-review.log) stdout: stdout: Shared connection to 127.0.0.1 closed.^M Command returned '1' (failure).

Ah CalledProcessError != RuntimeError. Try the pattern in rmdepcheck instead

That's also somehow weird

Test invocation process spawned with pid 3705330. stdout: INFO: Processing local files: hatch stdout: INFO: Getting .spec and .srpm Urls from : Local files in /var/tmp/tmt/run-030/plans/fedora-review/data stdout: INFO: --> SRPM url: file:///var/tmp/tmt/run-030/plans/fedora-review/data/hatch-1.18.0-1.eln159.src.rpm stdout: INFO: --> Spec url: file:///var/tmp/tmt/run-030/plans/fedora-review/data/hatch.spec stdout: INFO: Using review directory: /var/tmp/tmt/run-030/plans/fedora-review/data/review-hatch stdout: WARNING: I can't remove check: Check1 stdout: WARNING: I can't remove check: FooBarCheck stdout: WARNING: I can't remove check: Baz stdout: WARNING: No disttag found in prebuilt packages stdout: INFO: Use --define DISTTAG to set proper dist. e. g. --define DISTTAG fc21. stdout: ERROR: 'No disttag in package and no DISTTAG flag. Use --define DISTTAG to set proper dist e. g., --define DISTTAG=fc21.' (logs in /root/.cache/fedora-review.log) stdout: Copying fedora-ci.toml to the plan data stdout: Copying fedora-review.toml to the test results stdout: Skipping these checks: ['CheckNoNameConflict', 'CheckLicensInDoc', 'CheckLicenseField', 'Check1', 'FooBarCheck', 'Baz', 'CheckPythonBuildRequires'] stdout: Running: fedora-review --config /var/tmp/tmt/run-030/plans/fedora-review/data/fedora-review.toml --prebuilt --name hatch stdout: The fedora-review command failed

See how the "Running: fedora-review" line is after the actual failure?

LecrisUT · 2026-08-21T01:52:11Z

+    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",
+    ]


Could we put these in a fedora-review.toml file that is merged with possible data from the user? It would make it easier for the user to reference the format for also.

So we would run something like fedora-review --config fedora-review-generic.toml --config fedora-review-from-user.toml ...?

I am not opposed to it but it would require some additional changes on the fedora-review side so if you don't mind, I'd rather do it in a follow-up PR later. And for now maybe just document som how to exclude tests? Do you have a recommendation where?

LecrisUT · 2026-08-21T01:53:54Z

+    """
+    dist_git_path = args.workdir / "dist-git"
+    if config := utils.get_config(dist_git_path, CI_CONFIG_SECTION):
+        return config["toml"]


I think we can drop the toml part on this. rpmlint had it because it has 2 distinct helper files rpmlintrc and rpmint.toml, but for this one it is only one.

Then this is just a one-liner

return utils.get_config(args.workdir / "dist-git", CI_CONFIG_SECTION) or {}

I don't think we can because the config is a <class 'dict'> and looks like this

{'toml': {'mock_config': 'fedora-43-x86_64', 'cache': False, 'verbose': True, 'checksum': 'sha256', 'use_colors': True, 'exclude': 'Check1,FooBarCheck,Baz,CheckPythonBuildRequires'}}

so we need to go through the toml key first. Unless you want to bypass it on the utils level.

LecrisUT · 2026-08-21T01:58:39Z

+    parser.add_argument(
+        "--koji-task-id",
+        default=os.environ.get("KOJI_TASK_ID"),
+    )


This is not used?

LecrisUT · 2026-08-21T02:03:14Z

+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)


I'm thinking if these should be in the distgit-prepare.py. Reasoning being that we would have a different logic for copr, with different defaults as well.

I noticed that distgit-prepare.py does something related

tmt-plans/tests/rpmlint/distgit-prepare.py

Lines 26 to 39 in 8c265cf

def set_config_files(config: dict[str, Any], args: argparse.Namespace) -> None:

if rc_content := config.get("rc"):

rc_content: str

rc_file: Path = args.workdir / "rpmlintrc"

rc_file.write_text(rc_content)

with args.env_file.open("a") as f:

f.write(f"RPMLINT_RC_FILE={rc_file}\n")

if toml_content := config.get("toml"):

toml_content: dict[str, Any]

toml_file: Path = args.workdir / "rpmlint.toml"

with toml_file.open("wb") as f:

tomli_w.dump(toml_content, f)

with args.env_file.open("a") as f:

f.write(f"RPMLINT_TOML_FILE={toml_file}\n")

but I was thinking the exact opposite. I see distgit-prepare.py as a generic helper script that shouldn't be specific to any one test but rather useful for all of them. So IMHO there shouldn't be any fedora-review or rpmlint configuration handling inside that script and it should rather be done in the tests.

If there is some re-usable piece of code that all the tests should use, it should probably be in utils.

FrostyX · 2026-08-21T08:59:33Z

Quick note, can you look at the failures in #40 (comment). If it's an easy fix we can squeeze it in, otherwise let's track it.

Happened to me too, when I tried to test on Koji task which was for Fedora ELN. I'll try to fix it within this PR.

FrostyX · 2026-08-24T11:08:35Z

Quick note, can you look at the failures in #40 (comment). If it's an easy fix we can squeeze it in, otherwise let's track it.

Happened to me too, when I tried to test on Koji task which was for Fedora ELN. I'll try to fix it within this PR.

Turns out it is not that easy, I'll submit a separate PR. For me the bug happened because I tested on an ELN pacakge, I am not sure what is the problem in your 149244309 task yet. For example the task 149283758 with hatch-1.18.0-1.fc45 works fine.

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"

Seems like the bug was resolved, so we don't need the hack anymore

FrostyX force-pushed the fedora-review-3 branch from 1523a4d to ac3c01a Compare May 3, 2026 14:14

FrostyX commented May 3, 2026

View reviewed changes

Comment thread tests/fedora-review/run-fedora-review.py Outdated

FrostyX force-pushed the fedora-review-3 branch from bdf0afa to 19bf1fa Compare May 3, 2026 14:23

FrostyX mentioned this pull request May 24, 2026

Add generic config handling and move the generic code #36

Merged

FrostyX force-pushed the fedora-review-3 branch 8 times, most recently from 5168581 to 8c265cf Compare August 20, 2026 21:26

LecrisUT reviewed Aug 21, 2026

View reviewed changes

FrostyX force-pushed the fedora-review-3 branch from 0aef4d5 to 65aa793 Compare August 24, 2026 11:03

FrostyX added 2 commits August 24, 2026 14:15

fedora-review: drop temporary workaround for missing viewer.html

6a55042

Seems like the bug was resolved, so we don't need the hack anymore

FrostyX force-pushed the fedora-review-3 branch from 65aa793 to 6a55042 Compare August 24, 2026 12:15

	def set_config_files(config: dict[str, Any], args: argparse.Namespace) -> None:
	if rc_content := config.get("rc"):
	rc_content: str
	rc_file: Path = args.workdir / "rpmlintrc"
	rc_file.write_text(rc_content)
	with args.env_file.open("a") as f:
	f.write(f"RPMLINT_RC_FILE={rc_file}\n")
	if toml_content := config.get("toml"):
	toml_content: dict[str, Any]
	toml_file: Path = args.workdir / "rpmlint.toml"
	with toml_file.open("wb") as f:
	tomli_w.dump(toml_content, f)
	with args.env_file.open("a") as f:
	f.write(f"RPMLINT_TOML_FILE={toml_file}\n")

Uh oh!

Conversation

FrostyX commented May 3, 2026

Uh oh!

Uh oh!

FrostyX commented Aug 20, 2026

Uh oh!

LecrisUT commented Aug 20, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrostyX commented Aug 21, 2026

Uh oh!

FrostyX commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FrostyX commented Aug 24, 2026 •

edited

Loading