Bug 636819

+ = OK
- = NA
? = issue


+ Package meets naming and packaging guidelines
+ Spec file matches base package name.
? Spec has consistent macro usage.
+ Meets Packaging Guidelines.
+ License
? License field in spec matches
? License file included in package
+ Spec in American English
? Spec is legible.

- Sources match upstream md5sum:

- Package needs ExcludeArch
+ BuildRequires correct
- Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
? Package has %defattr and permissions on files is good.
+ Package has a correct %clean section.
? Package has correct buildroot
 %{_tmppath}/%{name}
+ Package is code or permissible content.
+ Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

- Package is a GUI app and has a .desktop file

+ Package compiles and builds on at least one arch.
http://koji.fedoraproject.org/koji/taskinfo?taskID=2489884

+ Package has no duplicate files in %files.
+ Package doesn't own any directories other packages own.
+ Package owns all the directories it creates.
? No rpmlint output.

- final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =;
rpm -qp --requires $i; echo; done
manually indented after checking each line.  I also remove the rpmlib junk and
anything provided by glibc.)


XXXXX ISSUES XXXXXX


1. Spec has consistent macro usage.

All instances of gnome-exe-thumbnailer can be replaced by %{name} macro

2. License field in spec matches

The license is LGPLv2+

3. License file included in package

License file is not included in the package. Ask upstream to include license files in next release.

4. Spec is legible.

Provide spaces/lines between %pre,%build,%install tags and %post %preun tags

5. Package has correct buildroot
Buildroot should be
 %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

6. ? No rpmlint output.
-------------------------
[zer0c00l@gnubox SPECS]$ rpmlint ~/rpmbuild/RPMS/noarch/gnome-exe-thumbnailer-0.6-0.fc13.noarch.rpm

#Fix the spelling
gnome-exe-thumbnailer.noarch: W: spelling-error Summary(en_US) thumnailes -> thumbnails, thumbnail, thumbstalls

gnome-exe-thumbnailer.noarch: W: spelling-error %description -l en_US py -> pt, p, y

#License is LGPLv2+
gnome-exe-thumbnailer.noarch: W: invalid-license LGPL

#Remove _executable_ permission from the all other files other than the gnome-exe-thumbnailer.sh file

gnome-exe-thumbnailer.noarch: E: script-without-shebang /etc/gconf/schemas/gnome-exe-thumbnailer.schemas
gnome-exe-thumbnailer.noarch: W: spurious-executable-perm /usr/share/doc/gnome-exe-thumbnailer-0.6/README

Example:
----------
install -pm 755 gnome-exe-thumbnailer.sh $RPM_BUILD_ROOT%{_bindir}/
install -pm 644 gnome-exe-thumbnailer.schemas $RPM_BUILD_ROOT%{_sysconfdir}/gconf/schemas/
install -pm 644  README $RPM_BUILD_ROOT%{_datadir}/doc/%{name}-%{version}/
install -pm 644 gnome-exe-thumbnailer-template.png $RPM_BUILD_ROOT%{_datadir}/pixmaps/
install -pm 644 gnome-exe-thumbnailer-generic-x.png $RPM_BUILD_ROOT%{_datadir}/pixmaps/
install -pm 644 gnome-exe-thumbnailer-generic.png $RPM_BUILD_ROOT%{_datadir}/pixmaps/

#can be ignored
gnome-exe-thumbnailer.noarch: W: no-manual-page-for-binary gnome-exe-thumbnailer.sh

#This is Ok you can ignore the below warnings
gnome-exe-thumbnailer.noarch: W: dangerous-command-in-%pre rm
gnome-exe-thumbnailer.noarch: W: dangerous-command-in-%post rm
1 packages and 0 specfiles checked; 1 errors, 7 warnings.

Forgot!

Instead of modifying the upstream sources. Do your deletion in %prep section.

Avoid using macros for trivial commands like 'install' and 'rm'

I've updated the spec file and the src.rpm according to your review.

is it possible to use the %{name} maro in the files section of the spec?

(In reply to comment #3)
> I've updated the spec file and the src.rpm according to your review.
> 

1. When you update a spec file, you need to increase (bump) the Release: by 1 and tell about your modifications in Changelog. This is how we track all the modifications done to  a spec file. You haven't done that.

2. Please update your SRPM as well, not just the spec file and provide the links here.


your %install section is wrong,

>install -D gnome-exe-thumbnailer.sh $RPM_BUILD_ROOT%{_bindir}/gnome-exe-thumbnailer.sh

Why do you want above line?

> is it possible to use the %{name} maro in the files section of the spec?

yes you can use the macro anywhere in the spec file.

Also, do not forget to run 'rpmlint' on your RPMS and try to remove common rpmlint warnings/errors https://fedoraproject.org/wiki/Common_Rpmlint_issues

i've fixed more errors.
new src.rpm link: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.6-1.fc13.src.rpm

rpmlint issue: 

gnome-exe-thumbnailer.noarch: W: non-conffile-in-etc /etc/gconf/schemas/gnome-exe-thumbnailer.schemas

Mark your schemas file as config file in %config section

%config %{_sysconfdir}/gconf/schemas/%{name}.schemas

Once done, please go through http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

done. 
new src.rpm file: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.6-2.fc13.src.rpm

fixed a typo:
http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.6-3.fc13.src.rpm

Align your spec files. Adjust the tab position like in http://sagarun.fedorapeople.org/SPECS/sqlninja.spec

Suggest you to do unofficial reviews of other packages and post the link here.

(In reply to comment #11)
> Align your spec files. Adjust the tab position like in
Done.
> 
> Suggest you to do unofficial reviews of other packages and post the link here.
I will.

In other news, upstream released a new version today. They include the license now.
New src.rpm: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.7-0.fc13.src.rpm

i've done my first unofficial review: https://bugzilla.redhat.com/show_bug.cgi?id=635788#c4

Please update both the spec and srpm with each modification

(In reply to comment #14)
> Please update both the spec and srpm with each modification

What make you think i didn't update the spec?

Hi Elad,

I'm a sponsor and I like how you've been very responsive so far in this bug report. I would like to sponsor you, but first I would like to see some more of your work / packaging skills. Perhaps there is another piece of sw which you would like to have packaged, which you can also package up and submit a review request for?

Or perhaps you can do another unofficial package review ?

Regards,

Hans

Correctly I can't seem to find another piece of software that I would like to pack.
I will do another unofficial package reviews as soon as possible, and I'll try to search some more for software to pack. 

Thanks!

I'm sorry it took me such long time, but I've been very busy lately.

I've created to more packages, for stopmotion, and for vgrabbj. 
Bug #659951
Bug #659950

I did another unofficial package review: Bug #627032

But I think I'll have to drop my two other packages that are pending review because of dead upstream...

(In reply to comment #19)
> I did another unofficial package review: Bug #627032
> 
> But I think I'll have to drop my two other packages that are pending review
> because of dead upstream...

Hi,

Great to see that you're still working actively to become a Fefora packager. And I'm so sorry for not responding sooner. I've changed teams at my work and I've been crazy busy with tasks for the new team ever since. I've put looking at your package submissions high on my to do list and I hope to get around to them sometime this week.

WRT dead upstreams, that is unfortunate. However if the software in question is reasonably complete and stable, and you're actively using it (ie have a purpose for it), then it is fine to package software with a dead upstream. We've plenty of examples of that. It is not an ideal solution though. So you'll need to decide if you want to move forward with those 2 submissions as well, or if you want to drop them. If you want to drop them, you can simply close the bugs, with a short explanation why you're dropping them.

Thanks & Regards,

Hans

The problem with packaging software with dead upstream is that I'll have to handle all bugs myself, and I don't know enough C or C++.
I'm trying to find a way to make stopmotion use gstreamer instead of vgrabbj, but I haven't succeeded yet.  
Although I use stopmotion and it works well for my usage, it has many bugs and problems. I would fork it an continue working on it (I have a lot of ideas on how to improve it) but I don't have the programming skills to do so.

Therefore I think it's better if I'll drop vgrabbj (which is only used for stopmotion) and stopmotion...

Another unofficial review: bug #689685

I did another unofficial review: bug #691635

I did another unofficial review: bug #691894

How many unofficial reviews do I still need to do?

(In reply to comment #21)
> The problem with packaging software with dead upstream is that I'll have to
> handle all bugs myself, and I don't know enough C or C++.
> I'm trying to find a way to make stopmotion use gstreamer instead of vgrabbj,
> but I haven't succeeded yet.  
> Although I use stopmotion and it works well for my usage, it has many bugs and
> problems. I would fork it an continue working on it (I have a lot of ideas on
> how to improve it) but I don't have the programming skills to do so.
> 
> Therefore I think it's better if I'll drop vgrabbj (which is only used for
> stopmotion) and stopmotion...

Ok, that (dropping them) is fine with me.

(In reply to comment #25)
> How many unofficial reviews do I still need to do?

Hi, none you've done plenty! Sorry if I've been unclear about that. As said in comment #20 I hope to make time to do an official review of gnome thumbnailer soon, and I'll look at your unofficial reviews then too!

The only thing we are waiting for atm is for me to have / make some time for this.

Regards,

Hans

(In reply to comment #26)
> (In reply to comment #21)
> > The problem with packaging software with dead upstream is that I'll have to
> > handle all bugs myself, and I don't know enough C or C++.
> > I'm trying to find a way to make stopmotion use gstreamer instead of vgrabbj,
> > but I haven't succeeded yet.  
> > Although I use stopmotion and it works well for my usage, it has many bugs and
> > problems. I would fork it an continue working on it (I have a lot of ideas on
> > how to improve it) but I don't have the programming skills to do so.
> > 
> > Therefore I think it's better if I'll drop vgrabbj (which is only used for
> > stopmotion) and stopmotion...
> 
> Ok, that (dropping them) is fine with me.
> 
> (In reply to comment #25)
> > How many unofficial reviews do I still need to do?
> 
> Hi, none you've done plenty! Sorry if I've been unclear about that. As said in
> comment #20 I hope to make time to do an official review of gnome thumbnailer
> soon, and I'll look at your unofficial reviews then too!
> 
> The only thing we are waiting for atm is for me to have / make some time for
> this.
> 
> Regards,
> 
> Hans

Thanks! ☺

Please note that gnome-exe-thumbnailer would not function in Fedora 15. This is due to the move of gnome from gconf to gsettings, and gnome-exe-thumbnailer's upstream doesn't provide a gsettings schema (yet). 

I reported this bug upstream.

Created attachment 491115 [details]
thumbnailer config file for gnome-3

Hi,

I've been working on a review today, but got side-tracked. I noticed your last comment that gnome-exe-thumbnailer won't work with gnome-3. I didn't like the idea of introducing a new package which won't work with the latest Fedora, so I set out to fix it. The attached file is a thumbnailer config file for gnome 3.

If you add it to your package, and install it as:
/usr/share/thumbnailers/gnome-exe-thumbnailer.thumbnailer

gnome-exe-thumbnailer will start working with gnome-3 too :)

Can you please:
1) Do a new version of your gnome-exe-thumbnailer package including gnome-3 support
2) Attach this file to the upstream bug report you filed for this.

Thanks & Regards,

Hans

BTW, as discussed I'll review this package and sponsor you, so lets update the bugs status.

Done.
New SRPM: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.7-1.fc15.src.rpm

I had to change %u to %i in the thumbnailer file to make it work, here is the new file: http://elad.fedorapeople.org/gnome-exe-thumbnailer.thumbnailer

SPEC remains http://elad.fedorapeople.org/gnome-exe-thumbnailer.spec

Bugfix: add missing dependency to make sure gnome-exe-thumnailer would not fail when trying to display the version of exe files. 

New SRPM: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.7-2.fc15.src.rpm

And here is the long promised full review:

Good:
=====
- rpmlint checks return:
 gnome-exe-thumbnailer.src: W: spelling-error %description -l en_US py -> pt, p, y
 gnome-exe-thumbnailer.noarch: W: spelling-error %description -l en_US py -> pt, p, y
 gnome-exe-thumbnailer.noarch: W: no-manual-page-for-binary gnome-exe-thumbnailer.sh
 These can be ignored
- package meets naming guidelines
- package meets packaging guidelines
- license (LGPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Needs work:
===========
- rpmlint says:
 gnome-exe-thumbnailer.noarch: W: empty-%pre
 gnome-exe-thumbnailer.noarch: W: empty-%post
 gnome-exe-thumbnailer.noarch: W: empty-%preun
 This can simply be fixed by moving the "%if 0%{?fedora} < 15"
 above the %pre / %post / %preun
- %pre is in a weird place in the specfile, normally all installation
  scripts (be it pre or post) are put between  the %clean and the
  %files section in the file
- %post / %preun are in a bit of a weird place either, not as weird as
  %pre thoug. But normally they are after %clean instead of before it
- Please put a blank line in the changelog before each data-name-version line
  (except for the first). And prefix / itemize items below the
  data-name-version line with "- ". IE change:
   * Sun Sep 26 2010 Elad Alfassa <el.il.il> - 0.6-3
   Fix typo.
   * Sun Sep 26 2010 Elad Alfassa <el.il.il> - 0.6-2
   Mark schemas file as config file
   * Sun Sep 26 2010 Elad Alfassa <el.il.il> - 0.6-1
   More spec file fixes
   * Thu Sep 23 2010 Elad Alfassa <elad.il> - 0.6-0
   Version update, some general spec file fixes.
   * Sun Jul 25 2010 Elad Alfassa <elad.il> - 0.3-1
   initial build
  to:
   * Sun Sep 26 2010 Elad Alfassa <el.il.il> - 0.6-3
   - Fix typo.

   * Sun Sep 26 2010 Elad Alfassa <el.il.il> - 0.6-2
   - Mark schemas file as config file

   * Sun Sep 26 2010 Elad Alfassa <el.il.il> - 0.6-1
   - More spec file fixes

   * Thu Sep 23 2010 Elad Alfassa <elad.il> - 0.6-0
   - Version update, some general spec file fixes.

   * Sun Jul 25 2010 Elad Alfassa <elad.il> - 0.3-1
   -initial build

- Release should start at 1 after a version bump, not 0. This is not something
  to fix retroactively but to keep in mind for the next time upstream
  releases a new version.


All the need work items are rather small things, so all in all this looks good.
Based on your informal reviews as well as your withdrawn submission for 2 more packages I'm ready to sponsor you as soon as a new gnome-exe-thumbnailer fixing the mentioned (small) issues are fixed.

Fixed.
New SRPM: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.7-3.fc15.src.rpm

Looks good now, approved!

So you can create a FAS account now if you've not done so already, see:
http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account

Then let me know your FAS account username and I'll add you to the packagers group and sponsor you.

I already have a FAS account, the username is elad.

Ok, added you to the packager group and sponsored you, so you can now move forward with creating a scm admin request to create a module in pkg git for gnome-exe-thumbnailer per:
http://fedoraproject.org/wiki/Package_SCM_admin_requests

Note that it may take up to an hour for your new packager rights to propagate, and that you will not be able to set the fedora-cvs bugzilla flag until these rights have propagated.

If you've any questions regarding the next steps of getting the package imported / build / updates issued for F-15 / F-14 / F-13 don't hesitate to ask me!

Thanks!

New Package SCM Request
=======================
Package Name: gnome-exe-thumbnailer
Short Description: Shows thumbnails of exe files in nautilus
Owners: elad
Branches: f13 f14 f15
InitialCC: elad

Git done (by process-git-requests).

gnome-exe-thumbnailer-0.7-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/gnome-exe-thumbnailer-0.7-3.fc15

gnome-exe-thumbnailer-0.7-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/gnome-exe-thumbnailer-0.7-3.fc14

gnome-exe-thumbnailer-0.7-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/gnome-exe-thumbnailer-0.7-3.fc13

gnome-exe-thumbnailer-0.7-3.fc14 has been pushed to the Fedora 14 testing repository.

gnome-exe-thumbnailer-0.7-4.fc14 has been pushed to the Fedora 14 stable repository.

gnome-exe-thumbnailer-0.7-4.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.