[Bug 198758] Review Request: gnome-phone-manager

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: gnome-phone-manager


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=198758





------- Additional Comments From paul at city-fan.org  2006-07-14 06:46 EST -------
(In reply to comment #3)
> (In reply to comment #2)
> > >       - This package used macros.
> > 
> > But did it use them *consistently*? That's what the review guidelines are asking
> > to be checked.
>  Did i missed somthing to check in SPEC?

I don't know; I haven't looked at the spec. The review guidelines ask you to
check that macros are used consistently, not just that they are used. So you're
looking for things like:

 * Using both $RPM_BUILD_ROOT and %{buildroot}
 - both expand to the same value; use one or the other but not both in the same
spec file

 * Using %{__rm} but not %{__make} etc.
 - if the packager is using macros for some commands (e.g. %{__rm}), they should
use them in all cases where a macro is available; alternatively, they could just
use "rm" instead of "%{__rm}"; the key is consistency

> > >       - Document files are included like README, NEWS, COPYING, AUTHORS
> > 
> > Did you look to see if there are any other document files in the package that
> > might be included, or whether any of the included files don't have anything
> > useful to end users of the package?
>      I didn't get you?

Supposing the package contains docs:
README, NEWS, COPYING, AUTHOR

Did you check to see if there are any other useful documents in the tarball that
might be useful to end users? There might be some other .txt files, a docs
directory full of useful HTML files, perhaps a TODO file? A reviewer should be
looking at a package in much the same way as they would do if they were
packaging something themselves, looking to see what would be sensible to include.

Also on the subject of documentation, sometimes the documentation provided in
tarballs isn't very useful. So sometimes you may see a "NEWS" file that just
says to look in the "ChangeLog" file. There is no point in including such a
"NEWS" file in the package. That's something a reviewer should be looking at too.

> > >       * BuildRequires is correct
> > 
> > No, they're not. The package failed to build in mock because of the missing
> > buildreqs of perl(XML::Parser) and gettext.
> > 
>  I forgot that i added perl(XML::Parser) not AUTHOR.

There's also the missing gettext buildreq that caused the package build to fail
in mock.


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.