Spec URL: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv.spec SRPM URL: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv-0.8-1.fc6.src.rpm Description: A picture viewer for X, with a thumbnail-based file selector. It uses GTK+ and Imlib. Most file formats are supported, and the thumbnails used are compatible with xv, zgv, and the Gimp.
I'd be happy to review this package. Look for a full review in a few here...
OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. OK - Spec has consistant macro usage. OK - Meets Packaging Guidelines. OK - License (GPL) OK - License field in spec matches OK - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream md5sum: e392277f1447076402df2e3d9e782cb2 xzgv-0.8.tar.gz e392277f1447076402df2e3d9e782cb2 xzgv-0.8.tar.gz.1 See below - BuildRequires correct OK - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Package has correct buildroot OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime. OK - Package has rm -rf RPM_BUILD_ROOT at top of %install OK - Package is a GUI app and has a .desktop file OK - Package compiles and builds on at least one arch. OK - Package has no duplicate files in %files. OK - Package doesn't own any directories other packages own. OK - Package owns all the directories it creates. OK - No rpmlint output. OK - final provides and requires are sane. SHOULD Items: See below - Should build in mock. OK - Should build on all supported archs OK - Should have dist tag OK - Should package latest version Issues: 1. You might add "|| :" to the end of your install-info calls. See:http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-47896da5fb2662d75deefeb9ba75145a398515db 2. Might add AUTHORS and NEWS as doc files? 3. You should use %{?_smp_mflags} with your make call unless it doesn't work. If it doesn't please add a comment that it's not being used. 4. You seem to be missing a BuildRequires: desktop-file-utils Which causes the build to fail. Adding that gets it working. 5. The "Application" Categories in your desktop file is no longer used. It should be removed. See: http://standards.freedesktop.org/menu-spec/latest/apa.html 6. Is redhat-graphics an appropriate icon for this app?
> 1. You might add "|| :" to the end of your install-info calls. > 2. Might add AUTHORS and NEWS as doc files? > 3. You should use %{?_smp_mflags} > 4. You seem to be missing BuildRequires: desktop-file-utils > 5. The "Application" Categories in your desktop file is no longer used. > 6. Is redhat-graphics an appropriate icon for this app? All should be fixed, I switch icon to eog in absent of something better. SPEC: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv.spec SRPM: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv-0.8-2.fc7.src.rpm
Thanks for the quick fixes. I see 2 issues now: 1. You added AUTHORS and NEWS to your chmod line, but didn't add them in %doc under files, so they are still not shipped. 2. Not sure using the eog icon is a good idea. This is after all not that application. Perhaps you could ask upstream? Or possibly request an icon from http://fedoraproject.org/wiki/Artwork/DesignService ? Perhaps for now this shouldn't have any icon?
> 1. You added AUTHORS and NEWS to your chmod line, but didn't add them in %doc > under files, so they are still not shipped. :-) fixed > 2. perhaps for now this shouldn't have any icon? What about image-viewer.png from gnome-icon-theme? Seems like a good fit. I see that gv is doing the same thing (using postscript-viewer.png from gnome-icon-theme). Also added xterm add to req, as the help is based on forking a xterm.
spec: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv.spec srpm: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv-0.8-3.fc7.src.rpm
Yeah, using that icon seems like a good solution. However, should you not then require gnome-icon-theme? Or is there some dependency here that would bring it in? I don't see one off hand...
> However, should you not then require gnome-icon-theme? Done. > Or is there some > dependency here that would bring it in? I don't see one off hand... only gtk2 and hicolor-icon-theme which in turn requires coreutils. gnome-session also need gnome-icon-theme, hence g-i-t is most likely installed on most systems already. spec: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv.spec srpms: http://web.phys.ntnu.no/~terjeros/xzgv/xzgv-0.8-4.fc7.src.rpm
ok, that looks good. I see no further blockers here... so this package is APPROVED. Don't forget to close this review request once it's been imported and built. Also, do consider going and reviewing some other packages waiting in the queue.
> ok, that looks good. I see no further blockers here... so this package is APPROVED. Thanks! > Also, do consider going and reviewing some other packages waiting in the queue. yes, just did one yesterday. BTW: could you please have a look at #229521 . New Package CVS Request ======================= Package Name: xzgv Short Description: A GTK+/Imlib-based picture viewer for X Owners: terjeros.no Branches: FC-6 F-7
Is there a reason you removed the fedora-review + flag? That should stay to show this package is approved. > BTW: could you please have a look at #229521 Apparently not. "You are not authorized to access bug #229521"
> Is there a reason you removed the fedora-review + flag? > That should stay to show this package is approved. Mistake, I set fedora-review to ?, realised that was wrong, fedora-cvs should be ?, then set fedora-review to <blank> which is of course also wrong. Sorry. > Apparently not. "You are not authorized to access bug #229521" I need some sleep (past midnight here..), typo it's: #229591 .
cvs done.