Bug 557169 - Review Request: bristol - Synthesizer emulator
Summary: Review Request: bristol - Synthesizer emulator
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-01-20 16:09 UTC by Gwyn Ciesla
Modified: 2010-01-29 03:30 UTC (History)
4 users (show)

Fixed In Version: 0.40.7-6.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-29 03:24:27 UTC
Type: ---
tomspur: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Gwyn Ciesla 2010-01-20 16:09:44 UTC
Bristol is an emulation package for a number of different 'classic' synthesizers including additive and subtractive and a few organs. The application consists of the engine, which is called bristol, and its own GUI library called brighton that represents all the emulations.

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

Comment 1 Thomas Spura 2010-01-20 22:43:28 UTC
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?

Comment 2 Gwyn Ciesla 2010-01-21 15:10:19 UTC
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

Comment 3 Thomas Spura 2010-01-22 11:10:04 UTC
(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)

Comment 4 Gwyn Ciesla 2010-01-22 16:18:37 UTC
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

Comment 5 Thomas Spura 2010-01-22 18:12:09 UTC
%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

Comment 6 Gwyn Ciesla 2010-01-22 18:49:07 UTC
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

Comment 7 Thomas Spura 2010-01-24 01:49:29 UTC
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 :(

Comment 8 Thomas Spura 2010-01-24 03:12:20 UTC
Forgot to add:

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

Comment 9 Christoph Wickert 2010-01-24 11:53:18 UTC
The icon cache scriptlets are outdated, please use the latest version from 
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

Comment 10 Thomas Spura 2010-01-24 12:04:59 UTC
(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 )

Comment 11 Gwyn Ciesla 2010-01-25 16:48:50 UTC
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

Comment 12 Christoph Wickert 2010-01-25 17:06:42 UTC
(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

Comment 13 Gwyn Ciesla 2010-01-25 17:13:02 UTC
Whoops, I mixed this up with something else.  I thought this INSTALL was actually useful.  It'll be removed in the next iteration.

Comment 14 Thomas Spura 2010-01-25 23:18:09 UTC
(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

Comment 15 Gwyn Ciesla 2010-01-26 13:07:18 UTC
Excellent, thanks for the review!

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

Comment 16 Jason Tibbitts 2010-01-27 05:20:37 UTC
CVS done (by process-cvs-requests.py).

Comment 17 Gwyn Ciesla 2010-01-27 14:25:16 UTC
Imported and built in rawhide.  Thanks all!

Comment 18 Fedora Update System 2010-01-27 14:45:52 UTC
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

Comment 19 Fedora Update System 2010-01-27 14:45:57 UTC
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

Comment 20 Fedora Update System 2010-01-29 03:24:22 UTC
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.

Comment 21 Fedora Update System 2010-01-29 03:30:12 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.