SIGs/Package Review

(Need a fancy header)

Fedora Package Review SIG

Who Are We?

This SIG is currently in its infancy and things are still being organized.

• Our job is to process new package review submissions and review them for quality and adherence to the Packaging Guidelines. As this is the initial experience for many new contributors to the Fedora Project, we also work towards making this procedure as enjoyable as possible.

• Anyone is welcome to join. Obviously it is necessary for one to already be a packager in order to review packages, but there are many tasks that can be done by anyone, and participation in the SIG is a good path to sponsorship.

Members

• Jason Tibbitts
• Rahul Sundaram
• Emmanuel Seyman
• Lakshmi Narasimhan T V
• Thomas Spura
• Gwyn Ciesla
• Stanislav Ochotnicky
• Richard Shaw
• Ankur Sinha "FranciscoD"
• Alexander Kurtakov
• Ryan Rix
• Jorge Gallegos
• Tim Lauridsen
• Adam Huffman
• Volker Fröhlich
• Denis Arnaud
• Petr "contyk" Sabata
• Mario Blättermann
• Devin Bost
• Siddharth Waikar
• Igor Gnatenko
• Chase Southwood

Getting Involved

Need some more structure before we can do much more. However, the Package review ticket reports are a good starting point for anyone looking for packages to review.

Scope of Action

• Actually reviewing packages.
• Coordinating reviews of difficult or large packages that may be too much for one person.
• Review triage (ensuring the form of the submission is correct, making sure the package builds and providing rpmlint output).
• Assisting new contributors in doing pre-reviews.
• Investigating modifications to the review process and initial review submission requirements.
• Generating reports and reporting to upstream committees.

Brainstorming

Since we're not yet organized, a communal brain dump can help get us started.

As a reminder, signing is easy as writing four tildes (i.e., the '~' sign four times) at the end of the post. It will automatically add your name and a date-time stamp. To have fancier signatures, you can check that Wikimedia how-to.

• Should we require (or perhaps strongly suggest) koji scratch builds to be provided with the initial submission? A significant percentage of the packages I look at simply don't build. Tibbs 17:05, 20 August 2008 (UTC)
  • Generally recommended IMO. Usually I reject review requests with srpms which don't build on koji with the comment "this doesn't build". However the problem is that this cannot be applied for requests by new contributors seeking for sponsors (currently I give priority over reviewing NEEDSPONSOR blockers). Mamoru 17:30 2008-08-20
  • I think it should be required for review requests for contributors that have already been sponsored. bpepple
  • Yes. I also think we should ask contributors who are already sponsored to give koji scratch builds with initial submission. Another thing I would like to suggest is to provide rpmlint output on SRPM as well as RPMS for newer package initial submission. This will also save contributor and reviewer's time in package review. -Paragn
  • I think it's a good idea but I wouldn't tie it down to koji. I do test builds with mock on my workstation before submitting a review which accomplishes the same result. Maybe it would be better to have an indirect requirement that there be rpmlint output posted to the review for at least one (supported) release and architecture. Richard
  • Reviewers should try building the package as well so this is quite pointless. Also, people sometimes submit packages that depend on other reviews and therefore can't be built at all (yet). contyk

• How about providing a review template file less experienced reviewers can download from the wiki and use as a basis for their final formal package review? Petersen 2008-10-03
  • This would help people like me who would like to help more (and feel bad about not reviewing nearly as many packages as they submit), but feel like their earlier reviews were dismissed as unhelpful. Konradm 20:52, 9 December 2008 (UTC)
  • Note however that not all packages are created equal and for example we have special review template for Java packages that doesn't apply at all to other type of rpms Sochotni 10:40, Jul 27, 2011 (UTC)
    • I have found it easiest to treat language-specific guidelines as add-ons that one can simply append to one's usual review checklist. (An example) --Gholms 21:09, 27 July 2011 (UTC)
    • I have done the same. Perhaps if we end up codifying some recommended templates, we can produce a tool that provides an appropriate one given a source package. Tibbs 21:17, 27 July 2011 (UTC)
    • In that case I'd like the basic template not to contain references specific to "normal" project (i.e. *so handling and such). Make it really modular. Big chunks of common review templates are useless in Java for example (I used to tick half the boxes as "non-applicable" before creating our template) Sochotni 09:30, 23 August 2011 (UTC)
• How about allowing pointing to public git repo in review request instead of srpm? I'm thinking about the following benefits:
  • Use fedpkg local and other similar tools directly on the clone
    • I might be mistaken, but fedpkg local depends on sources to be in the lookaside cache. Can new packagers (i.e. non-sponsored packagers) upload sources into the lookaside cache? Am I wrong? --jerboaa
  • Have the whole history kept in the scm and pushing the whole repo to Fedora repo once it's provisioned
  • Get the contributors used to the Fedora way of doing things even during the review
  • Incorporating sochotni's idea about per repo rpmlint ignore and commit hook so certain commits are blocked/warned about thus warning submitter earlier and saving work for reviewer --Akurtakov

• Regarding new packagers, in the list, I see a which only have submitted one package. For the new packagers that I am following, I ask them to submit two reviews and if possible for a little bit different type of packages. This imho shows better their understanding of the guidelines. Would this be something worth advising ? --Pingou 07:19, 28 July 2011 (UTC)
• I am working on a Review Helper tool to make it easier and faster to review a package -- Tim Lauridsen 2011-07-28
  • I have been trying Tim Lauridsen's FedoraReview tool and can confirm that it's useful--Verdurin 16:49, 28 July 2011 (UTC)

• I have added a sub-section on my page referencing material about how the Git work-flow can efficiently support the packaging efforts, with the corresponding discussions on Debian side. For instance, Git would be used to store the package source, on top of which the potential patches would simply be mere Git commits. Hence, it is trivial to work on patches, as well as to extract and review a list of patches. Moreover, the RPM specification would get simpler, because not only the patches would disappear, but also all the manipulations on files and directories (e.g., changing permissions, converting files from DOS to Unix end-of-lines, renaming directories, sedding/awking files, etc). -- darnaud (Talk - Contrib) 20:09, 6 August 2011 (UTC)
  • As suggested by Richar W.M. Jones in his blog, it would also allow "getting rid of the RPM %changelog and populate it from somewhere sensible instead".
  • Thanks to patches in connection with rpm.org ticket 54, we can probably expect further simplification for that Git work-flow. --Jpokorny (talk) 12:56, 23 August 2012 (UTC)

• Run rpmlint against the installed pacakge(s).
  • This can be done in mock so you don't have to install the packages on your main system and is useful for catching problems like undefined-non-weak-symbol which I believe can not be checked until the package is installed and ldconf run.

   $ mock --init
   $ mock --install <packages> rpmlint
   $ mock --shell
   mock-chroot> rpmlint <package1> <package2> ...

• We need a policy for outdated review requests. Some of them are quite old. If a request (which is a bug report anyway) becomes older than xxx days, we should test if the reporter responses within a reasonable time frame. If he/she doesn't response, the bug report should be closed for the time being. This would help us the "New package review tickets" list to let shrink a bit, to see only the really active review requests. -- mariobl