Fedora.us QA (was: Re: Prelink success story :))

[Erik LaBianca]

> -----Original Message-----
> From: fedora-devel-list-admin at redhat.com [mailto:fedora-devel-list-
> admin at redhat.com] On Behalf Of Michael Schwendt
> Sent: Thursday, February 26, 2004 7:15 PM
> To: fedora-devel-list at redhat.com
> Subject: Fedora.us QA (was: Re: Prelink success story :))
> 
<snip>
> 
> ... and, of course, because you've realized that you take a big
portion of
> responsibility with your approval of a package, so you wanted to aim
> at perfection, didn't you? ;)  Yes, of course you wanted to avoid any
> mistakes. :)

You got me there :)
 
 
> > 1. Information how "how to do some useful qa" is scattered.
> 
> Remains the question whether you could really instruct newbies without
the
> help of a book or a few courses.

A good question it is. My belief is that it must be possible. At least,
I like to think I made a useful contribution, and I read no books and
took no courses. They'll not be packaging guru's overnight, but they'll
be able to "get up to speed" doing something useful. 

> I added an entry to the checklist yesterday. A show-stopper IMO.
> To check file permissions and ownership. Has happened several times,
> that a %defattr was missing or files/directories were inaccessible.

I agree on that one. File %defattr can make a nasty mess to be sure.

> 
> > There should be a template like I have above, that can be answered
> > with simple yes or no answers, that covers all the showstoppers.
That
> > way a newbie can fill it in the blank with yes's or no's and no he's
> > done something useful.
> 
> But would that be useful? Do you expect someone to go through the list
a
> newbie posted and check step by step whether he made a mistake?  I
think
> the checklist is for personal use only and should not be attached to
> package requests as a Yes/No answered list. Approvals should be short
and
> to the point and concentrate on a few items. If I had to fill out a
list
> of what I check, that would slow me down a lot unless some special
> software would aid me upon maintaining the list.

Well, honestly, yes, I do expect someone to go over a newbies work.
That's what the "2 reviews" policy for, isn't it? For someone such as
yourself, who clearly knows what makes a good package at a much deeper
than just "did it pass the showstopper checklist", the need to fill out
the checklist becomes less if at all.

I don't know why it would take more than a few seconds to fill out a
basic showstopper checklist. Obviously, the more skilled the QA'er, the
more additional "value" they add beyond the checklist. But the checklist
makes a good start, and allows someone without that "common sense" you
referred to earlier today to make a meaningful contribution and get up
to speed.

Think of it as a pre-flight checklist. I think this sort of formalism is
one of the area's where fedora.us / extra's has a chance to improve upon
the single-maintainer policies, because theres a written down list of
"showstoppers" which aren't allowed to pass QA. And if they do, the
multiple reviews over time will eventually catch them.

> 
> > Since all the checklist items are there, answered
> > with yes or no questions, it's easy to know if they actually made a
good
> > faith effort to check the package.
> 
> Which increases the workload a lot, when many items are not needed
> for a particular package or when many more items are added to the list
> (because it isn't complete). Reviewers should develop a feeling for
> what is important and what is not.
> 

How is a new reviewer supposed to develop that feeling? You've got to
have a place to start. I'm sure there are people out there who have
never seen a spec file before that would like to be able to help fedora.
These people should be recommended to read maximum rpm, and then start
qa'ing using the showstopper checklist. I'd say there shouldn't be more
than a dozen or so show stoppers, which could easily be included in the
preflight checklist. 


> > Some critique of the list:
> >
> > 1. Does the package follow the Fedora Package Naming Guidelines?
> > This is pretty darn complicated for a newbie QA'er. They should be
> > allowed to opt out. A "senior developer" could look at the package
in 10
> > seconds and understand whether or not the name is ok.
> 
> But package naming and versioning issues don't stop anyone from
reviewing
> the rest of a package, do they?
> 

No they don't. However, as a newbie, my impression was that I was to
review ALL aspects on the checklist. After doing some QA and looking at
other peoples bugzilla reports I figured out that it would be ok if I
just downloaded the package and made some comments about what I didn't
like.

However, people just making comments about what they like doesn't
provide a deterministic path to REVIEWED status.


> > 2. Are the sources from upstream pristine and free from trojans?
> > This is important, but exceedingly difficult to do "properly"... At
what
> > point does this become a showstopper? At some point it becomes
> > impossible to verify the source without actually being the author of
the
> > code and inspecting it line by line. For a newbie this is very
> > intimidating
> 
> Of course. Still, the least someone can do is compare the MD5
checksums
> and also check packages from other distributions (not because they are
> trusted ultimately, but to fetch the source from multiple locations)
and
> visit the home page (and e.g. directories like freshmeat.net) to check
> whether a release might be official.
> 
> Asking upstream authors for MD5 checksums via e-mail doesn't work
> well. I've tried without luck to get some to post clearsigned
checksums in
> the download area.

I'd have been happier about this item if I'd known this was somewhat
optional, and not expected to be 100% possible. However, it needs to get
done at some point. If it's on the checklist, then you know it got done.
If I just make comments about broken things, and then submit an
approval, how do you know I verified md5sums?

> > I'd suggest something like "are all
> > pathnames in the pre/post(un) install scripts complete?
> 
> What does that mean? Macros in scripts are okay and are expanded at
> build-time. Environment variables are not okay because they are
evaluated
> at run-time.

These are tough. I don't think you can expect a newbie to really
evaluate this very effectively. That being said, if they install the
package and it works, the scripts can't be TOO bad. They can be improved
later when somebody more knowledgeable looks at the code. Maybe this
item is pedantic and should be instead included under "does it work?".
 
> 
> > Are macros used for all package files?
> 
> Depends on whether it makes sense. If the source code has some paths
> hardcoded, it doesn't make much sense to use corresponding macros in
the
> spec file.

Ditto as above. It's a tough call, but not exactly a showstopper. People
will have differing opinions on it. If the package works, let it go or
argue about it, but don't stall it in QA over this.

> 
> > Also. Wheres the "does this package appear to work" line item? Do we
QA
> > packages and not actually run them? It seems like this is pretty
basic
> > criteria.
> 
> Are step by step items on "does it build?", "does it install?",
> "does it upgrade?", "does it erase?" really necessary?

I think so, to a degree. This list needs to be basic, so people can get
up to speed. Also, in the interests of formalism, knowing that the
package builds properly, and appears to work after installing is
important. Maybe the review just downloaded the srpm, made some
complaints about the spec file syntax ($RPM_BUILD_ROOT vs %{buildroot}
anyone) and moved on. 

I'm seriously concerned that packages be able to move through fedora
quickly and efficiently, without compromising their quality. I'm not
convinced the current process is getting that job done.

> > 5. Are there no missing BuildRequires?
> >
> > This one took me a LONG time to check. The manual "remove all devel
> > rpms" method is apparently deprecated according to the url above,
> > however fedora.us doesn't even include a copy of mach. No less, mach
is
> > pretty complicated and you've got to figure out how to create a
> > fedora.us enabled buildroot.
> 
> "mach" is not mandatory. I've tried it once, long ago. Use what works
for
> you. It can be embarrassing if an approved package fails in the build
> system.

Ok so it's not mandatory but it seems the sanest way to check
buildrequires. Is it used for the fedora.us buildsystem or not? It
doesn't make much sense to me for packages to pass QA that can't be
built properly on the build system.

> 
> > 6. Is the Group tag an official one? See
/usr/share/doc/rpm-*/GROUPS.
> >
> > Not sure if this is relevant to anything, but again, it's easy to
check.
> > Couldn't rpmlint do this?
> 
> AFAIK, rpmlint checks whether a group is known, but not whether
> a package is put into the proper category.

Point well made. I agree. A human will need to verify that the
categorization makes sense.

> 
> > 10. Is the package free of unowned directories?
> >
> > This is also very hard to check. You mean I have rpm -ivvh this
package
> > and then scroll through a whole pile of cruft to see the unowned
> > directories? And then I have to know which ones it's "supposed" to
own,
> > and which ones it's now?
> 
> You can also "rpm -qlv |less" the package and make sure that it owns
> its _own_ directories. ;)
> 
<snip>
> This depends on a package's dependencies, e.g. whether the package
adds
> files to the directory of a package it depends on. In that case it
would
> not need to own the directory.

These are good tips. Lets get them into the docs somewhere, along with a
definition of "its own directories". This wasn't immediately obvious to
me, and I would hope to be able to have people be able to contribute
more easily than I did.

> > 14. Does the package handle a parallel build cleanly?
> >
> > Does this really matter? Seems like a nice thing to check but not a
> > showstopper. Make it go away.
> 
> It is a showstopper, because a package which "make"s with %_smp_mflags
in
> the spec file might break badly in the build system. It is not always
> obvious whether such breakage is due to SMP make flags. Defining
> "%_smp_mflags -j3" in your local ~/.rpmmacros file also on UP machines
> can help detect breakage.

Ok. So we need everybody to put "%_smp_mflags -j3" in their
~/.rpmmacros, and then we can just ask them "does it build". Maybe what
I'm getting at here is that it would be really helpful to have an easily
duplicated (checked out from fedora.us!!), defined build environment... 

> 
> > 11. Does it pass rpmlint cleanly?
> 
> Not always possible. :)
> 

Ok true, but if it doesn't maybe that's time for a more knowledge person
to approve that which doesn't pass rpmlint?

> > With good solid descriptions for 1-6 especially, I think it will
become
> > much easier to get useful information from would-be QA'ers. I'd like
to
> > see the how to help page say "Please do QA. Heres how"
> 
> And when issues not covered in the list come up, those people are fed
up.
> E.g. but not limited to:
> 
>  * build not accepting $RPM_OPT_FLAGS
>  * build stripping binaries
>  * bad -rpath found
>  * archdependent files below %_datadir
>  * not adhering to FHS
>  * spec file sections in wrong order (e.g. builds in %install section)
>  * libtool archives not deleted and not needed or wrong paths
>    in libtool archives
>  * *.so links in non-devel package and not needed there
>  * python *.py{c,o} files not marked %ghost
>  * perl modules installed into site location, not vendor location
>  * macros in %changelog

I'm not sure I understand what you're saying here. Maybe I should have
said "heres how to start" instead?

I understand all those and many more can be possible problems, but
expecting packages to be perfect before they pass QA is going to stall
the process indefinitely. If those issues come up, the newbie may not be
able to detect them. They'll get noticed sometime, and should get fixed
then.

> 
> > 2. Provide some feedback. I QA'd some packages. I waited. And
waited. A
> > week later, AFTER I saw a discussion about bugzilla keywords and
added
> > "NEEDSWORK" to the packages I had QA'd,
> 
> NEEDSWORK is to move packages out of the QA queue, because they don't
> build or need more work before they are moved back into the QA queue.
> This can also be set by the packager if major changes are planned.
> REVIEWED is the new keyword to flag reviewed packages, so they are
> easy to locate.
> 

OK. So at what point do I make the determination that the package has
passed QA? The stuff I marked as needs work ended up being due to some
stuff I saw on the "QA Checklist" which turns out not to really be a
showstopper, or is it? 

With a showstopper checklist, I know that if there are no showstoppers
it's safe to approve. If there are it's not. 

This way my likelihood of looking like an idiot is very low, if I
followed the instructions properly. Not feeling like an idiot is what
makes people come back and feel good about contributing.

> > I got my first piece of
> > feedback. This doesn't encourage repeat customers. It's a whole lot
more
> > rewarding when you submit something, and an hour later you get a
message
> > back from somebody saying "Hey, you messed up, but thanks for
trying,
> > can you please check a, b, and c?".
> 
> Lack of man power. More reviewers are needed. Especially since we
don't
> know how/what official Fedora Extras will change.

Agreed. And understood. And I'm trying to help make it easier to get new
reviewers. I know several semi-serious linux users I might get QA some
packages, but until the process is streamlined enough I can point them
to a relatively straightforward set of tasks I'm very unlikely to get
them to actually do it.


> > I think it's imperative that packages make it through the QA
process. It
> > doesn't do any good if packages never make it into the repo. Right
now,
> > they appear to just sit there. Then the packager gets fed up,
doesn't
> > respond if anybody checks his package, and starts a new repo.
> 
> Not every packager can work on his packages as soon as someone found
> issues. Sometimes you need to wait till the next weekend. If the
packager
> doesn't respond, simply move the request out of the package queue by
> deleting the QA keyword and setting the NEEDSWORK keyword.


OK, that's fine, but in all reality I'm uninterested in removing the
package from the QA queue. I want the package published! And now! At
what point is it ok for me to just post my own version of the SRPM? 

Also, documentation of these keywords was not apparent to me in the web
of pages I looked at while trying to figure this process out.

> > This is
> > not building community. This process has GOT to be fast enough that
when
> > I submit a package for something that people actually use, it gets
QA'd
> > SOON, before I've lost all interest in it.
> 
> If there are people with interest in a package, surely there should be
> two reviewers, too?
> 

I'm sure they are out there, but we've got to make the process so darn
straightforward they can't help but succeed once they decide to help
out. I'm skeptical that it's that way now. I'm not sure if I succeeded
yet, and I can assure you I attacked it unsuccessfully a few times
before finally making it over the hump of actually posting a review
comment.

Just $0.02!

--erik