[Bug 755890] Review Request: Snap A modular cross-platform system backup/restore utility

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=755890

--- Comment #5 from Mo Morsi <mmorsi at redhat.com> 2011-11-22 18:35:59 EST ---
Again thank both of you so much for the very quick feedback. This is my first
python package (I've got a ton of ruby ones under my belt) and my first time
packaging a graphical app so also thanks for bearing with me as I work through
the relevant guidelines.

New Spec: http://mo.morsi.org/files/rpms/snap.spec
New SRPM: http://mo.morsi.org/files/rpms/snap-0.5-3.fc15.src.rpm


(In reply to comment #3)
> I assume that the gsnap binary is a GUI[1]?
> 

Yessir

> If so you should probably add a .desktop file and icon. The guidelines cover
> what to do with the desktop file but where to place icons is a little more
> fuzzy :)

Done

> 
> I prefer to use /usr/share/icons/... For instance, if you had a 48x48 and
> scalable (svg) icons named gsnap-48x48.png and gsnap.svg respectively you could
> install them to the following locations (in your makefile or spec):
> 
> /usr/share/icons/hicolor/48x48/apps/gsnap.png (notice I rename the file without
> the size) 
> /usr/share/icons/hicolor/scalable/apps/gsnap.svg
> 
> Then grab them both in %files with:
> 
> %{_datadir}/icons/hicolor/*/apps/*

I just included a scalable svg icon for the time being. This is acceptable
correct?


(In reply to comment #4)
> RPMLint output is not clean:
> 
> > $ rpmlint ./snap-0.5-2.fc15.src.rpm 
> > snap.src: E: summary-too-long C A modular system backup/restore utility which uses the native package management system
> 
> Please keep lines under 40 characters.

Fixed

> 
> > snap.src:34: W: macro-in-comment %check
> > snap.src:35: W: macro-in-comment %{__python}
> 
> Please escape RPM Macros in comments (e.g. %%check)

Fixed

> 
> > snap.src: W: invalid-url Source0: http://mo.morsi.org/files/snap/snap-0.5.tgz HTTP Error 404: Not Found
> 
> The Source0 URL 404s.  Please fix it.  (Be it server-side or in the specfile. 
> ;-)

Fixed (uploaded source to there)

> 
> > 1 packages and 0 specfiles checked; 1 errors, 3 warnings.
> 
> > $ rpmlint snap-0.5-2.fc16.noarch.rpm 
> > snap.noarch: E: summary-too-long C A modular system backup/restore utility which uses the native package management system
> 
> See above.


Fixed

> 
> > snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/adapters/httpd.py 0644L /usr/bin/python
> > snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/filemanager.py 0644L /usr/bin/python
> > <snip>
> 
> Consider removing the unnecessary shebang lines from these files.


Fixed, the shebangs have been removed

> 
> >> snap.noarch: W: non-conffile-in-etc /etc/snap.conf
> 
> Please mark this file with %config or %config(noreplace).  See
> http://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files for more
> information.

Fixed

> 
> > snap.noarch: W: no-manual-page-for-binary gsnap
> > snap.noarch: W: no-manual-page-for-binary snaptool
> 
> You might want to rename the manpage to snaptool.1 to match the binary it
> describes.  gsnap is graphical and doesn't strictly require a manpage, but if
> it has advanced command line options it might be worthwhile to write one
> anyway.
> 

I simply symlinked snaptool.1 to snap.1 since I want 'man snap' to work as
well. gsnap has no command line options (at least not for the time being) so
not included a man page for that.


> > 1 packages and 0 specfiles checked; 34 errors, 3 warnings.
> 
> It seems your package is missing dependencies.  gsnap fails on my system with
> the following error:
> 
> > Traceback (most recent call last):
> >   File "/usr/bin/gsnap", line 33, in <module>
> >     from gi.repository import Gtk, Gdk, GObject
> > ImportError: No module named gi.repository
>

Ah hrm, which version of Fedora are you running against? It always worked for
me right out of the box. Are you running a KDE only environment or such? 

I believe the missing dependency was pygtk2 which I've added. Feel free to
correct me if I'm wrong and I'll track it down.


> Additionally, you also might want to consider packaging the Gtk interface in a
> "snap-gtk" subpackage so command-line only users don't need to install Gtk
> dependencies.

Done, the gtk bits are now shipped in a separate subpackage.

As a side note, the next couple of days is a holiday in the states so I might
not be around. If so I'll get to any more feedback as soon as I get back.
Appreciate it!

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.