Quick comments on the specfile:

- You can use "find . -name '*.cpp' -exec chmod -x {} \;" to chmod the source files, instead of typing them all out manually.  You can repeat this for header files, etc.  It will make your spec more legible and save you from having to add/remove files from the list as the program evolves.

- In the %files section, using the line %{_libdir}/globus will assign ownership of that directory to your package, and install all of the files within it.  You don't have to specify the %dir, and then all of the libraries within it.  The same goes for %{datadir}/gloobus, you can install the directory and the images within it with one line.  If there's some files you don't want to include, you can use %exclude

- Use of macros is inconsistent, some lines use %{name} and some use gloobus.  Pick one or the other to make your spec more clear

- If you're not installing libraries into %{_libdir}, you don't need to run ldconfig.

- You should specify what your patch does, "all-mods" isn't really descriptive.  Breaking your patches up is also a good idea, less maintenance for you if some/all of the changes get merged upstream.  If applicable, you should also supply a link to the upstream bug/patch

Spec URL: http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-preview.spec
SRPM URL: http://git.dignan.net/packaging/gloobus-preview-0.4.1-2.fc13.src.rpm
Patch 0: http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-autotools.patch
Patch 1: http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/fedora-readme.patch

Modified the chmod statement per your recommendation

Changed the files section to reflect that.

The inconsistent use of macros is intentional, gloobus-preview is one of several gloobus-* programs, and they all use the files in %{_libdir}/gloobus.  However, since they all use different data directories, I have changed that.

Fixed that.  I thought I needed to call it for sub-directories as well.

Changed the name of the patch and gave a better description.  It's basically an update to fix the autotools scripts included by default, and the fix has been sent upstream.

1. --libdir=%{_libdir} is unnecessary.
2. find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';' is a less cluttered way to get rid of libtool archives.
3. make install %{_libdir} DESTDIR=$RPM_BUILD_ROOT is most likely incorrect.
4. The homepage is actually https://launchpad.net/gloobus-preview.
5. You might want to use one more %{version} macro in the source url.

Also, wouldn't it be possible to ship a gconf schema with the entries readme is asking to create? Not that many users really read the documentation.

--datadir=%{_datadir}/%{name} is strange. This way icons and desktop entries are sort of hidden from the system.

Created attachment 387664 [details]
icons missing

The path is indeed wrong, have a look at the screenshot - icons are missing.

Updated to fix those issues.

Spec URL: http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-preview.spec
Source1 URL: http://git.dignan.net/packaging/README.fedora.tar.gz
SRPM URL: http://git.dignan.net/packaging/gloobus-preview-0.4.1-3.fc12.src.rpm
Patch0: http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-location-prereconf.patch

Spec URL:
http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-preview.spec
Source1 URL: http://git.dignan.net/packaging/README.fedora.tar.gz
SRPM URL: http://git.dignan.net/packaging/gloobus-preview-0.4.1-4.fc12.src.rpm
Patch0:
http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-location-prereconf.patch    

Fixed desktop file issue.  Updated

Sorry I forgot to tell you earlier, but you need to add icon cache scriptlets:
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
What is more, you need to add libtool to BuildRequres, package does not build in Koji otherwise.

Spec URL:
http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-preview.spec
Source1 URL: http://git.dignan.net/packaging/README.fedora.tar.gz
SRPM URL: http://git.dignan.net/packaging/gloobus-preview-0.4.1-5.fc12.src.rpm
Patch0:
http://git.dignan.net/gloobus-rpm.git/blob_plain/HEAD:/gloobus-location-prereconf.patch 

Addressed those concerns.  Updated package posted.  It now successfully builds in Koji.  Everything seems to behave as expected.

1. rpmlint is silent: rpmlint gloobus-preview-0.4.1-5.fc12.src.rpm  ../RPMS/x86_64/gloobus-preview-*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
2. Name is correct.
3. Spec file name is correct.
4. Package meets packaging guidelines
5. Licensing is OK: GPLv3.
6. Spec is written in American English and is legible.
7. Sources match upstream: d41d8cd98f00b204e9800998ecf8427e
8. Builds fine for f13: https://koji.fedoraproject.org/koji/taskinfo?taskID=1975674
9. BuildRequires are correct.
10. There is no locale to handle.
11. Shared libs are private, no need for ldconfig.
12. Nothing is bundled.
14. Not relocatable.
15. Ownerships are correct.
16. No duplicates in %files.
17. Permissions are correct.
18. %clean section is present.
19. Macros are consistent.
20. Package contains code.
21. No large doc.
22. %doc is not required at runtime.
23. No header files static libs nor pkgconfig files.
24. Libtool archives are removed.
25. Desktop file is included and validated.
26. Buildroot is cleaned at the beginning of %install.
27. Filenames are utf-8.
28. Package seems to work correctly.

New Package CVS Request
=======================
Package Name: gloobus-preview
Short Description: A Gnome extension to enable previews for any kind of file
Owners: dignan
Branches: F-12
InitialCC: dignan

CVS done (by process-cvs-requests.py).

belegdol: Please remember to assign the review to yourself when you take it on. ;)

gloobus-preview-0.4.1-5.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/gloobus-preview-0.4.1-5.fc12