Bug 755890

A couple of quick comments on your spec file

1. It looks like you have mixed spaces and tabs. You can fix it manually or use something like "expand"

2. I've never seen a man page have a .man suffix. Should it not be snap.1?

Hey thanks for the quick feedback. Updated the spec to remove the tabs and change snap.man to snap.1

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

I assume that the gsnap binary is a GUI[1]?

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 :)

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/*

HTH,
Richard

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

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.

> 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)

> 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.  ;-)

> 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.

> 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
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/metadata/service.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/snapshottarget.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/files/sapt.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/mock.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/adapters/iptables.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/packages/syum.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/config.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/packages/mock.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/repos/sapt.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/__init__.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/crypto.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/adapters/mysql.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/packages/win.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/dispatcher.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/adapters/mock.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/linuxdispatcher.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/files/win.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/metadata/sfile.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/repos/mock.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/adapters/postgresql.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/metadata/snapfile.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/exceptions.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/metadata/package.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/windowsdispatcher.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/repos/syum.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/osregistry.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/files/mock.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/files/syum.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/packages/sapt.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/backends/services/adapters/iis.py 0644L /usr/bin/python
> snap.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/snap/callback.py 0644L /usr/bin/python

Consider removing the unnecessary shebang lines from these files.

>> 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.

> 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.

> 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

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.

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!

(In reply to comment #5)
> > Please keep lines under 40 characters.
> 
> Fixed

Glad to see you read that as 80 characters.  ;-)  Sorry about that.

> 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.

Fedora 16.  I do run KDE, but I already had PyGTK2 installed:

$ rpm -q pygtk2
pygtk2-2.24.0-3.fc15.x86_64

I think pygobject3 is what's missing:

# yum whatprovides "/usr/lib/python2.7/site-packages/gi/"
Loaded plugins: fastestmirror, langpacks, presto, refresh-packagekit

pygobject3-3.0.1-1.fc16.i686 : Python 2 bindings for GObject Introspection
Repo        : fedora
Matched from:
Filename    : /usr/lib/python2.7/site-packages/gi/



pygobject3-3.0.2-1.fc16.i686 : Python 2 bindings for GObject Introspection
Repo        : updates
Matched from:
Filename    : /usr/lib/python2.7/site-packages/gi/

(In reply to comment #5)
> > 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?

I think that meets the technical requirements, but here's where the guidelines get "fuzzy". The Fedora guidelines[1] say we need to meet the freedesktop desktop entry specs[2], which in turn say we should follow the Icon Theme Specification[3] which says we should install a 48x48 icon as a minimum[4].

Yeah, it's convoluted :)

I don't think I'd hold up a review for it, but it's not much work to export a 48x48 icon (png) just to be sure :) Although I would be interested in the results across supported desktops (Gnome 2, Gnome Shell, KDE, XFCE, LXDE, etc.) with only the scalable icon. 

If someone went to the trouble of testing all those configurations then I think a valid argument can be made to install only a scalable icon.

Richard

[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
[4] http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#install_icons

> 
> > 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.
> 
> Fedora 16.  I do run KDE, but I already had PyGTK2 installed:
> 
> $ rpm -q pygtk2
> pygtk2-2.24.0-3.fc15.x86_64
> 
> I think pygobject3 is what's missing:

Ah good catch, added the missing dependency


> 
> I don't think I'd hold up a review for it, but it's not much work to export a
> 48x48 icon (png) just to be sure :)

Done, added the 48x48 icon to the project / spec


As a side note, unless I'm missing something it doesn't seem like the icons macros are defined in F16 and up. I had to declare them myself to get it building against rawhide on Koji. Regardless, updated:

New Spec: http://mo.morsi.org/files/rpms/snap.spec
New SRPM: http://mo.morsi.org/files/rpms/snap-0.5-4.fc15.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3534746


I should be sporadically available during the holiday weekend to finish getting Snap in.

(In reply to comment #8)
> > I don't think I'd hold up a review for it, but it's not much work to export a
> > 48x48 icon (png) just to be sure :)
> 
> Done, added the 48x48 icon to the project / spec
> 
> 
> As a side note, unless I'm missing something it doesn't seem like the icons
> macros are defined in F16 and up. I had to declare them myself to get it
> building against rawhide on Koji. Regardless, updated:

Interesting. I never knew there were macros for icons :) Learn something new every day! 

Though from a readability standpoint, I don't think it's any better than using %{_datadir}/icons...

Richard

ping? Do either of you guys happen to be a sponsored packager that can give the official review? Would you mind doing so, so that we can get this in.

Appreciate it.

Yes, sorry. I got busy with family and other projects I was working on. Since T.C. hasn't taken the review officially, I'll take it.

If you don't mind a swap, have a look at:
https://bugzilla.redhat.com/show_bug.cgi?id=753453

It should be a very easy one.

Thanks,
Richard

Ok, I took a look at the new spec ans srpm. I ran rpmlint on the results:

$ rpmlint mockbuild/15/snap/*.rpm
snap.src: W: file-size-mismatch snap-0.5.tgz = 706769, http://mo.morsi.org/files/snap/snap-0.5.tgz = 702522
snap-gtk.noarch: W: spelling-error %description -l en_US frontend -> fronted, front end, front-end
snap-gtk.noarch: W: spelling-error %description -l en_US snapshotter -> snaps hotter, snaps-hotter, snapshot
snap-gtk.noarch: W: no-documentation
snap-gtk.noarch: W: no-manual-page-for-binary gsnap
3 packages and 0 specfiles checked; 0 errors, 5 warnings.

Since you're technically "upstream" for this I assume you can fix the file-size-mismatch issue?

The other warnings can be ignored.

Also, I changed two things in your spec. Not functional issues but more readability/good practices related.

1. Right at the beginning of %install where you create directories I changed it to:
%install
mkdir -p $RPM_BUILD_ROOT%{_mandir}/man1 \
         $RPM_BUILD_ROOT%{_iconsscaldir} \
         $RPM_BUILD_ROOT%{_icons48dir}

instead of letting them wrap at 80 characters. 

2. Although rpmlint didn't complain about the symbolic links for the manpage (it will if the source path is absolute path rather than relative). It still has a much longer path than necessary. The better practice is to change to one of the directories (in this case the source and destination paths are the same) and create the link there with a minimal path, i.e.:

pushd $RPM_BUILD_ROOT/%{_mandir}/man1
ln -s snap.1.gz snaptool.1.gz

which will change the result from:
-rw-r--r--. 1 build build 485 Dec  7 08:38 snap.1.gz
lrwxrwxrwx. 1 build build  29 Dec  7 08:38 snaptool.1.gz -> /usr/share/man/man1/snap.1.gz

to:
-rw-r--r--. 1 build build 485 Dec  7 08:38 snap.1.gz
lrwxrwxrwx. 1 build build  29 Dec  7 08:38 snaptool.1.gz -> snap.1.gz

Full review to come.
Richard

Ok, two more things:

1. If python 2.7 is going to be supported you need to update the Requires for snap-gtk to something like:

%if 0%{?with_python3}
Requires: pygobject3
%else
Requires: pygobject2
%endif

Since pygobject3 doesn't exist on F15 and even if it did it would require that python3 be used.

2. Also, I tried running it on a remote X11 session over ssh and when I clicked the "Backup" button I got the following in the terminal:

$ gsnap
Traceback (most recent call last):
  File "/usr/bin/gsnap", line 83, in show_backup_window
    backup_window = BackupOperationWindow()
  File "/usr/bin/gsnap", line 122, in __init__
    snapfile = snap.options.DEFAULT_SNAPFILE + "-" + snapfile_id + ".tgz"
AttributeError: 'module' object has no attribute 'DEFAULT_SNAPFILE'

Richard

Thanks for the review. Will try to take a look at your package in a bit.

Updated Submission:
New Spec: http://mo.morsi.org/files/rpms/snap.spec
New SRPM: http://mo.morsi.org/files/rpms/snap-0.5-5.fc15.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3573274

(In reply to comment #12)
> Ok, I took a look at the new spec ans srpm. I ran rpmlint on the results:
> 
> $ rpmlint mockbuild/15/snap/*.rpm
> snap.src: W: file-size-mismatch snap-0.5.tgz = 706769,
> http://mo.morsi.org/files/snap/snap-0.5.tgz = 702522
> snap-gtk.noarch: W: spelling-error %description -l en_US frontend -> fronted,
> front end, front-end
> snap-gtk.noarch: W: spelling-error %description -l en_US snapshotter -> snaps
> hotter, snaps-hotter, snapshot
> snap-gtk.noarch: W: no-documentation
> snap-gtk.noarch: W: no-manual-page-for-binary gsnap
> 3 packages and 0 specfiles checked; 0 errors, 5 warnings.
> 
> Since you're technically "upstream" for this I assume you can fix the
> file-size-mismatch issue?
> 

Done. Updated source uploaded and the srpm rebuilt.

> The other warnings can be ignored.
> 
> Also, I changed two things in your spec. Not functional issues but more
> readability/good practices related.
> 
> 1. Right at the beginning of %install where you create directories I changed it
> to:
> %install
> mkdir -p $RPM_BUILD_ROOT%{_mandir}/man1 \
>          $RPM_BUILD_ROOT%{_iconsscaldir} \
>          $RPM_BUILD_ROOT%{_icons48dir}
> 
> instead of letting them wrap at 80 characters. 

Done

> 
> 2. Although rpmlint didn't complain about the symbolic links for the manpage
> (it will if the source path is absolute path rather than relative). It still
> has a much longer path than necessary. The better practice is to change to one
> of the directories (in this case the source and destination paths are the same)
> and create the link there with a minimal path, i.e.:
> 
> pushd $RPM_BUILD_ROOT/%{_mandir}/man1
> ln -s snap.1.gz snaptool.1.gz
> 
> which will change the result from:
> -rw-r--r--. 1 build build 485 Dec  7 08:38 snap.1.gz
> lrwxrwxrwx. 1 build build  29 Dec  7 08:38 snaptool.1.gz ->
> /usr/share/man/man1/snap.1.gz
> 
> to:
> -rw-r--r--. 1 build build 485 Dec  7 08:38 snap.1.gz
> lrwxrwxrwx. 1 build build  29 Dec  7 08:38 snaptool.1.gz -> snap.1.gz

Done


(In reply to comment #13)
> Ok, two more things:
> 
> 1. If python 2.7 is going to be supported you need to update the Requires for
> snap-gtk to something like:
> 
> %if 0%{?with_python3}
> Requires: pygobject3
> %else
> Requires: pygobject2
> %endif
> 
> Since pygobject3 doesn't exist on F15 and even if it did it would require that
> python3 be used.

Done

> 
> 2. Also, I tried running it on a remote X11 session over ssh and when I clicked
> the "Backup" button I got the following in the terminal:
> 
> $ gsnap
> Traceback (most recent call last):
>   File "/usr/bin/gsnap", line 83, in show_backup_window
>     backup_window = BackupOperationWindow()
>   File "/usr/bin/gsnap", line 122, in __init__
>     snapfile = snap.options.DEFAULT_SNAPFILE + "-" + snapfile_id + ".tgz"
> AttributeError: 'module' object has no attribute 'DEFAULT_SNAPFILE'
> 
> Richard

Ah ya this is an upstream bug. Will look into fixing for the next release.

Appreciate it.

Ack! Sorry I keep finding stuff :)

1. There's no documentation in the main package. I would include a line like this, with our without the PDF depending on if it's relevant:

%files
%doc CHANGELOG LICENSE README docs/paper.pdf
... rest of %files section ...

2. Do we need python2 if we're building with python3?

If not change your BuildRequires to:

%if 0%{?with_python3}
BuildRequires:  python3-devel
%else
BuildRequires:  python2-devel
%endif # if with_python3

Richard

(In reply to comment #15)
> Ack! Sorry I keep finding stuff :)
> 
> 1. There's no documentation in the main package. I would include a line like
> this, with our without the PDF depending on if it's relevant:
> 
> %files
> %doc CHANGELOG LICENSE README docs/paper.pdf
> ... rest of %files section ...
> 

Done

> 2. Do we need python2 if we're building with python3?
> 
> If not change your BuildRequires to:
> 
> %if 0%{?with_python3}
> BuildRequires:  python3-devel
> %else
> BuildRequires:  python2-devel
> %endif # if with_python3

I believe the original way is the correct way. At least every package I've seen does not conditionalize the BR: python2-devel and it is this way in the guidelines as well

http://fedoraproject.org/wiki/Packaging:Python

BTW also fixed the DEFAULT_SNAPFILE bug (this will be shipped in the first release after this RPM gets into Fedora, though you can manually apply the fix for now if you want)

https://github.com/movitto/snap/commit/7a53119b1d5bd048a4ddfbdf5bf9962f3d5699dc


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

> > 2. Do we need python2 if we're building with python3?
> > 
> > If not change your BuildRequires to:
> > 
> > %if 0%{?with_python3}
> > BuildRequires:  python3-devel
> > %else
> > BuildRequires:  python2-devel
> > %endif # if with_python3
> 
> I believe the original way is the correct way. At least every package I've seen
> does not conditionalize the BR: python2-devel and it is this way in the
> guidelines as well
> 
> http://fedoraproject.org/wiki/Packaging:Python

I think the section of the python guidelines you're referring to are if you're building for *BOTH* python2 and python3[1]. I believe in your case it's an either/or, so it shuold be conditionalized.

Let me know if that is not the case.


> BTW also fixed the DEFAULT_SNAPFILE bug (this will be shipped in the first
> release after this RPM gets into Fedora, though you can manually apply the fix
> for now if you want)
> 
> https://github.com/movitto/snap/commit/7a53119b1d5bd048a4ddfbdf5bf9962f3d5699dc

Is there any reason not to include this fix as a patch? 

Richard

[1] http://fedoraproject.org/wiki/Packaging:Python#Building_more_than_once

(In reply to comment #17)
> > > 2. Do we need python2 if we're building with python3?
> > > 
> > > If not change your BuildRequires to:
> > > 
> > > %if 0%{?with_python3}
> > > BuildRequires:  python3-devel
> > > %else
> > > BuildRequires:  python2-devel
> > > %endif # if with_python3
> > 
> > I believe the original way is the correct way. At least every package I've seen
> > does not conditionalize the BR: python2-devel and it is this way in the
> > guidelines as well
> > 
> > http://fedoraproject.org/wiki/Packaging:Python
> 
> I think the section of the python guidelines you're referring to are if you're
> building for *BOTH* python2 and python3[1]. I believe in your case it's an
> either/or, so it shuold be conditionalized.
> 
> Let me know if that is not the case.
> 

Makes sense. Done

> 
> > BTW also fixed the DEFAULT_SNAPFILE bug (this will be shipped in the first
> > release after this RPM gets into Fedora, though you can manually apply the fix
> > for now if you want)
> > 
> > https://github.com/movitto/snap/commit/7a53119b1d5bd048a4ddfbdf5bf9962f3d5699dc
> 
> Is there any reason not to include this fix as a patch? 

Included the patch in the release / new rpm

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

Ok, last question I hope until I do the full guideline checks...

How did you include the fix? I was looking for a "Patch0: ..." and "%patch0 ..." in the spec file but it's not there.

Also, I noticed you're using github. I'm not a git expert but I've noticed other projects use the tags for released versions, so in your case I would expect to see a 0.5.0 release tagged, then you would generate a patch. For instance, if you created a tag "snap-0.5.0" you could use something like:

git diff -p --stat snap-0.5.0 7a53119b1d5bd048a4ddfbdf5bf9962f3d5699dc > snap-snapfile.patch

Of course, do it first without the ">" so you can make sure it's what you meant.

If you've already included the fix, maybe just bump the patch rev and call it 0.5.1.

Then you would reset your "Release:" tag back to 1 (but keep all the changelog history)

Richard

Agreed, will tag the release once all the changes to make the package fedora compliant are said and done. Would like to hold off on tagging the release until then though (yes a little backwards than normally done, but it keeps things cleaner / simpler). 

In any case shouldn't be a blocker for the review.

Yup, not a blocker, just needed to know what your plan was.

Full review to follow.

Richard

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

MUST:
[+] rpmlint output: Good
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: GPLv3
[+] license field matches the actual license: Actual source is GPLv3.
[+] license file is included in %doc: LICENSE
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum matches (b11447108c0cc0e2eb1cd249693bcde7)
[+] package builds on at least one primary arch: Tested F15 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
[N] 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
[N] 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
[+] 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:
[+] sane scriptlets
[+] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[+] package contains man pages for binaries/scripts

*** APPROVED ***

Great! Thanks alot for the review

New Package SCM Request
=======================
Package Name: snap
Short Description: A modular cross-platform system backup/restoration utility
Owners: mmorsi
Branches: f15 f16 el6
InitialCC:

Summary name is Snap, SCM request name is snap, they should match.

My apologies. It is acceptable to edit the summary correct? (which I have done). I would prefer the lower case version. Thank you.

New Package SCM Request
=======================
Package Name: snap
Short Description: A modular cross-platform system backup/restoration utility
Owners: mmorsi
Branches: f15 f16 el6
InitialCC:

Git done (by process-git-requests).

snap-0.5-7.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/snap-0.5-7.fc16

snap-0.5-7.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/snap-0.5-7.fc15

snap-0.5-7.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/snap-0.5-7.el6

snap-0.5-7.el6 has been pushed to the Fedora EPEL 6 testing repository.

snap-0.5-7.fc16 has been pushed to the Fedora 16 stable repository.

snap-0.5-7.fc15 has been pushed to the Fedora 15 stable repository.

snap-0.6-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/snap-0.6-1.fc15

snap-0.6-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/snap-0.6-1.fc16

snap-0.6-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/snap-0.6-1.el6

snap-0.6-1.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

snap-0.6-1.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.

snap-0.6-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.