Spec URL: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu.spec SRPM URL: http://sgordon.fedorapeople.org/repo/SRPMS/aqemu-0.8.2-1.fc15.src.rpm Description: AQEMU is a graphical user interface to QEMU and KVM, written in Qt4. The program has a user-friendly interface and allows user to set the majority of QEMU and KVM options on their virtual machines. Hi, I just finished packaging aqemu based on a SuSE SRPM I found for a previous version (0.8.1). The SRPM is for the latest release (0.8.2) and I believe contains the modifications required for inclusion in Fedora. I would appreciate a package review so that I can proceed to make any further changes that might be required for this to happen. rpmlint output is as follows (seems to be intermittent, sometimes I get the 403 on the URL sometimes I don't): $ rpmlint /home/sgordon/rpmbuild/RPMS/x86_64/aqemu-0.8.2-1.fc15.x86_64.rpm aqemu.x86_64: W: invalid-url URL: http://aqemu.sourceforge.net HTTP Error 403: Forbidden 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
Should also mention, the reference patch file is here: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu-0.8.2-fedora-desktop.patch
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
Spec URL: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu.spec SRPM URL: http://sgordon.fedorapeople.org/repo/SRPMS/aqemu-0.8.2-2.fc15.src.rpm Thanks for your comments Richard, responding inline to confirm action on each item. (In reply to comment #2) > 1. Your Souce: tag should be the full URL whenever possible. There's a specific > guideline[1] for SourceForge. Updated. > 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. Updated, was originally considering building for EL5 but seems unlikely now. > 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. Updated, moved to %install. > 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" Updated. > Also, unless it creates a problem, smp flags should be set on make to speed up > building: > > make %{?_smp_mflags} > Updated. > 5. You should never strip the binaries. That would make the debuginfo > sub-package useless. Rpmbuild will take care of that for you. Updated, including the change to specify DESTDIR. Note that this results in the following additional warning from rpmlint: aqemu.x86_64: W: unstripped-binary-or-object /usr/bin/aqemu The resultant binary does however still appear to be executable.
Here are some additional notes: - The package currently fails to build because of the macros in the %changelog. Add a second percent sign to quote macros in comments or changelog entries, e.g. %%prep. - Drop the explicit Requires fields. The dependency on qt and qt-x11 is detected automatically. - .desktop files must be installed or verified with desktop-file-install or desktop-file-validate, respectively. See https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files - Don't mix $RPM_BUILD_ROOT and %{buildroot}. Choose one variant and stick with it. - The doc files should go to %{_datadir}/doc/%{name}-%{version}. The easiest way to achieve this here is * to remove directory %{_datadir}/%{name}/doc in %install, * to drop %{_datadir}/%{name}/doc from %files, and * to add "%doc AUTHORS CHANGELOG COPYING README TODO" to the %files section.
(In reply to comment #4) > * to remove directory %{_datadir}/%{name}/doc in %install, > * to drop %{_datadir}/%{name}/doc from %files, and Sorry, this should read %{_datadir}/doc/%{name}/ rather than %{_datadir}/%{name}/doc/.
Spec URL: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu.spec SRPM URL: http://sgordon.fedorapeople.org/repo/SRPMS/aqemu-0.8.2-3.fc15.src.rpm (In reply to comment #4) > Here are some additional notes: > > - The package currently fails to build because of the macros in the %changelog. > Add a second percent sign to quote macros in comments or changelog entries, > e.g. %%prep. This is a bit odd as it was still building for me on F15, but makes sense nonetheless. Updated. > - Drop the explicit Requires fields. The dependency on qt and qt-x11 is > detected > automatically. Removed. > - .desktop files must be installed or verified with desktop-file-install or > desktop-file-validate, respectively. See > https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files Updated to use desktop-file-validate. > - Don't mix $RPM_BUILD_ROOT and %{buildroot}. Choose one variant and stick > with it. Updated. > - The doc files should go to %{_datadir}/doc/%{name}-%{version}. The easiest > way to achieve this here is > * to remove directory %{_datadir}/%{name}/doc in %install, > * to drop %{_datadir}/%{name}/doc from %files, and > * to add "%doc AUTHORS CHANGELOG COPYING README TODO" to the %files section. Updated. This however has resulted in rpmlint highlighting that the COPYING file contains an out of date FSF address which I will have to take up with upstream.
(In reply to comment #6) > This is a bit odd as it was still building for me on F15, but makes sense > nonetheless. Updated. Did you try to build the package with mock [1]? The mock build failed as you can see here: http://koji.fedoraproject.org/koji/taskinfo?taskID=3310708 Your latest package builds properly: http://koji.fedoraproject.org/koji/taskinfo?taskID=3310977 There are a few further things to address: - desktop-file-validate reports to issues: /usr/share/applications/aqemu.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated /usr/share/applications/aqemu.desktop: error: (will be fatal in the future): value "aqemu.png" for key "Icon" in group "Desktop Entry" is an icon name with an extension, but there should be no extension as described in the Icon Theme Specification if the value is not an absolute path => drop the Encoding entry and remove the .png suffix from aqemu.png (in the .desktop file). - you can remove the slash from %{buildroot}/%{_datadir} because %{_datadir} is an absolute path (usually /usr/share). - also quote %cmake in the %changelog to make rpmlint happy [1] http://fedoraproject.org/wiki/Using_Mock_to_test_package_builds
Spec URL: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu.spec SRPM URL: http://sgordon.fedorapeople.org/repo/SRPMS/aqemu-0.8.2-4.fc15.src.rpm (In reply to comment #7) > There are a few further things to address: > > - desktop-file-validate reports to issues: The original upstream desktop file had a few other issues which are the reason for the Patch0. I've now updated it to drop the suffix on the Icon key and drop the Encoding entry completely. > - you can remove the slash from %{buildroot}/%{_datadir} because %{_datadir} is > an absolute path (usually /usr/share). Done. > - also quote %cmake in the %changelog to make rpmlint happy Done.
I'll go aheand and take this review. I've done some test builds but haven't installed and run them yet. I did make a (minor?) change to the icon handling. Since the upstream package provides more than one size of icon it seemed a shame not to include it. The guidelines are quite sparse in this area so don't consider this authoritative, but it seems that /usr/share/pixmaps is better if you only have one icon file because it doesn't have any "sized" sub-directories. Because we have two icon files available, it seems more appropriate to move them to /usr/share/icons. The code below is what I changed/added to your spec file. # Move icons to a better location mkdir -p %{buildroot}%{_datadir}/icons/hicolor/{48x48,64x64}/apps mv %{buildroot}%{_datadir}/pixmaps/%{name}_48x48.png \ %{buildroot}%{_datadir}/icons/hicolor/48x48/apps/%{name}.png mv %{buildroot}%{_datadir}/pixmaps/%{name}_64x64.png \ %{buildroot}%{_datadir}/icons/hicolor/64x64/apps/%{name}.png rm -rf %{buildroot}%{_datadir}/pixmaps As I mentioned I haven't installed the package yet but I have verified it does include the icons in the rpm so there's no reason it shouldn't work. Hoepfully I'll have some time this weekend to do the full guideline review. Richard
Richard, since this is Stephen's first package submission (as per the FE-NEEDSPONSOR block above), the review must be done by a sponsor. According to your FAS entry, you're not a packager sponsor (yet), thus you can't do the formal review. Please correct me if I'm mistaken and checked the wrong user information.
Nope, your right, I forgot he needed a sponsor. Richard
Thanks for the feedback regarding the icons anyway Richard :).
Spec URL: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu.spec SRPM URL: http://sgordon.fedorapeople.org/repo/SRPMS/aqemu-0.8.2-5.fc15.src.rpm (In reply to comment #9) > Since the upstream package provides more than one size of icon it seemed a > shame not to include it. The guidelines are quite sparse in this area so don't > consider this authoritative, but it seems that /usr/share/pixmaps is better if > you only have one icon file because it doesn't have any "sized" > sub-directories. > > Because we have two icon files available, it seems more appropriate to move > them to /usr/share/icons. The code below is what I changed/added to your spec > file. I have updated as suggested. I also however found that I had to add %post, %postun, and %posttrans scriptlets to make use of gtk-update-icon-cache as described here: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
Good catch! Yes, you are correct. Sorry I missed that! Richard
Added reference to more complete review attempt in Bug # 786668, still seeking a sponsor :).
Stephen, I'll look through your informal reviews in the next couple of days and will sponsor you afterwards. Currently, I'm pretty busy with my job, so it could take a few days.
Here's the formal review of your package. There are a couple of things that need to be fixed: - The package currently doesn't build in mock (F16) because of * missing BR: gnutls-devel * clash of "error" macros I suggest to manually expand the macro in file Embedded_Display/vncview.cpp and drop its definition from the same file. This should be reported upstream. - The license of aqemu is GPLv2+ according to the source file headers. => update the License field accordingly - Add a short comment above Patch0 telling what the patch does. Have you sent the patch upstream? - File README contains installation instructions only. => it should be removed from the package - Please add the section suffix to the manpage filename in %files: %{_mandir}/man1/%{name}* => %{_mandir}/man1/%{name}.1* - Add blank lines between the %changelog entries to improve legibility. $ rpmlint *.rpm aqemu.src: W: invalid-url URL: http://aqemu.sourceforge.net HTTP Error 403: Forbidden aqemu.x86_64: W: invalid-url URL: http://aqemu.sourceforge.net HTTP Error 403: Forbidden aqemu.x86_64: E: incorrect-fsf-address /usr/share/doc/aqemu-0.8.2/COPYING aqemu-debuginfo.x86_64: W: invalid-url URL: http://aqemu.sourceforge.net HTTP Error 403: Forbidden 3 packages and 0 specfiles checked; 1 errors, 3 warnings. - the invalid URL messages are false positive - incorrect FSF address has been reported upstream --------------------------------- key: [+] OK [.] OK, not applicable [X] needs work --------------------------------- [+] MUST: The package must be named according to the Package Naming Guidelines. [+] MUST: The spec file name must match the base package %{name}. [+] MUST: The package must meet the Packaging Guidelines. [+] MUST: The package must be licensed with a Fedora approved license. - GPLv2+ according to boilerplates [X] MUST: The License field in the package spec file must match the actual license. - update the License field accordingly [+] MUST: The file containing the text of the license(s) for the package must be included in %doc. [+] MUST: The spec file must be written in American English. [+] MUST: The spec file for the package MUST be legible. [+] MUST: The sources used to build the package must match the upstream source. $ md5sum aqemu-0.8.2.tar.bz2* dcec083f566e0a22df7bfe8e7ca4c593 aqemu-0.8.2.tar.bz2 dcec083f566e0a22df7bfe8e7ca4c593 aqemu-0.8.2.tar.bz2.upstream [+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [.] MUST: If the package does not successfully compile, build or work on an architecture, ... [X] MUST: All build dependencies must be listed in BuildRequires. [+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied. [+] MUST: The spec file MUST handle locales properly. [+] MUST: If a package installs files below %{_datadir}/icons, the icon cache must be updated. [.] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun. [+] MUST: Packages must NOT bundle copies of system libraries. [.] MUST: If the package is designed to be relocatable, ... [+] MUST: A package must own all directories that it creates. [+] MUST: A Fedora package must not list a file more than once in %files. [+] MUST: Permissions on files must be set properly. [+] MUST: Each package must consistently use macros. [+] MUST: The package must contain code, or permissable content. [.] MUST: Large documentation files must go in a -doc subpackage. [+] MUST: Files in %doc must not affect the runtime of the application. [.] MUST: Header files must be in a -devel package. [.] MUST: Static libraries must be in a -static package. [.] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [.] MUST: devel packages must require the base package using a fully versioned dependency. [.] MUST: Packages must NOT contain any .la libtool archives. [+] MUST: Packages containing GUI applications must include a %{name}.desktop file [+] MUST: .desktop files must be properly installed with desktop-file-install in the %install section. [+] MUST: Packages must not own files or directories already owned by other packages. [+] MUST: All filenames in rpm packages must be valid UTF-8. EPEL <= 5 only: [X] MUST: The spec file must contain a valid BuildRoot field. [X] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}. [X] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}. [.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' [.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [X] SHOULD: All patches should be commented in the spec file [X] SHOULD: The reviewer should test that the package builds in mock. [+] SHOULD: The reviewer should test that the package functions as described. [+] SHOULD: If scriptlets are used, those scriptlets must be sane. [.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg. [.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [+] SHOULD: Your package should contain man pages for binaries/scripts.
Thanks, I will update shortly.
(In reply to comment #17) > - Add a short comment above Patch0 telling what the patch does. Have you sent > the patch upstream? It appears I was not logged into Source Forge at the time but I believe I filed this ticket upstream: http://sourceforge.net/tracker/?func=detail&aid=3430317&group_id=229794&atid=1078458 It's now marked fixed so I expect to see it in their next release and I will be able to drop the patch.
(In reply to comment #17) > * clash of "error" macros > I suggest to manually expand the macro in file > Embedded_Display/vncview.cpp and drop its definition from the same file. > This should be reported upstream. Looks like this was reported upstream as well: http://sourceforge.net/tracker/?func=detail&aid=3429937&group_id=229794&atid=1078458 I will apply their patch in the SRPM for now, pending the next stable release.
(In reply to comment #17) Updated files, responses inline: Spec URL: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu-0.8.2-6.spec SRPM URL: http://sgordon.fedorapeople.org/repo/SRPMS/aqemu-0.8.2-6.fc16.src.rpm > Here's the formal review of your package. There are a couple of things that > need to be fixed: > > - The package currently doesn't build in mock (F16) because of > * missing BR: gnutls-devel Added BuildRequires. > * clash of "error" macros > I suggest to manually expand the macro in file > Embedded_Display/vncview.cpp and drop its definition from the same file. > This should be reported upstream. Updated upstream change applied as patch for now (see comment # 20), will drop the patch after next stable release. > - The license of aqemu is GPLv2+ according to the source file headers. > => update the License field accordingly Updated. > - Add a short comment above Patch0 telling what the patch does. Have you sent > the patch upstream? Updated, also included comment for the additional patch. > - File README contains installation instructions only. > => it should be removed from the package Removed. > - Please add the section suffix to the manpage filename in %files: > %{_mandir}/man1/%{name}* => %{_mandir}/man1/%{name}.1* Updated to include the section suffix. > - Add blank lines between the %changelog entries to improve legibility. Updated. Going through the needs work items from the checklist just to confirm: > --------------------------------- > key: > > [+] OK > [.] OK, not applicable > [X] needs work > --------------------------------- > > [X] MUST: The License field in the package spec file must match the actual > license. > [X] MUST: All build dependencies must be listed in BuildRequires. Actioned as above. > EPEL <= 5 only: > [X] MUST: The spec file must contain a valid BuildRoot field. > [X] MUST: At the beginning of %install, each package MUST run rm -rf > %{buildroot}. > [X] MUST: Each package must have a %clean section, which contains rm -rf > %{buildroot}. > [.] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' I do not intend to package for EPEL at this time. > separate file from upstream, the packager SHOULD query upstream to include it. > [X] SHOULD: All patches should be commented in the spec file > [X] SHOULD: The reviewer should test that the package builds in mock. Actioned as above.
Martin, Please take another look when you get a chance, thanks! Steve
Steve, the package looks almost fine now. I forgot to mention that another BuildRequires is missing: hicolor-icon-theme provides the directories where the icons are installed. You probably already got an email about your sponsorship, so welcome to the packager group! You're now allowed to do formal reviews of other packager's submissions (except those blocked by FE-NEEDSPONSOR), and all other packagers can review your submissions.
Thanks Martin, I will make one more revision to add that BuildRequires before I get the package added.
New Package SCM Request ======================= Package Name: aqemu Short Description: A QT graphical interface to QEMU and KVM Owners: sgordon Branches: f15 f16 f17 InitialCC:
Steve, you can't request a Git repo at this time, as the package isn't approved yet. Please post a link to the updated package first. If everything looks good, I will approve it. Sorry for being nit-picking, but since it's your first submission, I'm a bit more pedantic. ;)
Spec URL: http://sgordon.fedorapeople.org/repo/SOURCES/aqemu-0.8.2-7.spec SRPM URL: http://sgordon.fedorapeople.org/repo/SRPMS/aqemu-0.8.2-7.fc16.src.rpm
OK, the package looks good and is ready now. ---------------- Package APPROVED ----------------
Git done (by process-git-requests).
aqemu-0.8.2-7.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/aqemu-0.8.2-7.fc17
aqemu-0.8.2-7.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/aqemu-0.8.2-7.fc16
aqemu-0.8.2-7.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/aqemu-0.8.2-7.fc15
aqemu-0.8.2-7.fc17 has been pushed to the Fedora 17 stable repository.
aqemu-0.8.2-7.fc15 has been pushed to the Fedora 15 stable repository.
aqemu-0.8.2-7.fc16 has been pushed to the Fedora 16 stable repository.