[Bug 734275] Review Request: aqemu - A QT graphical interface to QEMU and KVM

[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.


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

Richard Shaw <hobbes1069 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hobbes1069 at gmail.com

--- Comment #2 from Richard Shaw <hobbes1069 at gmail.com> 2011-08-29 21:33:09 EDT ---
I make take this one if no one else grabs it, but here's a quick review of the
spec file.

1. Your Souce: tag should be the full URL whenever possible. There's a specific
guideline[1] for SourceForge.

2. You don't need the "BuildRoot:", "%clean", or "defattr" in "%files" if
you're not going to build for EL5 (Redhat, CentOS, Scientific Linux). I can't
quite remember at what specific release those became unnecessary, but if you're
only building for F14+/EL6+ they're all safe to remove.

3. I don't recall any rule for it, but unless it's needed for successful
building, I wouldn't do any file manipulation in %prep.

In this specific case depending on how "make install" works I would just mv
(rename) the file instead of creating a symbolic link.

4. Speaking of building, Fedora has a cmake macro, "%cmake", that takes care of
the most common configuration options. In your case, you would replace the
whole line with just "%cmake"

Also, unless it creates a problem, smp flags should be set on make to speed up
building:

make %{?_smp_mflags}

One (intended) side-effect of this is that the install location will no longer
include the build root. Sometimes you can get away with including it but if
program hard coded that path somewhere in a binary or library then it would
fail. We'll fix the install location in #5.

5. You should never strip the binaries. That would make the debuginfo
sub-package useless. Rpmbuild will take care of that for you.

Also, don't use macros for common shell commands (rm, mv, cp, install, etc.)
unless you're going to override their default behavior. I'm not sure why SUSE
uses them so frequently.

Now we fix where the files are "installed" using the DESTDIR environment
variable.

make DESTDIR=%{buildroot} install

That's all I see at the spec file level. Be sure to post new links for the
updated spec and SRPM. Since you've officially submitted the package you'll
need to bump the release to 2 and update your changelog so anyone who follows
behind me and easily see what you've done.

Richard

[1] http://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.