I realize we’ve had several discussions on what to do with our current Package Review Process, so my apologies for starting yet another one. But please bear with me.
On Flock 2026 we had a 1on1 discussion with @churchyard and brainstormed a rough idea what the Package Review Process could look like in (the near) future. The whole idea is based on a few key principles:
Intermediate repository on forge.fedoraproject.org where we would propose new packages into, but after being accepted, they would get removed from the repository and imported to DistGit
Proposing new packages via pull requests (with code comments, proper CI, etc)
Possibility to propose, test and accept multiple packages within one pull request
I elaborated on all of these points in my blog post, please give it a look.
I wasn’t sure if explaining the ideas was good enough, so I implemented a prototype and recorded a demo for you.
(I realize it is a bit on the longer side for a demo, but I didn’t want a heavily edited video that skips interesting parts, leaving them for the imagination)
The most common feedback that I got from the demo so far was that:
We need to submit the specfiles in subdirectories
We can’t submit specfiles into Copr. We need to submit SRPMs so that downstream patches etc are supported
I already implemented both of these features.
One other thing I want to point out is the codebase size. I realize that lines of code is a bad metric but what I want to show here is that the codebase is really small. Even if it doubled or tripled its size to get out of the prototype state to production, it would still be perfectly maintainable by one person.
I’d really like to get this to the finish line, so I am asking you for feedback. Do you like this approach? Do you see any blockers that are non-negotiable and need to be addressed before this could become a way of proposing new Fedora packages? Please let me know your thoughts.
I’d really like to get this to the finish line, so I am asking you for feedback. Do you like this approach? Do you see any blockers that are non-negotiable and need to be addressed before this could become a way of proposing new Fedora packages? Please let me know your thoughts.
Me too! This is a significant process improvement! I last looked at this
a week or two ago, so I’m not sure if of these details have changed, but
I think the main blockers would be:
Getting a dedicated Forge runner set up in the packaging org that is
only meant for use by the package reviews repo (I guess the repo would
be named packaging/reviews or packaging/review-process?). Currently, we
only have a shared runner for the entire packaging org that has
concurrency and job time limits, so that wouldn’t work well here.
Figuring out how to integrate this with the request repo process and
toddler. Currently, this all relies on Bugzilla and various checks that
the review bug is in the correct state before creating a package repo in
dist-git.
A couple other details:
Intermediate repository on forge.fedoraproject.org where we
would propose new packages into, but after being accepted, they would
get removed from the repository and imported to DistGit
I don’t think they need to be accepted/merged into the intermediate
repository at all. PRs can probably just be closed after the package is
approved and the dist-git repo is created.
The most common feedback that I got from the demo so far was that:
We need to submit the specfiles in subdirectories
We can’t submit specfiles into Copr. We need to submit SRPMs so
that downstream patches etc are supported
I don’t think we want people committing entire SRPMs to git. Or do you
mean something else by “submit SRPMs” here? It should be possible to
commit all the source files that are needed to build the SRPM to the git
repository in the subdirectory named after the package, like we do in
dist-git. For things like generated vendor archives, we probably need to
find a place other than the git repo to store these large files to be
used by the review process CI.
That’s true, but if we close all PRs without merging, it could look like all the packages were rejected. It’s a minor thing, but I think merging them will be a nicer experience for the contributors.
I am sorry, I worded it quite badly. What I mean is that the contributor will commit files like
and we will do rpmbuild -bs foo/foo.spec --define "%_sourcedir ./foo" and submit the resulting SRPM into Copr. Instead of simply submitting the specfile.
Having the automation add a green approved label and closing the merge request with a link to the created repo should be enough and would avoid increasing the size of the review process repo and making it take longer to clone/fork and create package review PRs against it.
That makes sense. Thanks. The issue of how to handle Source archives still remains. Take this example from the golist package:
Source0: %{url}/archive/v%{version}.tar.gz#/golist-%{version}.tar.gz
# Generated by go-vendor-tools
Source1: golist-%{version}-vendor.tar.bz2
Source2: go-vendor-tools.toml
Source0 can probably be downloaded from the upstream Forge and Source2 is a text file that can be committed to git. But what about the generated vendor archive? All Go packages use vendored dependency archives, and so do other ecosystems like nodejs. Can the review automation gain the ability to run a custom script to download/generate sources?
And now that I think about it, running custom scripts, rpmbuild -bs, or anything else that runs rpm parsing in the Forgejo action with pull_request_target is likely a security issue. Can we use the Copr custom build script feature (the one Packit uses) to run any untrusted code instead?
That’s an interesting trade-off that I didn’t think of.
Yep, thanks to --define "%_disable_source_fetch 0"
That’s a good idea, I’ll do that. Thanks.
I think @msuchy told me about some efforts to have a standardized download/generate script on the RPM level. I am not sure what the exact situation is, but I’ll look into details and maybe use that as a starting point.
But personally, I don’t have any experience with generated vendor archives in RPM packages so if anyone has ideas regarding this, please do tell
That was one of the main things I wanted to comment, but seems there are already ideas for it .
Overall I think it would be worth pursuing even just for the in-line code review, maybe can start with the basic support to figure out the nitty-gritty of how to implement it.
On to the more nitty-gritty stuff.
Multi-package reviews
I have some reservations of the approach of doing the multi-package review in the same PR primarily because: it becomes less searchable when trying to find duplicate reviews and would make merging part of the packages reviewed harder. The alternative to this would be to use multiple PRs with some dependency field specified.
On the other hand I do like it as a way to get better context on the dependencies without 100 clicks, so maybe we could have both?
Packit-CI, actions, something else
I would probably avoid actions for this to avoid reserving the runners while it would just be waiting for copr builds. The actions would be more visible and contributable, but I expect it to be a whole bunch of spaghetti-code at the end of it in any approach.
Between Packit-CI and some bespoke micro-service, there are a bunch logics needed to be implemented so I don’t think it would make much difference other than the long-term maintenance and the speed of iteration.
As a starting-point for the Packit-CI I think the handling of dist-git scratch-build would be the closest point to this workflow.
What are the specific actions needed to implement
Would probably be useful to track the individual ideas from your video (and some added by me as it comes to me) and see how far we tackle each one
Listening to PR changes and trigger copr-build
Multi-package review in a single PR workflow
Defining dependencies on other PRs or issues (like a package needing update)
Keeping the copr dependencies clean
Approving/Rejecting a review
ACL for the people allowed to approve
The base repo structure and making sure it is clean after the accepted review
Repo creation and import
Could also mirror the files under review with the final one (e.g. bundling scripts, rust2rpm.toml, rpminspect, rpmlint etc.)
Request multiple branches on import
Special case for packages that are available in only one branch like epel-only
Intermediate repository on forge.fedoraproject.org where we
would propose new packages into, but after being accepted, they would
get removed from the repository and imported to DistGit
I don’t think they need to be accepted/merged into the intermediate
repository at all. PRs can probably just be closed after the package is
approved and the dist-git repo is created.
Well, it will need to change when distgit is moved to forge, but short
term with the existing pagure setup, how about just making a ‘remote’
pull request on src.fedoraproject.org, ie:
approved review, checks run, repo created.
remote pr filed against the new project “initial import” that imports
everything, sets up the package, points back to the review, etc.
Once merged the review pr is closed pointing to the merged upstream
one?
Or is that too complicated? I think it nicely allows you to easily track
back from a package to it’s review, makes sure the import is all done
right and on the review shows where the package was imported.
Searchability is a good point, I didn’t think too much about that. Thanks. Personally, I’d like to avoid dependencies between PRs. It would IMHO be less intuitive for users and more complicated to implement. I’d rather focus on how to improve the searchability of multiple packages within one PR.
I like the idea of using Packit instead of Forgejo Actions, and I am prepared for it. All my logic is in a Python script and my current Forgejo Action only runs that script. So moving it into Packit will be easy.
That being said, I wanted to contribute to the Packit codebase a few times, and I didn’t have luck running the service locally. Can I plan a 1on1 with you, please, so that we can discuss where exactly I should put the code and how I can set up the development environment?
But before opening a PR, you need to create a DistGit repository and its branches so you have something to open it against. I wrote a script to do that
and since we need to run something like this, I added a code there to download the reviewed SRPM and import it to the repository as an initial commit. Personally, I like it better than a PR against the DistGit repo, because merging it would be an unnecessary additional step for the user.
My offer to implement something in spectool (or similar tooling) stands, but I’m kind of still waiting for confirmation that this is what people would actually use
Unfortunately you and I are in the same boat w.r.t. getting packit-service to work locally. But I was mostly doing it in the test adjustment context. @betulependule should be the most recent to look into that, maybe she has some fresh tips on that. If not I know @nforro has a working setup locally.
Until we settle on the standard solution via Making sure you're not a bot!, I implemented a very straightforward support for this feature
Once somebody (RPM, spectool, whoever) provides this feature for us, I’ll switch to that. But at least, until it happens, we can use this as a testing ground and get some feedback.
I still agree that the correct solution is using Packit instead of Forgejo Actions, but I get the sense that it will not be that easy. Especially when both of us tried to run the service localy and failed.
I wanted to move the prototype a bit further, so I finished the Action that I had. It was quite trivial to do - it was mainly about configuring the secrets. It works now:
That looks reasonable, but I would suggest implementing
generate-sources-dependencies.txt instead of requiring all deps to be
hardcoded. I think including only git-core and rpmdevtools by default
makes sense. For Go packages, we can update go2rpm to automatically
create generate-sources.sh and generate-sources-dependencies.txt.
Also, the go_vendor_archive commands in the gh example in the Codeberg
repo are need an explicit path to the config file:
Once somebody (RPM, spectool, whoever) provides this feature for us,
I’ll switch to that. But at least, until it happens, we can use this as
a testing ground and get some feedback.
The solution you implemented here is actually pretty reasonable, so I
wouldn’t be opposed to standardizing on that . Packit also
implemented an actions.post-modifications config in packit.yaml to
accommodate this use case that we utilize to both generate vendor
archives (go_vendor_archive) and verify licensing before uploading the
vendor archive to the lookaside cache (go_vendor_license). See https://src.fedoraproject.org/rpms/gh/blob/rawhide/f/.packit.yaml for
an example. It would be nice for packages not to have to specify both,
i.e., one config for the package review CI and one for Packit version
update PRs. Also, I like the generate-sources-dependencies.txt concept,
since for Packit, every possible dependency that could be needed to run
any project’s Packit actions needs to be hardcoded and bundled into the
sandcastle image.
Anyway, I think you’ve addressed all my feedback other than
The first one is pretty straightforward; you just need to file a ticket
in the forge/forge tracker using the runner resources request template.
The second one would probably be best implemented as a new toddler plugin that consumes Forge Fedora Messaging messages from the package review repository. Once a review PR has been approved, the author could type /request-repo [branches...] and the toddler would validate that the ticket was properly acked by a packager member, create the repo, and import the package. Or did you have a different plan? A separate service could handle dist-git imports, but there’s already infrastructure in place to run toddlers and we could probably share/reuse some code form the existing scm_request_processor toddler. It would be good to hear what releng thinks.
I think you are right, and we will have to do it, because we probably cannot cover everything for every ecosystem, that users might need. The other idea that I had was running dnf builddep to install the BuildRequires even when building just the SRPM. Do you think that would solve the issue?
Will do, thanks
I actually don’t like it that much because it is a separate script. I’d like to do it in the specfile itself. The discussion that @decathorpe linked is really interesting
Wow, I had no idea, thank you. That only proves that we need some standardized solution.
This will probably be my next step. But I also need to request some test repository on Forge, because I am currently testing it against a repo on codeberg.
Coincidentally, I had an off-list chat with @zlopez earlier today, about how to contribute this into the toddlers repo
I plan to listen Fedora Messaging for an “approval” from a package reviewer and create repo and branch request tickets in
and once they are created, import the package. I am still not sure what “approval” means though. If we are going to be merging the PRs, or of it is going to be some label. Or closed PR with some reason (if that’s suported on Forgejo).
