Hi,

Just a few comments.

   - You could add  build requires dependencies one per line:
      BuildRequires: gtk2-devel
      BuildRequires: intltool
      BuildRequires: desktop-file-utils

    It's more readable.

  - Use the full length of a line for description, up to 80 characters.
  - Consider to add %doc section after %defattr and before the package files.

$ rpmlint /var/lib/mock/fedora-15-x86_64/result/clipit-* 
clipit.x86_64: W: non-conffile-in-etc /etc/xdg/autostart/clipit-startup.desktop
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

   - Mark the file %{_sysconfdir}/xdg/autostart/%{name}-startup.desktop as %config in the spec file.

Thanx for the advice. I wasn't aware of the %config option.

I updated both spec and srpm:

Spec URL: http://comzeradd.fedorapeople.org/specs/clipit.spec
SRPM URL: http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-15/SRPMS/clipit-1.4.1-2.fc15.src.rpm

Hello, 

This is the same spec file. but the srpm is OK.

Good luck.

Spec looks good, I'm going to review this.

Thanks,

I'll try to do this pre-review.

Hi Nikos,

I'm not a sponsor (Christoph is a sponsor). I'm not allowed to approve the package yet. So, this is an informal review.

OK - MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.
$ rpmlint /var/lib/mock/fedora-15-x86_64/result/clipit-* 
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

OK - MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
OK - MUST: The package must meet the Packaging Guidelines.
MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
    * The license of this package is GPLv3+

OK - MUST: The License field in the package spec file must match the actual license.
OK - MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
OK - MUST: The spec file must be written in American English.
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.
     
OK - MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
     * Source match the upstream by md5: 0f255859469dbdf8ad5408556cb610b8

OK - MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
     * Koji task Info: http://koji.fedoraproject.org/koji/taskinfo?taskID=3187630

N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.
FIX - MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
    * Please, add gettext as a build dependency. See: http://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files
    * 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. 

OK - MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
OK - MUST: Packages must NOT bundle copies of system libraries.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker.
OK - MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
OK - MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations).
OK - MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example.
OK - MUST: Each package must consistently use macros.
OK - MUST: The package must contain code, or permissable content.
N/A - MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity).
OK - MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - 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.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release}
N/A - MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
FIX - MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation.
    * 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

FIX - SHOULD: At the beginning of the %install section, the package runs rm -rf %{buildroot}
    * Please, consider to add this command.

OK - MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time.
OK - MUST: All filenames in rpm packages must be valid UTF-8.

Should Items
============

OK - 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.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: The reviewer should test that the package builds in mock. 
    * Koji task Info: http://koji.fedoraproject.org/koji/taskinfo?taskID=3187630
OK - SHOULD: The package should compile and build into binary rpms on all supported architectures.
FIX - SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
    * Well, the package xdotool is a dependency. If you check Settings > Automatic paste selected item, and select a file from history menu, this message appears:
        /bin/sh: xdotool: command not found.
    * The icon doesn't appear in system tray (I'm using GNOME 3, Fedora 15), but you can contact upstream about this.

N/A: SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
N/A SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb.
OK - 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.
OK - SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense


So, some suggestions:
---------------------
    - Use the full length of a line for the description, up to 80 characteres. See above.
    - Add the packages xdotool and gtk2 (>= 2.10.0) as dependencies. See above
    - Add the package gettext as a build dependency. See above.
    - Please, consider to run the command rm -rf %{buildroot} at the beginning of the %install section
    - Remove value "Application" from key "Category". See above.
    - Contact upstream about system tray icon, if possible.

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!

Hello Christoph,

Many thanks for your advice. Use a list is very nice!

I fixed the errors. In details:

- I made a list of features on the description to beautify it.

- Added gettext as build requirement

- Added xdotool as requirement

- Removed the deprecated "Application" category from .desktop file

- Added "rm -rf %{buildroot}" on %install section for compatibility reasons.

- I added the necessary commands in order to update gtk icons cache, although I couldn't find what does the %posttrans section. Please guide me.

- I added the patch fixing the German translation inconsistency. I also sent it upstream: http://sf.net/tracker/?func=detail&aid=3367028&group_id=369179&atid=1538558

Many thankns on both of you.

SPEC: http://comzeradd.fedorapeople.org/clipit/clipit.spec
SRPM: http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-15/SRPMS/clipit-1.4.1-3.fc15.src.rpm

rpmlint is good.

Hi Nikos, 

%pretrans and %posttrans are run before and after a transaction. A brief explanation here:

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Syntax

and here:

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Scriptlet_Ordering

About rpm transactions:
http://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch-transactions.html

It's a draft, but very useful.

REVIEW FOR f398282810c9e53313128e4a7c7a23e1  clipit-1.4.1-3.fc15.src.rpm


OK - MUST: $ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/clipit-*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK - MUST: named according to the Package Naming Guidelines
OK - MUST: spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines (GPLv3+)
OK - MUST: License field in spec file matches the actual license
OK - MUST: license file included in %doc
OK - MUST: spec is in American English
OK - MUST: spec is legible
OK - MUST: sources match the upstream source by MD5 0f255859469dbdf8ad5408556cb610b8
OK - MUST: successfully compiles and builds into binary rpms on x86_64
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: all build dependencies are listed in BuildRequires.
OK - MUST: handles locales properly with %find_lang
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
OK - MUST: Package does not bundle copies of system libraries.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: owns all directories that it creates (none)
OK - MUST: no duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, includes %defattr(...)
OK - MUST: consistently uses macros
OK - MUST: package contains code, or permissable content
N/A - MUST: Large documentation files should go in a -doc subpackage
OK - MUST: Files included as %doc do not affect the runtime of the application
N/A - MUST: Header files must be in a -devel package
N/A - MUST: Static libraries must be in a -static package
N/A - MUST: library files that end in .so are in the -devel package.
N/A - MUST: devel packages must require the base package using a fully versioned dependency
OK - MUST: The package does not contain any .la libtool archives.
OK - MUST: Package contains a GUI application and includes a %{name}.desktop file, and that file is properly installed with desktop-file-install in the %install section.
OK - MUST: package does not own files or directories already owned by other packages.
OK - Should: at the beginning of %install, the package runs rm -rf %{buildroot}
OK - MUST: all filenames valid UTF-8


SHOULD Items:
OK - SHOULD: Source package includes license text(s) as a separate file.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: builds in mock.
OK - SHOULD: compiles and builds into binary rpms on all supported architectures.
OK - SHOULD: functions as described.
OK - SHOULD: Scriptlets are sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg
OK - SHOULD: no file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin
OK - SHOULD: package should contain man pages for binaries/scripts.

Other items:
OK - latest stable version
OK - SourceURL valid
OK - Compiler flags ok
OK - Debuginfo complete
OK - SHOULD: package has a %clean section, which contains rm -rf %{buildroot}
N/A - SHOULD: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.


One very minor thing: You could add preserve timstamps of the original files during install with

make install DESTDIR=%{buildroot} INSTALL='install -p'

but this only affects the svg and the docs, the rest is created during build. See https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

But this is trivial and not a blocker, this package is APPROVED. Well done!

Before I am going to sponsor you I'd like you to do at least one informal pre-review. Just like Elder did in this review. You can also help out in review with hints. The more you can show me, the quicker I will sponsor you.

Thank you Christoph! I'll do a Pre-Review on another ticket asap.

I also have 3 more review tickets pending.

Thanx again :)

Hi Nikos, did you find some time to look at other packages? I'd like to move on with this bug and to sponsor you.

Hi Christoph. Sorry for the delay, but we are kind in a vacation mode here in Greece :) I'm getting back to packaging right away. I'll soon have something.

Hi Christoph,

I did an informal review on:
https://bugzilla.redhat.com/show_bug.cgi?id=733603

Sorry it took so long. I have now sponsored you. Please continue with the SCM request.

New Package SCM Request
=======================
Package Name: clipit
Short Description: A lightweight, fully featured GTK+ clipboard manager
Owners: comzeradd
Branches: f15 f16
InitialCC:

Git done (by process-git-requests).

Package Change Request
======================
Package Name: clipit
New Branches: el6
Owners: comzeradd

Git done (by process-git-requests).