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=716299 --- Comment #7 from Christoph Wickert <cwickert@xxxxxxxxxxxxxxxxx> 2011-07-08 22:26:17 EDT --- Thanks for the pre-review. Most things are ok, however there are a few smaller problems that I'd like to explain. I hope both of you can learn a little from it. (In reply to comment #6) > FIX - MUST: The spec file for the package MUST be legible. > * It's legible but, if possible, use the full length of a line for the > description, up to 80 characteres. The first and the second lines are OK. You could also use a list for the features: ClipIts main features are: * Save a history of your last copied items * Search through the history * Global hot-keys for most used functions * Execute actions with clipboard items * Exclude specific items from history If you use markdown syntax, PackageKit will display a nice bulleted list. > * Please, add gettext as a build dependency. See: > http://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files On Fedora adding gettext is not needed because intltool already required gettext-devel (which requires gettxt). In older versions and in EPEL however this is not the case, so requiring gettext manually is a good idea. I suggest to add it. > * You need to add the dependencies in Requires. According to the file > src/main.c (line 233) and the README file (line 25) provided by upstream, you > need the package xdotool and the package gtk2. xdotool is a good catch and looking at the source is even better. :) However gtk2 doesn't need to be added manually because of $ rpm -qp --requires clipit-1.4.1-2.fc16.x86_64.rpm | grep gtk libgtk-x11-2.0.so.0()(64bit $ rpm -q --whatprovides "libgtk-x11-2.0.so.0()(64bit)" gtk2-2.24.4-2.fc15.x86_64 Generally speaking one should never require libraries explicitly but rely on rpm. Only commands like "xdotool" are required. > * The value "Application" for key "Categories" is deprecated. You can > remove it using desktop-file-install with the option --remove-category. Use > desktop-file-validate to check. See: > http://fedoraproject.org/wiki/Packaging/Guidelines#desktop Another good catch. Note that if you don't change anything, you can just use desktop-file-validate. > FIX - SHOULD: At the beginning of the %install section, the package runs rm -rf > %{buildroot} > * Please, consider to add this command. Again this is something that is no longer required, however I tend to add it for compatibility with older versions of rpmbuild or other systems. > * The icon doesn't appear in system tray (I'm using GNOME 3, Fedora 15), > but you can contact upstream about this. I guess this is a GNOME problem. works fine in Xfce and LXDE and there is a tray icon. However it is not the correct icon but image-mising.png. This is because the package doesn't update the gtk icon-cache after (un)installation. Please read http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache to fix this. IHMO there is no need to contact upstream about this. However I found a bug in the German translation. I had already fixed the same bug in parcellite (clipit is a fork of parcellite) and sent it upstream. Nikos, please make a patch similar to this one: http://pkgs.fedoraproject.org/gitweb/?p=parcellite.git;a=blob;f=parcellite-0.9.2-de.po.patch;h=f7440c28f203b66ee224b3e27db51784fd91e444;hb=33483b327da2ac4dada14712453db3dc702f108c and send it upstream. Fix the other issues that Elder and I found and let me have a look over the package before I approve it. Good work of both of you! -- 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review