Bug 391091

Why is this assigned to Jarod?  Is he reviewing?  If so, please set
fedora-review to '?'.

Yep, I was talking to James on irc, told him to go ahead and assign it over to me and I'd work with him on 
it, just hadn't got around to prodding the bug yet.

James, apologies for the delay, got caught up in that firewire stuff again...
I'll try to give this a good beating tonight and/or tomorrow. :)

Okay, first pass through the spec file:

1) numerous extraneous trailing spaces that should be deleted (shows up bright
red w/my vim setup).

2) The awk thingies for Version and Release... Ew. Not particularly happy for a
fedora package, though I dunno if it strictly violates guidelines or not.

3) spacing from one %section to another is a bit inconsistent (not a mustfix,
but something that always bugs me).

4) the %description for server should be wrapped at 80 characters (rpmlint will
complain about this)

5) use of /var instead of %{_localstatedir}

6) use of /etc instead of %{_sysconfdir}

7) changelog versioning doesn't match rpm versionsing (i.e., 0.9-1 is not the
same as 0.9-0.3git)


Now for a glance at rpmlint output...

1) non-executable-script errors on a bunch of .py files (all 0644)

2) the changelog thing mentioned above

3) the %description server line length thing

4) warning about the snake-server initscript enabling it by default. The
guidelines generally don't permit services to be enabled by default, it should
be left to the user to enable the service.

5) the 0660 perms on the files that comprise the srpm should generally be 0644.


Either remedy the above issues or provide justification why you don't need to,
and we'll rinse and repeat 'til we're both happy. :)

(In reply to comment #4)

> 1) numerous extraneous trailing spaces that should be deleted (shows up bright
> red w/my vim setup).

Great catch, thanks ... I've added the same .vimrc checks and fixed this
 
> 2) The awk thingies for Version and Release... Ew. Not particularly happy for
a fedora package, though I dunno if it strictly violates guidelines or not.

This was taken directly from another fedora project.  It was new for me as well.
   Changed things around a bit to use the spec file as the definitive source for
Version and Release.

> 3) spacing from one %section to another is a bit inconsistent (not a mustfix,
> but something that always bugs me).

Good catch, cleaned up and should be consistent now.

> 4) the %description for server should be wrapped at 80 characters (rpmlint
will complain about this)

Fixed

> 5) use of /var instead of %{_localstatedir}
> 6) use of /etc instead of %{_sysconfdir}

Fixed

> 7) changelog versioning doesn't match rpm versionsing (i.e., 0.9-1 is not the
> same as 0.9-0.3git)

Gotcha, okay this should line up now.

> Now for a glance at rpmlint output...

> 1) non-executable-script errors on a bunch of .py files (all 0644)
> 
> 2) the changelog thing mentioned above

should be fixed now ... reran rpmlist and it's happy
 
> 3) the %description server line length thing

same ... rpmlint seems happy now
 
> 4) warning about the snake-server initscript enabling it by default. The
> guidelines generally don't permit services to be enabled by default, it should
> be left to the user to enable the service.

Oh good catch, fixed
 
> 5) the 0660 perms on the files that comprise the srpm should generally be 0644.
 
Okay, that should be clean now

> Either remedy the above issues or provide justification why you don't need to,
> and we'll rinse and repeat 'til we're both happy. :)

The only rpmlint output that remains is the following.  Any thoughts as to
whether this needs to be addressed?

$ rpmlint snake-0.9-0.4git.fc8.src.rpm snake-0.9-0.4git.fc8.noarch.rpm
snake-server-0.9-0.4git.fc8.noarch.rpm 
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/machineinfo.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/zeroconf.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/tree.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/client.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/dbushelper.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/uri.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/install.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/saverestore.py 0644
snake.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/xmlhelper.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/labquery.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/config.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/treedb.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/plugins.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/labindex.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/log.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/server.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/dbushelper.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/kickstart.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/ksdb.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/machine.py 0644
snake-server.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/snake/compose.py 0644

Updated spec and source available at the following URLs:

Spec URL: http://jlaska.fedorapeople.org/snake/snake.spec
SRPM URL: http://jlaska.fedorapeople.org/snake/snake-0.9-0.4git.fc8.src.rpm

Running rpmlint -i highlighted the cause of the previous failure.  Updated
packages and resposted.  rpmlint now runs clean.

Spec URL: http://jlaska.fedorapeople.org/snake/snake.spec
SRPM URL: http://jlaska.fedorapeople.org/snake/snake-0.9-0.4git.fc8.src.rpm

Latest pass through the spec:

1) the lines between %prep and %setup probably shouldn't be there, %setup is a
macro used in the %prep section, not a new section. :)

2) I still see 6 /var and 2 /etc used instead of %{_localstatedir} and
%{_sysconfdir}, respectively.

Outside of that, yeah, rpmlint output is completely clean now, spec looks nice
and tidy, resulting packages look sane, etc... Okay, on to more in-depth formal
review...


Fedora Package Review: snake
----------------------------

MUST Items:

* rpmlint is silent

* meets Package Naming Guidelines

* spec file name matches %{name}, in the format %{name}.spec

* package meets the Packaging Guidelines

* open-source compatible license and meets fedora legal reqs

* License field in spec matches actual license
**** NOTE: ****
Interesting tidbit here... You have both a COPYING and a LICENSE file, which are
*almost*, but *not quite* the same file... You really only need one or the
other, and it looks like the main difference between them are the address listed
for the FSF and Lesser vs. Library.

* source includes text of license(s) in its own file, file in %doc

* spec file legible and in American English

* sources used match the upstream source, as provided in spec URL. Verify with
md5sum (if no upstream URL, source creation method must be documented and can be
verified using diff).
**** FIXME ****
Oops. I missed this on the initial passes through the spec... Granted, its a git
snap, but in that case, we need a bit of documentation on how the snap was
produced. And once a 'stable' release is out, there ought to be tarballs
available that you point to. Maybe check out how Jesse does things for Pungi.

* produces binary rpms on x86_64/F8 just fine

* No ExcludeArch used

* BuildRequires are sane (only needs python-devel)

* no locales

* no shared libs

* not relocatable

* package owns all directories it creates:
**** FIXME ****
Just noticed that it doesn't look like the resulting binaries own
/var/lib/snake. Just need to add a '%dir %{_localstatedir}/lib/snake' above the
lines in '%files server' that own the sub-directories of that directory.

* no duplicates in %files

* permissions on %files sane

* %clean includes rm -rf $RPM_BUILD_ROOT

* macros used consistently
**** FIXME ****
Just need to get the /var and /etc replacements in there.

* package contains code

* no need for a -doc subpackage

* files in %doc aren't required for package to work

* no header files

* no static libs

* no pkgconfig

* no libs

* no -devel

* no libtool archives

* not a GUI app

* doesn't own files or folders other package own

* %install starts with rm -rf $RPM_BUILD_ROOT

* filenames in packages are valid UTF-8



SHOULD Items (not absolutely mandatory, but highly encouraged)

* license text(s) included

* description and summary sections in spec should contain translations for
supported Non-English languages, if available: not available

* package should build in mock: builds fine in x86_64 devel mock chroot.

* package should build on all supported architectures: noarch package, no reason
to think it won't build everywhere.

* package should function as expected: untested by me personally, but known to
be functional.

* any scriptlets must be sane: server start/stop/add scriptlets are minimal and
perfectly sane.

* subpackages other than -devel require the base package using a fully versioned
dependency: snake-server does so.

* pkgconfig files go in -devel pkg, unless package is a devel tool itself: no
pkgconfig stuff to worry about.

* If package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or
/usr/sbin consider requiring the package which provides the file instead of the
file itself: n/a.


So only a few little things to fix up before I think its ready to approve.

(In reply to comment #7)
> Latest pass through the spec:
> 
> 1) the lines between %prep and %setup probably shouldn't be there, %setup is a
> macro used in the %prep section, not a new section. :)

Thanks, fixed

> 2) I still see 6 /var and 2 /etc used instead of %{_localstatedir} and
> %{_sysconfdir}, respectively.

My apologies ... I misinterpreted and thought the request was to remove usage of
those macros.  Okay, I've made appropriate changes.

> * License field in spec matches actual license
> **** NOTE: ****
> Interesting tidbit here... You have both a COPYING and a LICENSE file, which are
> *almost*, but *not quite* the same file... You really only need one or the
> other, and it looks like the main difference between them are the address listed
> for the FSF and Lesser vs. Library.

Removed COPYING in favor of LICENSE file

> * sources used match the upstream source, as provided in spec URL. Verify with
> md5sum (if no upstream URL, source creation method must be documented and can be
> verified using diff).
> **** FIXME ****
> Oops. I missed this on the initial passes through the spec... Granted, its a git
> snap, but in that case, we need a bit of documentation on how the snap was
> produced. And once a 'stable' release is out, there ought to be tarballs
> available that you point to. Maybe check out how Jesse does things for Pungi.

Okay, I've updated the spec file to use a url to point to the releases. 
Additionally, I have created a releases page on the hosted.fedoraproject.org
snake page that includes the spec, tarball, and src.rpm package
(https://hosted.fedoraproject.org/projects/snake/wiki/SnakeReleases).  Hopefully
this addresses it.

> * package owns all directories it creates:
> **** FIXME ****
> Just noticed that it doesn't look like the resulting binaries own
> /var/lib/snake. Just need to add a '%dir %{_localstatedir}/lib/snake' above the
> lines in '%files server' that own the sub-directories of that directory.

Doh!  Good catch, changes commited

> * macros used consistently
> **** FIXME ****
> Just need to get the /var and /etc replacements in there.

This should be addressed now

Updated spec and src.rpm available ...

Spec URL: http://jlaska.fedorapeople.org/snake/snake.spec
SRPM URL: http://jlaska.fedorapeople.org/snake/snake-0.9-0.4git.fc8.src.rpm

Just checked over the fixes, looks good to me. Package APPROVED and I'd be happy
to sponsor you.

New Package CVS Request
=======================
Package Name: snake
Short Description: SNAKE is a client/server python framework used to support
anaconda installations
Owners: jlaska,wwoods
Branches: FC-6 F-7 F-8 EL-4 EL-5
InitialCC: jlaska
Cvsextras Commits: no

cvs done except for the FC-6 branch. No FC-6 branches are allowed since the F-8
release. 

Bits are built and in the repos, closing.