Bug 636819 - Review Request: gnome-exe-thumbnailer - gnome thumbnailer for exe files
Summary: Review Request: gnome-exe-thumbnailer - gnome thumbnailer for exe files
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-09-23 11:32 UTC by Elad Alfassa
Modified: 2011-04-24 23:57 UTC (History)
5 users (show)

Fixed In Version: gnome-exe-thumbnailer-0.7-4.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-04-23 20:50:56 UTC
Type: ---
hdegoede: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)
thumbnailer config file for gnome-3 (192 bytes, text/plain)
2011-04-10 22:27 UTC, Hans de Goede
no flags Details

Description Elad Alfassa 2010-09-23 11:32:06 UTC
Spec URL: http://elad.fedorapeople.org/gnome-exe-thumbnailer.spec
SRPM URL: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.6-0.fc13.src.rpm
Description: 
"gnome-exe-thumbnailer is a thumbnailer for Gnome that will give Windows .exe files an icon based on their embedded icon and a generic "Wine program" icon. If the program has normal execute permissions, then the standard embedded icon will be shown.  This thumbnailer will also give a thumbnail icon for .jar, .py, and similar executable programs."

please note, this is my first package.

Comment 1 Arun S A G 2010-09-26 10:37:04 UTC
+ = 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.

Comment 2 Arun S A G 2010-09-26 10:43:24 UTC
Forgot!

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

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

Comment 3 Elad Alfassa 2010-09-26 12:52:54 UTC
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?

Comment 4 Arun S A G 2010-09-26 13:28:05 UTC
(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.

Comment 5 Arun S A G 2010-09-26 13:43:17 UTC
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

Comment 6 Elad Alfassa 2010-09-26 15:27:59 UTC
i've fixed more errors.
new src.rpm link: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.6-1.fc13.src.rpm

Comment 7 Arun S A G 2010-09-26 16:51:17 UTC
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

Comment 8 Arun S A G 2010-09-26 16:52:25 UTC
Once done, please go through http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Comment 9 Elad Alfassa 2010-09-26 17:44:34 UTC
done. 
new src.rpm file: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.6-2.fc13.src.rpm

Comment 10 Elad Alfassa 2010-09-28 09:50:40 UTC
fixed a typo:
http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.6-3.fc13.src.rpm

Comment 11 Arun S A G 2010-09-29 02:23:20 UTC
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.

Comment 12 Elad Alfassa 2010-09-29 13:13:27 UTC
(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

Comment 13 Elad Alfassa 2010-09-29 13:57:49 UTC
i've done my first unofficial review: https://bugzilla.redhat.com/show_bug.cgi?id=635788#c4

Comment 14 Hicham HAOUARI 2010-10-04 22:10:31 UTC
Please update both the spec and srpm with each modification

Comment 15 Elad Alfassa 2010-10-05 09:23:27 UTC
(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?

Comment 16 Hans de Goede 2010-11-03 12:02:01 UTC
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

Comment 17 Elad Alfassa 2010-11-03 14:16:44 UTC
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!

Comment 18 Elad Alfassa 2010-12-04 12:15:50 UTC
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

Comment 19 Elad Alfassa 2011-03-28 07:15:39 UTC
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...

Comment 20 Hans de Goede 2011-03-28 07:50:55 UTC
(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

Comment 21 Elad Alfassa 2011-03-28 08:15:57 UTC
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...

Comment 22 Elad Alfassa 2011-03-28 14:26:18 UTC
Another unofficial review: bug #689685

Comment 23 Elad Alfassa 2011-03-29 07:48:43 UTC
I did another unofficial review: bug #691635

Comment 24 Elad Alfassa 2011-03-30 07:11:23 UTC
I did another unofficial review: bug #691894

Comment 25 Elad Alfassa 2011-03-30 10:57:15 UTC
How many unofficial reviews do I still need to do?

Comment 26 Hans de Goede 2011-03-30 11:59:29 UTC
(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

Comment 27 Elad Alfassa 2011-03-30 12:07:48 UTC
(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! ☺

Comment 28 Elad Alfassa 2011-04-06 14:47:25 UTC
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.

Comment 29 Hans de Goede 2011-04-10 22:27:44 UTC
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

Comment 30 Hans de Goede 2011-04-10 22:28:41 UTC
BTW, as discussed I'll review this package and sponsor you, so lets update the bugs status.

Comment 31 Elad Alfassa 2011-04-11 08:18:42 UTC
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

Comment 32 Elad Alfassa 2011-04-11 11:25:15 UTC
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

Comment 33 Hans de Goede 2011-04-11 20:08:22 UTC
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@doom.co.il> - 0.6-3
   Fix typo.
   * Sun Sep 26 2010 Elad Alfassa <el.il@doom.co.il> - 0.6-2
   Mark schemas file as config file
   * Sun Sep 26 2010 Elad Alfassa <el.il@doom.co.il> - 0.6-1
   More spec file fixes
   * Thu Sep 23 2010 Elad Alfassa <elad@macron.co.il> - 0.6-0
   Version update, some general spec file fixes.
   * Sun Jul 25 2010 Elad Alfassa <elad@macron.co.il> - 0.3-1
   initial build
  to:
   * Sun Sep 26 2010 Elad Alfassa <el.il@doom.co.il> - 0.6-3
   - Fix typo.

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

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

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

   * Sun Jul 25 2010 Elad Alfassa <elad@macron.co.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.

Comment 34 Elad Alfassa 2011-04-12 07:44:53 UTC
Fixed.
New SRPM: http://elad.fedorapeople.org/gnome-exe-thumbnailer-0.7-3.fc15.src.rpm

Comment 35 Hans de Goede 2011-04-12 09:02:28 UTC
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.

Comment 36 Elad Alfassa 2011-04-12 09:07:00 UTC
I already have a FAS account, the username is elad.

Comment 37 Hans de Goede 2011-04-12 09:16:47 UTC
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!

Comment 38 Elad Alfassa 2011-04-12 09:25:24 UTC
Thanks!

Comment 39 Elad Alfassa 2011-04-12 10:15:47 UTC
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

Comment 40 Jason Tibbitts 2011-04-12 14:48:51 UTC
Git done (by process-git-requests).

Comment 41 Fedora Update System 2011-04-12 16:30:50 UTC
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

Comment 42 Fedora Update System 2011-04-12 16:36:54 UTC
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

Comment 43 Fedora Update System 2011-04-12 16:37:18 UTC
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

Comment 44 Fedora Update System 2011-04-12 21:24:36 UTC
gnome-exe-thumbnailer-0.7-3.fc14 has been pushed to the Fedora 14 testing repository.

Comment 45 Fedora Update System 2011-04-23 20:50:49 UTC
gnome-exe-thumbnailer-0.7-4.fc14 has been pushed to the Fedora 14 stable repository.

Comment 46 Fedora Update System 2011-04-24 23:57:33 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.