Bug 190029 - Review Request: whysynth-dssi - DSSI software synthesizer plugin
Review Request: whysynth-dssi - DSSI software synthesizer plugin
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Till Maas
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-26 15:59 EDT by Anthony Green
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-27 12:01:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
opensource: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Anthony Green 2006-04-26 15:59:05 EDT
Spec URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi-20060122-1.src.rpm
Description: 
WhySynth is a versatile softsynth which operates as a plugin for the
Disposable Soft Synth Interface (DSSI).
Comment 1 Anthony Green 2006-05-18 22:21:39 EDT
Updated bits here:

Spec URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi-20060122-2.src.rpm

These updates were based on the hexter-dssi package feedback.
Comment 2 Parag AN(पराग) 2006-06-01 09:45:27 EDT
Remove Twice written %changelog in SPEC file
Comment 3 Anthony Green 2006-06-01 11:15:49 EDT
(In reply to comment #2)
> Remove Twice written %changelog in SPEC file

Oops.. Fixed:

Spec URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi-20060122-3.src.rpm

Thanks!
Comment 4 Till Maas 2006-08-23 08:55:29 EDT
Here are some comments from me:

rpmlint shows some errors:

W: whysynth-dssi incoherent-version-in-changelog 0.1.3-3 20060122-3.fc5

last changelog entry mentions version 0.1.3-3 but the package is 20060122-3, 
the other entries may have the wrong version, too.

W: whysynth-dssi dangling-symlink /usr/bin/whysynth /usr/bin/jack-dssi-host

Don't know, what this means.

W: whysynth-dssi 
symlink-should-be-relative /usr/bin/whysynth /usr/bin/jack-dssi-host

Make sure, the symlink is relative.

W: whysynth-dssi macro-in-%changelog __rm
W: whysynth-dssi macro-in-%changelog makeinstall

Macros mentioned in %changelog must be escaped with an %, e.g. %%{__rm} 
instead of %{__rm}



Package builds in mock, but there are some warnigs in the build log:

whysynth_voice.c: In function 'y_voice_note_on':
whysynth_voice.c:79: warning: inlining failed in call to 'y_eg_setup': --param 
large-function-growth limit reached
whysynth_voice.c:214: warning: called from here
whysynth_voice.c:79: warning: inlining failed in call to 'y_eg_setup': --param 
large-function-growth limit reached
whysynth_voice.c:214: warning: called from here
whysynth_voice.c:79: warning: inlining failed in call to 'y_eg_setup': --param 
large-function-growth limit reached
whysynth_voice.c:214: warning: called from here
whysynth_voice.c:79: warning: inlining failed in call to 'y_eg_setup': --param 
large-function-growth limit reached
whysynth_voice.c:214: warning: called from here
[...]
whysynth_voice_render.c: In function 'y_voice_render':
whysynth_voice_render.c:1298: warning: inlining failed in call 
to 'oscillator': --param max-inline-insns-single limit reached
whysynth_voice_render.c:2171: warning: called from here
whysynth_voice_render.c:1298: warning: inlining failed in call 
to 'oscillator': --param max-inline-insns-single limit reached
whysynth_voice_render.c:2172: warning: called from here
whysynth_voice_render.c:1298: warning: inlining failed in call 
to 'oscillator': --param max-inline-insns-single limit reached
whysynth_voice_render.c:2173: warning: called from here
whysynth_voice_render.c:1298: warning: inlining failed in call 
to 'oscillator': --param max-inline-insns-single limit reached
whysynth_voice_render.c:2174: warning: called from here
whysynth_voice_render.c:283: warning: inlining failed in call 
to 'y_voice_update_lfo': --param large-function-growth limit reached
whysynth_voice_render.c:2352: warning: called from here
whysynth_voice_render.c:283: warning: inlining failed in call 
to 'y_voice_update_lfo': --param large-function-growth limit reached
whysynth_voice_render.c:2353: warning: called from here
whysynth_voice_render.c:283: warning: inlining failed in call 
to 'y_voice_update_lfo': --param large-function-growth limit reached
whysynth_voice_render.c:2354: warning: called from here
whysynth_voice_render.c:283: warning: inlining failed in call 
to 'y_voice_update_lfo': --param large-function-growth limit reached
whysynth_voice_render.c:2355: warning: called from here
whysynth_voice_render.c:283: warning: inlining failed in call 
to 'y_voice_update_lfo': --param large-function-growth limit reached
whysynth_voice_render.c:2356: warning: called from here

Comment 5 Till Maas 2006-08-23 09:40:28 EDT
You use
%defattr(-,root,root)
instead of
%defattr(-,root,root,-)
- is this intentionally?

Why is there no desktop file as in 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=191590 ? If you add it, 
please not inline but in an extra file.


Why do you use '%makeinstall' instead of the favoured 'make install 
DESTDIR="$RPM_BUILD_ROOT"'?
Comment 6 Anthony Green 2006-09-04 18:28:52 EDT
(In reply to comment #4)
> Here are some comments from me:
> 
> rpmlint shows some errors:
> 
> W: whysynth-dssi incoherent-version-in-changelog 0.1.3-3 20060122-3.fc5
> 
> last changelog entry mentions version 0.1.3-3 but the package is 20060122-3, 
> the other entries may have the wrong version, too.

Fixed

> W: whysynth-dssi dangling-symlink /usr/bin/whysynth /usr/bin/jack-dssi-host
> 
> Don't know, what this means.

This is OK.  It's a symlink to something that isn't in this RPM.
 
> W: whysynth-dssi 
> symlink-should-be-relative /usr/bin/whysynth /usr/bin/jack-dssi-host
> 
> Make sure, the symlink is relative.

Fixed.

> W: whysynth-dssi macro-in-%changelog __rm
> W: whysynth-dssi macro-in-%changelog makeinstall
> 
> Macros mentioned in %changelog must be escaped with an %, e.g. %%{__rm} 
> instead of %{__rm}

Fixed.
 
> Package builds in mock, but there are some warnigs in the build log:

I'm ignoring these for now.  Tweaking compile options may possibly result in
better performance.

Spec URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi-20060122-3.src.rpm


Comment 7 Anthony Green 2006-09-04 18:30:41 EDT
(In reply to comment #5)
> You use
> %defattr(-,root,root)
> instead of
> %defattr(-,root,root,-)
> - is this intentionally?

No - fixed.

> Why is there no desktop file as in 
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=191590 ? If you add it, 
> please not inline but in an extra file.

There's no icon.  I'll create one.  It's still not in the SPEC file I've posted.
 
> 
> Why do you use '%makeinstall' instead of the favoured 'make install 
> DESTDIR="$RPM_BUILD_ROOT"'?

Fixed.

Spec URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi-20060122-4.src.rpm

(which is what I meant to write in my previous reply)

Comment 8 Anthony Green 2006-09-04 18:51:05 EDT
This version has a .desktop file and icons.

Spec URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi-20060122-5.src.rpm

Thanks
Comment 9 Till Maas 2006-09-12 16:25:52 EDT
You should not convert the icon in your spec file. The different sizes are 
only needed if they are manually edited in the other sizes to look better. 
This simple resizing is done automatically by a window manager if needed.
Comment 10 Anthony Green 2006-09-16 17:00:22 EDT
(In reply to comment #9)
> You should not convert the icon in your spec file. 

Ok, fixed:

Spec URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/whysynth-dssi-20060122-6.src.rpm
Comment 11 Till Maas 2006-10-17 07:03:14 EDT
The categories can be added directly to the .desktop file:

  --add-category AudioVideo                       \
  --add-category Application                      \

These Requires should not be used
(http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28update-desktop-database%29#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda)
Requires(post):   desktop-file-utils
Requires(postun): desktop-file-utils

Here you make a symlink (cd $RPM_BUILD_ROOT%{_bindir}; ln -s jack-dssi-host
whysynth) but in the fluidsynth-dssi package you did not. I do not really know,
whether or not it is needed but from a user's point of view I think it is better
to be consistent here and either add these symlink or not for dssi plugins.
Comment 12 Anthony Green 2006-10-21 14:26:33 EDT
(In reply to comment #11)
> The categories can be added directly to the .desktop file:
> 
>   --add-category AudioVideo                       \
>   --add-category Application                      \
> 
> These Requires should not be used
>
(http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28update-desktop-database%29#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda)
> Requires(post):   desktop-file-utils
> Requires(postun): desktop-file-utils

Fixed and fixed.

> Here you make a symlink (cd $RPM_BUILD_ROOT%{_bindir}; ln -s jack-dssi-host
> whysynth) but in the fluidsynth-dssi package you did not. I do not really know,
> whether or not it is needed but from a user's point of view I think it is better
> to be consistent here and either add these symlink or not for dssi plugins.

I didn't do the same for fluidsynth because qsynth in Extras already serves 
same purpose as a fluidsynth jack-dssi-host link.  There is no
equivalent for whysynth.

New bits are here.  Thanks!

Spec URL: http://people.redhat.com/green/FE/devel/whysynth-dssi.spec
SRPM URL: http://people.redhat.com/green/FE/devel/whysynth-dssi-20060122-7.src.rpm


Comment 14 Florin Andrei 2007-01-27 00:47:45 EST
FWIW, the src.rpm rebuilds just fine on FC6 and the binary works pretty well (if
/etc/hosts is correct)
Comment 15 Till Maas 2007-03-25 17:01:54 EDT
rpmlint: 
W: whysynth-dssi dangling-relative-symlink /usr/bin/whysynth jack-dssi-host (can
be ignored)
E: whysynth-dssi non-standard-dir-perm /usr/share/doc/whysynth-dssi-20060122/doc
02755 (this should be fixed)

naming: ok
packaging guidelines:

.desktop file:
Categories=Application;AudioVideo;X-Synthesis;X-MIDI;X-Jack;
Use Midi instead of X-MIDI and maybe add Audio to the categories.
Also consider adding a GenericName entry to the .desktop file.

See http://standards.freedesktop.org/menu-spec/latest/apa.html

license: ok
sources: ok

76e99ef8585345b03424b4770f896c0f  whysynth-20060122.tar.bz2
76e99ef8585345b03424b4770f896c0f  whysynth-20060122.tar.bz2.1
files and directories: see rpmlint
mock build: ok

Why do you install the icon twice? I think the icon below pixmaps can / should
be skipped.

btw. a nice alternative for this "(cd $RPM_BUILD_ROOT%{_bindir}; ln -s
jack-dssi-host whysynth)" is:

pushd $RPM_BUILD_ROOT%{_bindir}
ln -s jack-dssi-host whysynth
popd

This makes it more clear, what you want to do here.
Comment 16 Anthony Green 2007-03-26 12:48:56 EDT
(In reply to comment #15)
> rpmlint: 
> W: whysynth-dssi dangling-relative-symlink /usr/bin/whysynth jack-dssi-host (can
> be ignored)
> E: whysynth-dssi non-standard-dir-perm /usr/share/doc/whysynth-dssi-20060122/doc
> 02755 (this should be fixed)

I don't see this now.

> .desktop file:
> Categories=Application;AudioVideo;X-Synthesis;X-MIDI;X-Jack;
> Use Midi instead of X-MIDI and maybe add Audio to the categories.

Done.

> Also consider adding a GenericName entry to the .desktop file.

I don't think this makes sense.  Maybe if it was a GM synth, but it isn't.
 
> See http://standards.freedesktop.org/menu-spec/latest/apa.html
> 
> license: ok
> sources: ok
> 
> 76e99ef8585345b03424b4770f896c0f  whysynth-20060122.tar.bz2
> 76e99ef8585345b03424b4770f896c0f  whysynth-20060122.tar.bz2.1
> files and directories: see rpmlint
> mock build: ok
> 
> Why do you install the icon twice? I think the icon below pixmaps can / should
> be skipped.

Ok.

> btw. a nice alternative for this "(cd $RPM_BUILD_ROOT%{_bindir}; ln -s
> jack-dssi-host whysynth)" is:
> 
> pushd $RPM_BUILD_ROOT%{_bindir}
> ln -s jack-dssi-host whysynth
> popd
> 
> This makes it more clear, what you want to do here.

Done.

New bits here:
http://people.redhat.com/green/FE/devel/whysynth-dssi-20060122-8.src.rpm
http://people.redhat.com/green/FE/devel/whysynth-dssi.spec
Comment 17 Till Maas 2007-03-26 12:55:43 EDT
(In reply to comment #16)
> (In reply to comment #15)
> > rpmlint: 

> > E: whysynth-dssi non-standard-dir-perm /usr/share/doc/whysynth-dssi-20060122/doc
> > 02755 (this should be fixed)

> I don't see this now.

You see this from rpmlint, after you installed it or with rpm -v -qpl
whysynth-dssi-*rpm (you see the permissions then, not the error, of course).
Seems to be a bug in rpmlint, but I did not have the time to investigate this
further.
Comment 18 Till Maas 2007-03-26 16:43:32 EDT
(In reply to comment #16)
> (In reply to comment #15)
> > E: whysynth-dssi non-standard-dir-perm /usr/share/doc/whysynth-dssi-20060122/doc
> > 02755 (this should be fixed)
> 
> I don't see this now.

Hm, this problem only occurs with a rpmbuilt package, but not with the mock build.

> Why do you install the icon twice? I think the icon below pixmaps can / should
> be skipped.

> Ok.

Eh, please pardon my bad english, I meant keep the icon that is installed in
%{_datadir}/icons/hicolor and do not install it in 
$RPM_BUILD_ROOT%{_datadir}/pixmaps/, so exactly the other way round.

%{_datadir}/icons/hicolor
Comment 19 Anthony Green 2007-03-27 05:38:09 EDT
(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > E: whysynth-dssi non-standard-dir-perm
/usr/share/doc/whysynth-dssi-20060122/doc
> > > 02755 (this should be fixed)
> > 
> > I don't see this now.
> 
> Hm, this problem only occurs with a rpmbuilt package, but not with the mock build.

I can't reproduce this problem. 

> 
> > Why do you install the icon twice? I think the icon below pixmaps can / should
> > be skipped.
> 
> > Ok.
> 
> Eh, please pardon my bad english, I meant keep the icon that is installed in
> %{_datadir}/icons/hicolor and do not install it in 
> $RPM_BUILD_ROOT%{_datadir}/pixmaps/, so exactly the other way round.
> 
> %{_datadir}/icons/hicolor

Ok, fixed.

New bits here:
http://people.redhat.com/green/FE/devel/whysynth-dssi-20060122-9.src.rpm
http://people.redhat.com/green/FE/devel/whysynth-dssi.spec

Is this package OK now?

Thanks!


Comment 20 Till Maas 2007-03-27 09:35:54 EDT
> I can't reproduce this problem. 

Ah, it is a "problem" of my rpmbuild setup, because the ~rpmbuilder/BUILD
directory has the set guid flag and this is not removed by the %doc. So it is
not a problem of the package-

> New bits here:
> http://people.redhat.com/green/FE/devel/whysynth-dssi-20060122-9.src.rpm
> http://people.redhat.com/green/FE/devel/whysynth-dssi.spec

The spec is release 7 and the src.rpm produces a 404, so I cannot tell.
Comment 21 Anthony Green 2007-03-27 09:46:50 EDT
(In reply to comment #20)
> The spec is release 7 and the src.rpm produces a 404, so I cannot tell.

Weird.  Please try again.  I just checked and they work.
Comment 22 Till Maas 2007-03-27 09:57:40 EDT
(In reply to comment #21)
> (In reply to comment #20)
> > The spec is release 7 and the src.rpm produces a 404, so I cannot tell.
> 
> Weird.  Please try again.  I just checked and they work.

Interesting, works now.


The spec looks fine now,
APPROVED

In case this is the first new review for you, you have to proceed as described in:

http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
http://fedoraproject.org/wiki/PackageMaintainers/NewPackageProcess
Comment 23 Anthony Green 2007-03-27 10:09:23 EDT
New Package CVS Request
=======================
Package Name: whysynth-dssi
Short Description: DSSI software synthesizer plugin
Owners: green@redhat.com
Branches: FC-6 devel
InitialCC: 

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