Running fedora-review on every commit

Project Discussion
1

Hello everyone,

a few years ago, I launched the Fedora Review Service over the Christmas break. It was a success, so this year I implemented another packaging-related idea that I’ve been sitting on for a while.

I’d like to work towards running the fedora-review tool for every commit for every (or at least opt-in) packages.

Motivation

We have a strong Package Review Process. Before a package is added to the official Fedora repositories, it is often rigorously scrutinized, and is only accepted after it meets our standards for quality. This relies on a heavy investment from both the contributor and the reviewer, but it is worth it because once the package gets accepted we know it is good enough.

The problem is, we have no idea what happens after that. We let the package(r) live on their own. Technically, I could revert all the changes that were recommended to me during the review, because they were too annoying, and nobody may notice. I guess that’s not a common scenario, though. What bothers me more is that I unknowingly let my packages decay out of my own ignorance. I maintain some sufficiently complicated packages and sometimes when updating to a new upstream version, I need to do non-trivial changes. I do them with the best intentions, but I have no idea whether they are correct or whether they’d pass the initial package review. Also, the best practices evolve as we invent new macros and stop using old ones.

And so, my packages probably get worse and worse every year.

Proof of concept

Currently, we use Pagure for our DistGit, but it is a done deal, that we are moving to Forgejo. I didn’t want to waste time with considering solutions that would work for Pagure, so my PoC is more future-oriented than I would normally like.

I created this Forgejo action - GitHub - FrostyX/fedora-review-ci

It can be easily used as a workflow by any other repository. Then, it takes a specfile within that repository, builds a SRPM out of it, and runs the fedora-review tool on it.

Since we don’t have a testing Forgejo instance for our future DistGit yet, I created a fake repository on GitHub that pretends to be a DistGit repository for the hello package for our purposes - GitHub - FrostyX/fedora-review-ci-hello

On the last commit, I got this CI failure:

Run cat /tmp/fedora-review/review-light/review.json |jq '[.results.MUST.Generic[] | select(.result == "fail")]'
[
  {
    "result": "fail",
    "text": "Permissions on files are set properly.",
    "note": "See rpmlint output",
    "name": "CheckFilePermissions",
    "url": "https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions"
  },
  {
    "result": "fail",
    "text": "If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license.",
    "note": "License file COPYING is not marked as %license",
    "name": "CheckLicensInDoc",
    "url": "https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text"
  }
]
Run test "2" -eq 0
Error: Process completed with exit code 1.

Random thoughts

  • Please see action.yml in the first repository to see what exactly it does
  • I want to run and report only MUST errors and ignore SHOULD and EXTRA to avoid being too pedantic
  • I want to report the errors in a more user-friendly way, e.g. comment in a pull request, or using whatever is the best practice for reporting errors in Forgejo Actions
  • We already do scratch builds for our PRs. I want to make this action dependent on those, so that this action could simply download the already built RPM results instead of building them for the second time
  • Examples are hosted on GitHub because we AFAIK don’t have any Forgejo instance with hosted action runners. But I wrote them based on the Forgejo Action documentation, not GitHub documentation, so they should work once we move to Forgejo
  • This is not an official Fedora Change Proposal and this is not something I’d want to force on us, no matter the cost. If we decide to continue, there will be a lot of time, official announcements, and it would definitely start as an opt-in.

What’s next?

I mainly wanted to start a discussion and see your thoughts about the topic. Maybe somebody else is already working on this, maybe this is a bad idea, maybe there is a better approach. I can easily see an alternative implementation for this using Packit and Testing Farm.

In case nobody weighs in, my plan would be continuing to work on this action, wait for the Forgejo migration, enable it for my own packages, allow anyone to do the same, try it for a couple of months, make the user experience nice for myself, and then suggest this for all Fedora packages with an option to opt-out.

5 Likes
2

I like the idea of doing this in general. I think perhaps it might be a better fit for it to be in packit instead of seperate though.

It could easily be opt in at first and advisory, then gradually be made default / blocking.

1 Like
3

I’m always in favour of things like this, that can catch errors, or non-standard things in packaging, as it just generally makes things better, when our packaging all sort of follows the same rules.

4

I have some issues for which I would like to understand more about the proposal:

  • Would the review be done at the current review levels, or the level that was performed at the time of the original review (we have improved/tightened the review over time)
  • Some packages have various approved exceptions, or special needs. How would that be annotated?
  • When releng does a mass rebuild, this could have substantial impact if the review process changes certain things from SHOULD to MUST (or a new criteria is added).

It would seem that the steps should be (which is somewhat equivalent, but not exactly, the same as the SPDX changes):

  • Someone (you?) runs the checks against all current packages
  • Someone (you?) open bugzillas and PRs, with the proposed fixes, for all current packages that need to be updated (packagers should be given help by the proposer)
  • Someone (you?) report the status (including percentage complete) of the approval/changes over some period of time
  • At some future point those packages not fixed should be proposed for retirement

Would it be better to start with adding the check to the bodhi CI stream (with it not being required)?

5
  • I would love to see this as a Packit job. And configured via packit.yaml.
  • I would love to get reports only if there is a new warning. I.e. if I previously ignored MUST item (for a reason) then I do not want to be nagged about it on every consequent commit.
6

Instead of full review again and again which frankly is quite wordy, would it be possible to do a diff?

However, the question is what would be the baseline. Maybe previous official build for specific branch?

7

Thank you all for the feedback,

I’ve also been considering that option and msuchy thinks it would be better too. I am going to explore it more.

Agree, there must be a way to ignore checks

The full review output is too long, I agree. But I actually don’t want to bother the user with the full output, only the found errors. I know there are many manual checks in the fedora-review output, those would be ignored. We would count only errors and show only those in the UI. Similarly to what the Fedora Review Service shows, e.g.:

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
- Systemd user unit service file(s) in phrog
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_user_units

Please know that there can be false-positives.

(randomly taken from 2427087 – Review Request: phrog - Mobile-friendly greeter for greetd)

Potentially we could also print failed SHOULD checks as a warning. So still a green check mark for the CI but a comment for the packager. The idea is that some SHOULD checks could become MUST checks in the future, so this could be a heads-up. But I didn’t think this one through yet.

This is a really good question, Gary. My idea is that the latest version of the fedora-review would be used, therefore every CI run would be done with the most up-to-date suite of checks. Potentially running additional checks compared to the initial package review.

Which, I understand, sound annoying. You don’t change anything, and your CI starts failing. I hate when linters for programming languages behave this way and suddenly force you to change 500 lines because a new syntax sugar was introduced. It only causes bugs, and the previous code was perfectly fine. But I think this is a different situation. The fedora-review CI as I am proposing, would be limited to only MUST items, so if it starts suddenly failing, we really should tend to it because as is, the package could not be added to Fedora now.

We will of course need to be careful when adding new MUST items. But I think we already are.

In case we move to Packit as others suggest, then we would add some config option for ignoring checks in packit.yaml

True, so we should probably think it through.

msuchy handled the SPDX migration superbly and set a great example how to add a new MUST item for packages. Another great example is the Python team. Their process would IMHO go well with my proposal. At some point, they open a ticket and file a PR. Much later, a new MUST check is added. If you merged the PR and fixed the ticket, you shouldn’t even notice.

8

As any new proposal for adding a MUST to the review will now need to go through the formal Fedora Fesco SystemWide Change approval proposal (with a package wide check with bugzillas and PRs in advance, which means probably over a year of effort), I suspect this will result in substantial lowering of interest for proposers in adding additional MUSTs (unless there is some substantial tooling improvements that such a proposal can be evaluated without substantial effort). That may, in fact, be an overall negative for the review process.