Bug 1034

Good news! I have been sponsored in Fedora, so I think we can continue with the review for this package.

Spec URL: http://rpms.infoglobe.ca/packages/sinthgunt.spec
SRPM URL: http://rpms.infoglobe.ca/packages/sinthgunt-2.0.2-2.fc12.src.rpm


I made some small corrections.

Small ping, I would like to go on with this package to include it in our default Fedora deployment. If anybody can help with the review of this package, that would be great.

Thank you for your time.

(In reply to comment #0)
> Spec URL: http://rpms.infoglobe.ca/packages/sinthgunt.spec
> SRPM URL: http://rpms.infoglobe.ca/packages/sinthgunt-2.0.2-1.fc12.src.rpm
> Description: 
> Sinthgunt is an open source graphical user interface for ffmpeg, a computer
> program that can convert digital audio and video into numerous formats.
> Using pre-configured conversion settings, it makes the task of converting
> between different media formates very easy.
> This package is not eligible in Fedora because it depends on ffmpeg from
> rpmfusion-free repository.
> $ rpmlint -v SPECS/sinthgunt.spec 
> 0 packages and 1 specfiles checked; 0 errors, 0 warnings.
> $ rpmlint -v SRPMS/sinthgunt-2.0.2-1.fc12.src.rpm 
> sinthgunt.src: I: checking
> 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
> $ rpmlint -v RPMS/noarch/sinthgunt-2.0.2-1.fc12.noarch.rpm 
> sinthgunt.noarch: I: checking
> sinthgunt.noarch: E: non-executable-script
> /usr/lib/python2.6/site-packages/Sinthgunt/app.py 0644 /usr/bin/python
> 1 packages and 0 specfiles checked; 1 errors, 0 warnings.
You need remove shebang from this file if it is unexecutable or add exec permission for it on the contrary. 

Remove shebang from files 
There are several ways to remove a shebang from a file 

sed -i -e '/^#!\//, 1d' %{buildroot}%{sugaractivitydir}Test.activity/*.pyIf you have several files, you can do: 

for Files in demo.py utils.py __init__.py test.py; do
  %{__sed} -i.orig -e 1d ${Files}
  touch -r ${Files}.orig ${Files}
  %{__rm} ${Files}.orig
done

Exec permission 
If rpmlint rightly complains about permission, you may have to set those. 

chmod a+x %{buildroot}%{sugaractivitydir}Analyze.activity/analyze.pyIf you have more than one file 

for file in %{buildroot}%{python_sitelib}/Onboard/{settings,settings-dialog,Keyboard,utils}.py; do
   chmod a+x $file
doneor 

chmod a+x %{buildroot}%{python_sitelib}/Onboard/{settings,settings-dialog,Keyboard,utils}.py

Spec URL: http://rpms.infoglobe.ca/packages/sinthgunt.spec
SRPM URL: http://rpms.infoglobe.ca/packages/sinthgunt-2.0.2-3.fc12.src.rpm


Thank you for your review. I have removed the shebang from the file. Also, I have added a missing BuildRequires for desktop-file-utils.

I you see anything else, please let me know.

1) Use %{__python} instead of python
2) License is GPLv3+
3) You can drop pygtk2 from Requires since pygtk2-libglade requires it

4) You are missing a Requires of mplayer

Spec URL: http://rpms.infoglobe.ca/packages/sinthgunt.spec
SRPM URL: http://rpms.infoglobe.ca/packages/sinthgunt-2.0.2-4.fc13.src.rpm


Thanks for taking the time to review my package. I made the corrections as asked.

Build fine and report no major error in rpmlint.


Thank you!

Spec URL: http://djj-consultants.com/rpms/sinthgunt.spec
SRPM URL: http://djj-consultants.com/rpms/sinthgunt-2.0.3-1.fc14.src.rpm


I updated the build to the new upstream version.

(In reply to comment #9)
> Spec URL: http://djj-consultants.com/rpms/sinthgunt.spec
> SRPM URL: http://djj-consultants.com/rpms/sinthgunt-2.0.3-1.fc14.src.rpm
> 
> I updated the build to the new upstream version.

Are you interested in pursing this review request? If so I'll review it for you. Also since you're sponsored in fedora make you you send a message to the list so Nicolas can clear you as a sponsor on RPM Fusion.

Richard

Spec URL: http://djj-consultants.com/rpms/sinthgunt.spec
SRPM URL: http://djj-consultants.com/rpms/sinthgunt-2.0.3-1.fc16.src.rpm


Yes, I am still interested in this package. I uploaded a new build for F16 with the same old spec file since there is no new version. I will post on the devel list as well.

Thank you!

Ok, first pass spec review:

1. The following sections or entries are no longer needed and can be removed from the spec:

- Python sitelib macro (only needed for Fedora < 13, RHEL < 6) at the top of the spec.
- BuildRoot: tag
- "rm -rf $RPM_BUILD_ROOT" in %install
- %clean entirely
- "%defattr(-,root,root,-)" in %files

2. There's no hard rule for this, but as a matter of good practices, I usually reserve using "%exclude" to make sure files don't get globbed by one %files section when I want to put them in another. 

In this case, you're not including them in another sub-package so I would just:
rm -f $RPM_BUILD_ROOT%{_datadir}/sinthgunt/README.txt \
      $RPM_BUILD_ROOT%{_datadir}/sinthgunt/LICENSE.txt

in %install instead. This way there are no unpackaged files in $RPM_BUILD_ROOT, excluded or otherwise.

Spec URL: http://djj-consultants.com/rpms/sinthgunt.spec
SRPM URL: http://djj-consultants.com/rpms/sinthgunt-2.0.3-2.fc16.src.rpm


Thank you for your review. I have addressed both of your points in this new version.

I was looking at the files in the package and had some observations:
$ rpm -qlp sinthgunt-2.0.3-2.fc16.noarch.rpm
/usr/bin/sinthgunt
/usr/bin/youtube-dl-sinthgunt
/usr/lib/python2.7/site-packages/Sinthgunt
/usr/lib/python2.7/site-packages/Sinthgunt/__init__.py
/usr/lib/python2.7/site-packages/Sinthgunt/__init__.pyc
/usr/lib/python2.7/site-packages/Sinthgunt/__init__.pyo
/usr/lib/python2.7/site-packages/Sinthgunt/app.py
/usr/lib/python2.7/site-packages/Sinthgunt/app.pyc
/usr/lib/python2.7/site-packages/Sinthgunt/app.pyo
/usr/lib/python2.7/site-packages/sinthgunt-2.0.3-py2.7.egg-info
/usr/share/applications/sinthgunt.desktop
/usr/share/doc/sinthgunt-2.0.3
/usr/share/doc/sinthgunt-2.0.3/LICENSE.txt
/usr/share/doc/sinthgunt-2.0.3/README.txt
/usr/share/pixmaps/sinthgunt.png
/usr/share/sinthgunt
/usr/share/sinthgunt/icon.png
/usr/share/sinthgunt/logo.png
/usr/share/sinthgunt/presets.xml
/usr/share/sinthgunt/sinthgunt.glade
/usr/share/sinthgunt/sinthgunt.html

Putting icons in /usr/share/pixmaps is pretty much depreciated but not forbidden. It would be better to put them in the appropriate /usr/share/icons/ location. I also noticed that there's sinthgunt.pnd which is identical to logo.png, that's probably because the logo is used in the html file. Also, sinthgunt is not square:

$ file pixmaps/sinthgunt.png
pixmaps/sinthgunt.png: PNG image data, 128 x 93, 8-bit/color RGBA, non-interlaced

but icon.png is:

$ file sinthgunt/icon.png
sinthgunt/icon.png: PNG image data, 24 x 24, 8-bit/color RGBA, non-interlaced

BUT it's only 24x24 and the Fedora guidelines indirectly require at least a 48x48 icon.

Ick.

I did all this remotely so I'm not sure what the image files even look like. I'll take a look at that and possible solutions tonight at home.

Thanks for the comment. I will try to solve these as soon as I can (mostly, by the end of the week).

If you can point me to a guideline on what should we do for the icons. Do we provide a new one that is square, do we use the one that upstream provide even if it's not good, etc? It would help as it is the first time that this problem happen to a package I own.

Thank you!

(In reply to comment #15)
> Thanks for the comment. I will try to solve these as soon as I can (mostly, by
> the end of the week).
> 
> If you can point me to a guideline on what should we do for the icons. Do we
> provide a new one that is square, do we use the one that upstream provide even
> if it's not good, etc? It would help as it is the first time that this problem
> happen to a package I own.

Ok, here's the (convoluted) short version :)

The packaging guidelines say we need to adhere to freedesktop's Desktop Entry Specification[1] which in turn says we have to follow the Icon Theme Specification[2], which says we should supply a 48x48 icon as a minimum[3].

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files
[2] http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html
[3] http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#install_icons

Ok, now what to do about it? There's no absolute requirement that I know of that an icon be "square", but if you look in /usr/share/icons/hicolor you'll see that all (or the vast majority) of the directories indicate a 1:1 ratio.

If upstream is at all active, I would petition them for a 48x48 icon and a scalable icon. If you have the scalable, then you can create your own 48x48 (either statically and include it as a Source: or on the fly during package building using mogify from ImageMagick).

Ok, having actually looked at the images now...

The icon is just one rampant horse while the logo is two with a film reel in between. I think this makes a much better icon but we still need to get it in a usable resolution. I'm not a graphic artist but it's pretty easy to increase the  height since the background is white to square it up to 128x128. Then we can make a scaled version in 48x48. I would install both. If you can manage that I'd send it upstream for inclusion. If not, I'll see what I can do but I'm not sure when I can get to it.

Another fix that's needed is that the Icon tag in the desktop file uses the extension of the icon. There's only two options:

1. Use absolute path to the icon file (including the extension)
2. Use just the basename of the icon file and let the DM figure out the extension (preferred). 

So the Icon tag in the desktop file would just be:
Icon=sinthgunt

Ok, last issue. Because this package will not own the directory the icon goes into, per the guidelines (another convoluted issue) we muse Require: the package that does own the directory, in this case "hicolor-icon-theme". Since this is a side effect and not an explicit requirement, not everyone knows about this so it would be good to add a comment above it like:

# We must require the package that owns the directories where the icon file is being installed.
Require: hicolor-icon-theme

OK! I think that's it for now! :) Just remember we need to push upstream to accept these changes as much as possible. That will make your job MUCH easier in the future.


Richard

Small ping to say that I'm not dead! I'm overwhelmed by my current job right now and don't have all the time I would wish for this package but that will come soon.

I got to the point that I placed all the icons in /usr/share/icons/. So I just need to fire up Gimp and work on the icons itself.

(In reply to comment #17)
> Small ping to say that I'm not dead! I'm overwhelmed by my current job right
> now and don't have all the time I would wish for this package but that will
> come soon.

No problem! I occasionally have the same problem.

 
> I got to the point that I placed all the icons in /usr/share/icons/. So I just
> need to fire up Gimp and work on the icons itself.

Gimp is fine but I wonder if there's a magic incantation of "mogrify" that would do it on the fly... That way if upstream changes something it will automatically get updated. I'll ponder that when I have a moment.

Richard

Figured it out! The montage command actually handles this nicely.
Something like this should do it:

# Create square icons from logo file.
montage -crop +0+1 -background white -geometry +0+18 logo.png icon.png
convert -resize 48x48 icon.png %{buildroot}%{_sharedir}/icons/hicolor/48x48/apps/sinthgunt.png
mv icon.png %{buildroot}%{_sharedir}/icons/hicolor/128x128/apps/sinthgunt.png

Spec URL: http://djj-consultants.com/rpms/sinthgunt.spec
SRPM URL: http://djj-consultants.com/rpms/sinthgunt-2.0.3-3.fc16.src.rpm


It's been a while but I have finally find the time to fix the issues. Thanks a lot for pointing me in the right direction with the montage and convert command.

The desktop file, icons and icons location should be fixed.

I also included the require for the icons theme.

I added ImageMagick as a build require to be sure the montage and convert are present.

I tested the package and everything seems to work fine. The logo is there on startup and the icon show correctly under Gnome 3.


Thanks!

Spec looks good, although if you wanted to you could combine the icon lines in %files like this:

%{_datadir}/icons/hicolor/*/apps/sinthgunt.png

Obviously not a blocker though. I'll start my full review.

+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment.
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: LGPLv3+
[+] license field matches the actual license.
[+] license file is included in %doc: LICENCE.txt
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5 sum matches (c04e2f473d395ee2ca2ec1f1b7434091) 
[+] package builds on at least one primary arch: Tested F16 x86_64
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[+] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[N] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[+] query upstream for license text
[N] description and summary contains available translations
[+] package builds in mock
[+] package builds on all supported arches: Tested x86_64
[?] package functions as described: Not tested
[+] sane scriptlets
[+] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[N] package contains man pages for binaries/scripts

*** APPROVED ***

Helps if I remember to change the review block bug to RF_ACCEPT :)

Thanks a lot for taking the time to review my package! I will add the fix to the icons line in the final version when I import my package into rpmfusion.


Package CVS request
======================
Package Name: sinthgunt
Short Description: An easy to use GUI for ffmpeg
Owners: jfsaucier
Branches: f15 f16 f17
InitialCC:
----------------------
License tag: free

If this has made it into the testing or stable repos we can probably go ahead and close the bug.

I'm not sure if packages were ever built but this has been left open over a year. Closing.

I'd like to test it and see what it does.