Just a few comments for now:

- no parallel make
  https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
- does not build for me, because of a rpath issue.
  https://fedoraproject.org/wiki/Packaging/Guidelines#Beware_of_Rpath

  Works with:
  %configure
  sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
  sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

- Why do you use ./configure and not %configure?
  * /usr hardcoded -> use %{_prefix}
  * no $RPM_OPT_FLAGS (would also be in %configure)

- Why are the shared libs in -libs and no -devel package?

- When starting, there was a crash...
  (Couldn't get a backtrace, ABRT seems to hang a bit.)

- Desktop file?

Thanks, I made the configure, rpath and make changes.

I'm really not sure *what* to do with the libs, they're a run-time dependancy of the binaries and I'm not sure that anyone else will us them.  Does it make sense to have foo and foo-devel when they foo requires foo-devel?

Odd, how did you get it to crash?  The only trouble I have is if something else is creating a sound at that time and it can't open the sound device.  Otherwise I can startBristol -mini, for example, and play my little heart out. :)

I'm torn on the .desktop file issue.  It'd be great to have one, maybe for -mini, but do startBristol -h and have a look at how many there are.  I'm not doing 20+ .desktop files.

SPEC: http://zanoni.jcomserv.net/fedora/bristol/bristol.spec
SRPM: http://zanoni.jcomserv.net/fedora/bristol/bristol-0.40.7-2.fc12.src.rpm

(In reply to comment #2)
> Thanks, I made the configure, rpath and make changes.
> 
> I'm really not sure *what* to do with the libs, they're a run-time dependancy
> of the binaries and I'm not sure that anyone else will us them.  Does it make
> sense to have foo and foo-devel when they foo requires foo-devel?

If nothing requires the libs, it would be ok for me, to package them into the main package (the versioned *.so.?.?.?). The other *.so and *.so.? should indeed go into a -devel package.

Why do you package static libs?
(Better delete them completely.)

> Odd, how did you get it to crash?  The only trouble I have is if something else
> is creating a sound at that time and it can't open the sound device.  Otherwise
> I can startBristol -mini, for example, and play my little heart out. :)

"Try 'startBristol -b3' or 'startBristol -jack', for example, if you are unsure."
So I started 'startBristol -jack' and it crashes on startup, or on clicking...

'startBristol -mini' works...

> 
> I'm torn on the .desktop file issue.  It'd be great to have one, maybe for
> -mini, but do startBristol -h and have a look at how many there are.  I'm not
> doing 20+ .desktop files.

I think, you should pick one as exapmle and start this with the desktop file.
e.g. -b3 (the upstream default one)

Ok, libs subpackage created.  I had initially removed the static libs, but I forgot to carry over that flag when i moved to using %configure.  Now it's not working, so I'm ripping them out manually.

-b3 works for me.

I created a .desktop file.

SPEC: http://zanoni.jcomserv.net/fedora/bristol/bristol.spec
SRPM: http://zanoni.jcomserv.net/fedora/bristol/bristol-0.40.7-3.fc12.src.rpm

%files
%{_libdir}/lib*.so

%files devel
%{_libdir}/lib*.so.*

This should be in the opposite way ;)

- There is an icon in the sources (bitmaps/bicon.svg)
  No need to ship an own one.

- I remember, the encoding line should be deleted in fedora, but not for rhel.
  But can't find the guidelines or something else for this...

- rpmlint:
  * some zero-length errors
  * standard-dir-owned-by-package /usr/share/icons

Whoops.  Fixed.

I couldn't get that icon to display.  If you can, I'll use it, but the PNG does for me.  

desktop-file-validate warns that it's deprecated.  I've removed it.  If I ever build for EPEL I can re-add it.

Ownership fixed.  Leaving the zero-length files for now.

SPEC: http://zanoni.jcomserv.net/fedora/bristol/bristol.spec
SRPM: http://zanoni.jcomserv.net/fedora/bristol/bristol-0.40.7-4.fc12.src.rpm

Review:

Good:
- name ok
- libs correctly packed
- no static libs
- %files ok
- permissions ok
- BR ok
- no *.la
- %install ok
- ldconfig ok
- %clean there
- desktop file ok
- license ok


Needswork:
- %doc: INSTALL is not needed as doc (just for you as maintainer, not for enduser)
- Please check, if the zero-length files are needed for runtime.
  If not delete them.
- The svg icon works great with:
  install -p -m 0644 bitmaps/bicon.svg $RPM_BUILD_ROOT%{_datadir}/pixmaps/bristol.svg

  Then no gtk-update-icon-cache is needed.

- You could query upstream to remove the excetuable bits, so %prep would look more pretty :(

Forgot to add:

Please use make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p' to preserve timestamps.

The icon cache scriptlets are outdated, please use the latest version from 
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

(In reply to comment #9)
> The icon cache scriptlets are outdated, please use the latest version from 
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache    

If he uses: install -p -m 0644 bitmaps/bicon.svg
$RPM_BUILD_ROOT%{_datadir}/pixmaps/bristol.svg

this won't be needed anyway.
(This suggestion is from:
https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files )

WRT the INSTALL, I see no harm in including it.  Also, this way, if something breaks, a user might see something in it that I missed and contribute to the fix.

WRT zero-length files, they may not always be zero-length, and I see no harm in including them.  

I switched to the SVG icon, not sure what I did wrong before, but there you go.

SPEC: http://zanoni.jcomserv.net/fedora/bristol/bristol.spec
SRPM: http://zanoni.jcomserv.net/fedora/bristol/bristol-0.40.7-5.fc12.src.rpm

(In reply to comment #11)
> WRT the INSTALL, I see no harm in including it.

"Any relevant documentation included in the source distribution should be included in the package. Irrelevant documentation include build instructions, the omnipresent INSTALL file containing generic build instructions, for example, and documentation for non-Linux systems, e.g. README.MSDOS."

Source: https://fedoraproject.org/wiki/Packaging/Guidelines#Documentation

Whoops, I mixed this up with something else.  I thought this INSTALL was actually useful.  It'll be removed in the next iteration.

(In reply to comment #11)
> WRT the INSTALL.

Can be removed before the import.
 
> WRT zero-length files, they may not always be zero-length, and I see no harm in
> including them.  

That's why I said 'check, if the zero-length files are needed for runtime' ;)

They are plainly 'seeable' and are not hidden files, so I consider this as a SHOULD, but not as a blocker.

###########################################

With the removed INSTALL, this is approved.

###########################################


APPROVED

Excellent, thanks for the review!

New Package CVS Request
=======================
Package Name: bristol
Short Description: Synthesizer emulator
Owners: limb
Branches: F-12 F-11
InitialCC:

CVS done (by process-cvs-requests.py).

Imported and built in rawhide.  Thanks all!

bristol-0.40.7-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/bristol-0.40.7-6.fc11

bristol-0.40.7-6.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/bristol-0.40.7-6.fc12

bristol-0.40.7-6.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

bristol-0.40.7-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.