Hi,
lately, I stumbled upon a review, which I thought, wouldn't suffice. It looks like the following
name: ok summary: ok license: ok handling locale files: ok rpmlint output: only spelling warning Not needed BuildRequires: (names), please remove them in git.
APPROVED.
My question is: is this review sufficient, if not, where is it written down, that it isn't? I'm especially aiming to the form of this review.
I wasn't able to spot a requirement to write something like approved (or something else) on http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
Further more, there isn't anything said about how the reviewer should document his work. If we deny the requirement of documenting reviewer's operation, then just setting the approved flag conforms with the guidelines; This also claims, everything has been checked and is well done.
Am I missing something? Is there any need to clarify our review guidelines? Do we need something more documented? Do we trust our reviewers, so there's no need of bureaucracy? Why should/must I do more than just setting the flag or writing 7 catchwords?
Thanks
On Wed, Feb 8, 2012 at 1:26 PM, Matthias Runge mrunge@matthias-runge.de wrote:
Hi,
lately, I stumbled upon a review, which I thought, wouldn't suffice. It looks like the following
name: ok summary: ok license: ok handling locale files: ok rpmlint output: only spelling warning Not needed BuildRequires: (names), please remove them in git.
APPROVED.
My question is: is this review sufficient, if not, where is it written down, that it isn't? I'm especially aiming to the form of this review.
I wasn't able to spot a requirement to write something like approved (or something else) on http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
Further more, there isn't anything said about how the reviewer should document his work. If we deny the requirement of documenting reviewer's operation, then just setting the approved flag conforms with the guidelines; This also claims, everything has been checked and is well done.
Am I missing something? Is there any need to clarify our review guidelines? Do we need something more documented? Do we trust our reviewers, so there's no need of bureaucracy? Why should/must I do more than just setting the flag or writing 7 catchwords?
I'd expect to see at least:
https://fedoraproject.org/wiki/Spots_Review_Cheat_Sheet
But you're right, what you saw was pretty spartan.
-J
Thanks
Matthias Runge mrunge@matthias-runge.de mrunge@fedoraproject.org -- packaging mailing list packaging@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/packaging
"MR" == Matthias Runge mrunge@matthias-runge.de writes:
MR> My question is: is this review sufficient, if not, where is it MR> written down, that it isn't? I'm especially aiming to the form of MR> this review.
Bottom line is that it isn't. This turns into a nice argument every few years.
In the past when I ran across one of these (i.e. back in the good old days when I actually read the entire package review mailing list) I'd double check and I pretty much always found something that was overlooked. Opening a discussion about how the review was probably insufficient was usually enough.
- J<
On 08/02/12 20:51, Jason L Tibbitts III wrote:
Bottom line is that it isn't. This turns into a nice argument every few years.
Thanks for your answers. It reads like there's no review documentation required. Asking for this leads to endless discussion, and probably to no result.
In the past when I ran across one of these (i.e. back in the good old days when I actually read the entire package review mailing list) I'd double check and I pretty much always found something that was overlooked. Opening a discussion about how the review was probably insufficient was usually enough.
In my earlier noted case, I should prove, the reviewer had something forgotten to check.
To be clear, imho there's no need to take back a granted review, the packager of the reviewed package knows his job well.
I think, a review generates work to do for the reviewer. Why one shouldn't see, how much, work or which results were found during this work. (even if it reads ... ok, ... ok, ... ).
"MR" == Matthias Runge mrunge@matthias-runge.de writes:
MR> To be clear, imho there's no need to take back a granted review, the MR> packager of the reviewed package knows his job well.
Again in the good old days, I was also processing most of the SCM requests, so I'd effectively stop the process until the review was re-done. Occasionally this meant that I did my own review.
MR> I think, a review generates work to do for the reviewer.
Well, theoretically it does. We have nothing to prevent someone from just setting the flag without actually doing any review work at all. Hopefully such stuff would be caught at some point, but really unless someone is actually looking over all of the tickets this doesn't happen. At best someone will notice that a package is in pretty poor shape, look up the review ticket and notice that it wasn't done well. But who has time for that? There are 350+ packages in the review queue. Still.
- J<
On Wed, 08 Feb 2012 21:09:18 +0100, MR (Matthias) wrote:
I think, a review generates work to do for the reviewer.
That's only one half of the story. :)
First of all, it is the _packager_ who ought to become familiar with the Packaging Guidelines *and* the Review Guidelines and submit a package that aims at passing the review process. Then, during the review process, it's _two_ people (at least) who work on the package as a team and apply best judgement. It's not only the reviewer. And often, it's the second pair of eyes that's helpful. Not because the reviewer does "all the work", but some reviewers spot poor packaging mistakes with the blink of an eye already, whereas for the package submitter, it may be the 2nd or 3rd package so far only.
Why one shouldn't see, how much, work or which results were found during this work. (even if it reads ... ok, ... ok, ... ).
A checklist may be the result of a quick cut'n'paste job. Mistakes included. It doesn't tell how much effort (or time) has been spent on the review. On doing testbuilds, skimming over build logs, examining subpackage contents and inter-dependencies, and on paying extra attention to details that perhaps are not covered by the guidelines yet.
On Wed, 08 Feb 2012 20:26:06 +0100, MR (Matthias) wrote:
Hi,
lately, I stumbled upon a review, which I thought, wouldn't suffice. It looks like the following
name: ok summary: ok license: ok handling locale files: ok rpmlint output: only spelling warning Not needed BuildRequires: (names), please remove them in git.
APPROVED.
Please mention the ticket number.
It may be that the spec file is short/simple and that the listed items cover most of what was necessary to review. I find it silly to even mention "name: ok".
My question is: is this review sufficient,
It is.
There is no requirement for the reviewer to flood the ticket with a huge list of checkmarks about things that possibly don't even apply to a package. It doesn't make reviews better, and it doesn't make them safer either. That is because the guidelines aren't bullet-proof and not complete either. In other places, the guidelines are not detailed enough and only experienced reviewers understand the background.
Btw, I think we've had cases before where reviewers, who have posted an overwhelming checklist, missed several items (or got them wrong). [not limited to %optflags, plugins in -devel packages, static libs, licensing, files in wrong subpkgs]
Do we trust our reviewers,
We do. In the same way we trust our packagers. And still, some packagers *and* reviewers (re-)introduce packaging mistakes *after* a package has been approved. ;) Sometimes the changes in package git invalidate the review results completely, because a packager messes up the packaging.
so there's no need of bureaucracy?
Fill a growing list of reviews where the reviewer has missed important items, and then let's figure out what can be done about that. Let's not punish good reviewers with tiresome bureaucracy.
Why should/must I do more than just setting the flag or writing 7 catchwords?
Do whatever helps you to gain confidence in approving a package. If you feel it's necessary to process a checklist and include that checklist, do that. Once you've posted such a list in a review, what would you do if another reviewer pointed out that you've missed a couple of unowned directories, for example? (note: that's still a MUST item in the review guidelines)
packaging@lists.fedoraproject.org
-
Jason L Tibbitts III -
Jon Ciesla -
Matthias Runge -
Michael Schwendt