fedora-review: allow skipping checks - #34
Open
FrostyX wants to merge 3 commits into
Open
Conversation
FrostyX
commented
May 3, 2026
FrostyX
force-pushed
the
fedora-review-3
branch
7 times, most recently
from
August 20, 2026 21:15
c075c81 to
5168581
Compare
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
August 20, 2026 21:26
5168581 to
8c265cf
Compare
Contributor
Author
|
Users can now put their configuration in [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 We still need to wait until the But I think this is ready for a review. |
Collaborator
|
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
reviewed
Aug 21, 2026
Comment on lines
-101
to
+116
| 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") |
Collaborator
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
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).
Collaborator
There was a problem hiding this comment.
Ah CalledProcessError != RuntimeError. Try the pattern in rmdepcheck instead
Comment on lines
+129
to
+143
| 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", | ||
| ] |
Collaborator
There was a problem hiding this comment.
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.
| """ | ||
| dist_git_path = args.workdir / "dist-git" | ||
| if config := utils.get_config(dist_git_path, CI_CONFIG_SECTION): | ||
| return config["toml"] |
Collaborator
There was a problem hiding this comment.
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 {}
Comment on lines
+230
to
+233
| parser.add_argument( | ||
| "--koji-task-id", | ||
| default=os.environ.get("KOJI_TASK_ID"), | ||
| ) |
Comment on lines
+151
to
+167
| 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) |
Collaborator
There was a problem hiding this comment.
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.
Contributor
Author
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. |
Co-authored-by: Cristian Le <github@lecris.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.