Bug 203217 - Review Request: csound - music synthesis system
Review Request: csound - music synthesis system
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul F. Johnson
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-08-18 22:23 EDT by Dan Williams
Modified: 2010-06-11 00:15 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-05 14:13:05 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)
Mock Build Log (86.52 KB, text/plain)
2006-08-24 12:53 EDT, Brian Pepple
no flags Details
Updated spec file (4.87 KB, text/plain)
2006-08-24 14:35 EDT, Anthony Green
no flags Details
Specfile v3 (5.86 KB, text/plain)
2006-08-24 17:50 EDT, Dan Williams
no flags Details
Update spec file (v3++) (6.71 KB, text/plain)
2006-08-24 19:33 EDT, Anthony Green
no flags Details
spec v4 (6.58 KB, text/plain)
2006-08-24 22:05 EDT, Dan Williams
no flags Details
csound manual sources extracted from cvs repository. (3.07 MB, application/octet-stream)
2006-08-25 02:39 EDT, Anthony Green
no flags Details
spec v4++ (7.26 KB, text/plain)
2006-08-25 02:52 EDT, Anthony Green
no flags Details
specfile v5 (9.00 KB, text/plain)
2006-08-25 10:12 EDT, Dan Williams
no flags Details
spec file v5++ (9.34 KB, text/plain)
2006-08-25 13:30 EDT, Anthony Green
no flags Details
specfile v6 (9.74 KB, text/plain)
2006-08-30 14:13 EDT, Dan Williams
no flags Details
mock build log (12.22 KB, text/plain)
2006-08-31 12:52 EDT, Paul F. Johnson
no flags Details

  None (edit)
Description Dan Williams 2006-08-18 22:23:07 EDT
Spec URL: http://people.redhat.com/dcbw/csound/csound.spec
SRPM URL: http://people.redhat.com/dcbw/csound/csound-5.03.0-1.src.rpm
Description: 
Csound is a sound and music synthesis system, providing facilities for
composition and performance over a wide range of platforms. It is not
restricted to any style of music, having been used for many years in
at least classical, pop, techno, ambient...
Comment 1 Brian Pepple 2006-08-24 12:53:22 EDT
Couple of quick items:

1. Source should be full URL.
2. Package fails to build in Mock.  I'll attach the build log for it.
Comment 2 Brian Pepple 2006-08-24 12:53:56 EDT
Created attachment 134832 [details]
Mock Build Log
Comment 3 Michael Schwendt 2006-08-24 13:09:14 EDT
Looking at the .spec alone reveals several orphaned/unincluded
directories:

%{_libdir}/%{name}/
%{_libdir}/%{name}/plugins/
%{_datadir}/%{name}/

# -devel
%{_includedir}/%{name}/

# -tk
%{_libdir}/%{name}/tcl/
Comment 4 Anthony Green 2006-08-24 14:35:17 EDT
Created attachment 134843 [details]
Updated spec file

We're trying to base our audio packages off othe PlanetCCRMA packages, but
PlanetCCRMA's Csound is a fair bit older than this one.

I'm attaching an updated spec file, but it still isn't perfect.  It adds
support for java, jack, OSC, DSSI and the Csound GUI.  Please include
incorporate these changes.

I think we should also have a -tutorial subpackage, and solve the missing
manual problem.
Comment 5 Dan Williams 2006-08-24 15:05:35 EDT
(In reply to comment #4)
> Created an attachment (id=134843) [edit]
> Updated spec file
> 
> We're trying to base our audio packages off othe PlanetCCRMA packages, but
> PlanetCCRMA's Csound is a fair bit older than this one.
> 
> I'm attaching an updated spec file, but it still isn't perfect.  It adds
> support for java, jack, OSC, DSSI and the Csound GUI.  Please include
> incorporate these changes.
> 
> I think we should also have a -tutorial subpackage, and solve the missing
> manual problem.
> 

I'll incorporate your patch.  BTW, do you know if enabling Jack makes it
required at runtime, or is csound smart enough to not use it?  I don't think
OLPC won't have Jack and we don't want to fork...
Comment 6 Anthony Green 2006-08-24 16:20:16 EDT
(In reply to comment #5)
> I'll incorporate your patch.  BTW, do you know if enabling Jack makes it
> required at runtime, or is csound smart enough to not use it?  I don't think
> OLPC won't have Jack and we don't want to fork...

Try packaging %{_libdir}/csound/plugins/librtjack.so into a subpackage.
The same may work for the libdssi4cs.so and libosc.so if you want to make it
really modular.

Comment 7 Paul F. Johnson 2006-08-24 16:40:03 EDT
I'm a tad concerned that it's redefining printf for some reason in a HELL of a
lot of files. It seems to be coming from h/csoundCore.h line 119

rpmlint warnings.

src.rpm is clean

csound-5.03.0-2 : W: csound no-soname %{_libdir}/lib_csnd.so (should this not be
in the -devel package?

csound-devel : W: csound-devel no-docs (can be ignored)

csound-python: W: dangling-symlink %{_libdir}/python2.4/site-packages/_csnd.so
%{_libdir}/lib_csnd.so
csound-python: W: symlink-should-be-relative
%{_libdir}/python2.4/site-packages/_csnd.so %{_libdir}/lib_csnd.so

csound-java: W: no-soname %{_libdir}/lib_jsound.so
             W: no-docs
             W: symlink-should-be-relative %{_datadir}/java/csnd.jar
%{_libdir}/csound/java/csnd.jar

csound-tk: E: only-non-binary-in-usr-lib
           W: no docs

csound-debuginfo: clean

For the missing .so files, these really should be in the csound-devel package
(and java-devel package). The tk Error must be attended to.

Your specfile is also missing Requires(post and postun): /sbin/ldconfig

Does the package not come with other vocabulary languages? If this is for the
OLPC programme, then it makes sense for translations to be in there!

In %files...

The %{_bindir} entries can be globbed to make things easier to read (or better
still, a simple for loop on the package names.

In %files devel
%{_includedir}/%{name}/* Need to be changed to %{includedir}/%{name}/ to take
ownership of the directory and files

In %files tk

I'm not sure about the .tk files in %{_libdir}. Is this the correct place for them?

csound is currently building in mock - I'll report back when it's done.
Comment 8 Dan Williams 2006-08-24 17:20:24 EDT
(In reply to comment #7)
> I'm a tad concerned that it's redefining printf for some reason in a HELL of a
> lot of files. It seems to be coming from h/csoundCore.h line 119

I asked about that, upstream is now aware that we dislike it :)  It was added to
make sure that all plugins use the Csound logging facilities.  I think it may go
away in the near future, and we can likely remove it if we wish.

> 
> rpmlint warnings.
> 
> src.rpm is clean
> 
> csound-5.03.0-2 : W: csound no-soname %{_libdir}/lib_csnd.so (should this not be
> in the -devel package?

No, it's the python module code, and it should be in
/usr/lib/python2.4/site-packages/.  I'll move it.

> csound-devel : W: csound-devel no-docs (can be ignored)
> 
> csound-python: W: dangling-symlink %{_libdir}/python2.4/site-packages/_csnd.so
> %{_libdir}/lib_csnd.so
> csound-python: W: symlink-should-be-relative
> %{_libdir}/python2.4/site-packages/_csnd.so %{_libdir}/lib_csnd.so

Will be fixed when that lib moves to python's site-packages.
 
> csound-java: W: no-soname %{_libdir}/lib_jsound.so
>              W: no-docs
>              W: symlink-should-be-relative %{_datadir}/java/csnd.jar
> %{_libdir}/csound/java/csnd.jar

This one shouldn't be a symlink either, should probably move csnd.jar to
%{_datadir} too.

> csound-tk: E: only-non-binary-in-usr-lib
>            W: no docs
> 
> csound-debuginfo: clean
> 
> For the missing .so files, these really should be in the csound-devel package
> (and java-devel package). The tk Error must be attended to.

Except that most aren't devel libs; they are the java and python module
libraries and should live in those packages, not in the main csound packages.

> 
> Your specfile is also missing Requires(post and postun): /sbin/ldconfig

Will add.

> Does the package not come with other vocabulary languages? If this is for the
> OLPC programme, then it makes sense for translations to be in there!

I don't think so...  I couldn't see a switch for it at build-time, but I'll ask.

> In %files...
> 
> The %{_bindir} entries can be globbed to make things easier to read (or better
> still, a simple for loop on the package names.

Can that be done even if some stuff from %{_bindir} is owned by a sub-package?

> In %files devel
> %{_includedir}/%{name}/* Need to be changed to %{includedir}/%{name}/ to take
> ownership of the directory and files

Will do.

> In %files tk
> 
> I'm not sure about the .tk files in %{_libdir}. Is this the correct place for
them?

Yeah, that's odd.  I'm not sure.

> csound is currently building in mock - I'll report back when it's done.

Comment 9 Paul F. Johnson 2006-08-24 17:21:24 EDT
mock fails - you need to add BR libpng-devel (failed building the GUI)
Comment 10 Paul F. Johnson 2006-08-24 17:23:53 EDT
8--->
> In %files...
> 
> The %{_bindir} entries can be globbed to make things easier to read (or better
> still, a simple for loop on the package names.

Can that be done even if some stuff from %{_bindir} is owned by a sub-package?
<---8

I know I've done it in other packages - but only where the main package has
owned the root directory. Remember, you can always use %dir
Comment 11 Anthony Green 2006-08-24 17:41:52 EDT
(In reply to comment #7)
> csound-java: W: no-soname %{_libdir}/lib_jsound.so

I think this is ignoreable, as lib_jcsound.so is only ever ldopened (btw, what
happened to the 'c' in lib_jcsound.so? cut-n-paste error?)
Comment 12 Anthony Green 2006-08-24 17:46:21 EDT
(In reply to comment #8)
> (In reply to comment #7)
> >              W: symlink-should-be-relative %{_datadir}/java/csnd.jar
> > %{_libdir}/csound/java/csnd.jar
> 
> This one shouldn't be a symlink either, should probably move csnd.jar to
> %{_datadir} too.

I think we should keep this as a symlink (although I suppose it can be
relative).  Upstream wants it in %{_libdir}/csound/java, but our fedora standard
is to place .jar files in %{_javadir} (== %{_datadir}/java).  We should leave it
in both places to satisfy both our requirement and any upstream packages that
expect to find it in csound's %{_libdir}/csound/java directory.

Comment 13 Dan Williams 2006-08-24 17:50:05 EDT
Created attachment 134864 [details]
Specfile v3

Splits out the jack, dssi, and osc bits.  We want this _really_ modular so OLPC
can use it.  Also incorporates cleanup suggestions from above.
Comment 14 Anthony Green 2006-08-24 19:33:57 EDT
Created attachment 134870 [details]
Update spec file (v3++)

This version adds -javadoc and -tutorial subpackages.
Comment 15 Dan Williams 2006-08-24 22:05:39 EDT
Created attachment 134885 [details]
spec v4

Incorporate v3++ changes and revert change that moved the jarfile.
Comment 16 Anthony Green 2006-08-25 00:35:51 EDT
I just realized that the gcj related stuff in %post and %postun need to move to
%post and %postun for the -java subpackage.  My bad.  Sorry about that!
Comment 17 Anthony Green 2006-08-25 02:39:40 EDT
Created attachment 134889 [details]
csound manual sources extracted from cvs repository.

These manual sources come from the csound cvs repository and correspond to the
5.03 release.  Details are in the spec file I'm about to upload.
Comment 18 Anthony Green 2006-08-25 02:52:07 EDT
Created attachment 134891 [details]
spec v4++

This version creates a -manual subpackage from the csound manual sources I've
uploaded (%{SOURCE1} in the spec file).

csound5gui's Help menu should be fixed to look for HTML from this package
instead of from somewhere in /usr/share.  I can do this tomorrow unless
somebody beats me to it.

Now that I think of it, csound5gui should get an application desktop icon.

Sorry for all of these changes -- I just want this package to be all that it
can be (and not have any feature regressions from the older PlanetCCRMA csound
package).
Comment 19 Dan Williams 2006-08-25 09:57:33 EDT
I've got an specfile that does the doc redirect, and also fixes up Requires.

Turns out I can't glob %{_bindir} files in the main package, because then RPM
thinks that the main package owns %{_bindir} files that it should not.  I'm
talking about:

%files
%{_bindir}/

%files tk
%{_bindir}/brkpt

That makes the main package think it owns brkpt, which means both a perl and a
tk dep in the main package...
Comment 20 Dan Williams 2006-08-25 10:12:45 EDT
Created attachment 134917 [details]
specfile v5

This specfile re-adds the non-globbed bindir and libdir files.	I folded in the
documentation changes and added bits to change the documentation path in
csound5gui to the correct path for the manual package, and also to use
'xdg-open' rather than the hardcoded 'firefox'.
Comment 21 Anthony Green 2006-08-25 13:30:26 EDT
Created attachment 134940 [details]
spec file v5++

This adds a -fluidsynth subpackage.  I didn't realize this was available until
I built on a machine with fluidsynth-devel installed.
Comment 22 Paul F. Johnson 2006-08-25 17:07:44 EDT
Are there any translations available for this package?
Comment 23 Paul F. Johnson 2006-08-26 05:27:55 EDT
Okay, from the latest...

fails to build in mock - you need to add libjpeg-devel to the BRs

rpmlint is mostly clean (I'm ignoring packages without docs). The exceptions are

csound-java : W: no-soname %{_libdir}/lib_jcsound.so
              W: symlink-should-be-relative %{_datadir}/java/csnd.jar
%{_libdir}/%{name}/java.csnd.jar
csound-osc: E: explicit-lib-dependency liblo
csound-manual: E: zero-length
%{_docdir}/%{name}-manual-%{version}/examples/ifthen.csd

Other things...

The Dist: number doesn't get increased with each new release. The problem with
that is that when I'm tracking, it's a bitch to find a difference when you have
to check datestamps instead of something nice like the Dist number. Can you
please increment with each alteration?

When you add a new tarball, can you please respin the src.rpm - it makes life a
damned sight easier for review and testing purposes. I don't have a problem with
minor things like spec files and patches, but whopping huge addins - that's
another thing.

Please change the printf redefinition. Not interested in upstream, it's wrong
and is also a possible security problem (printf is well defined in terms of what
it does and it's problems, the new printf is a different kettle of fish). If it
helps, rename the function cs_printf. It will probably take a minute to do a
glob|regex search and replace on the source.
Comment 24 Dan Williams 2006-08-29 16:21:18 EDT
Is it sufficient to fix the others, but leave the two warnings for csound-java?
Comment 25 Paul F. Johnson 2006-08-30 02:42:07 EDT
Not worried about the symlink, not happy with the no-soname. However, if it's
because the library is a java one masquerading as a real one for some reason,
then I'm okay with it.

Fix the others and so-name and we're good to go. The biggest killer IMO is the
printf redefinition - I'd not allow it through if it's still complaining.
Comment 26 Anthony Green 2006-08-30 03:06:15 EDT
(In reply to comment #25)
> Not worried about the symlink, not happy with the no-soname. However, if it's
> because the library is a java one masquerading as a real one for some reason,
> then I'm okay with it.

It's a JNI library.  Nobody ever links against it - it is dlopen'd by the JVM
running the java code shipped in this package.   Fedora Core/Extras includes
many such libraries with no SONAME.  This should be OK.
Comment 27 Patrice Dumas 2006-08-30 04:44:46 EDT
Shouldn't the JNI library be in %{_libdir}/java/, or 
%{_libdir}/java-ext/?
Comment 28 Anthony Green 2006-08-30 09:50:38 EDT
(In reply to comment #27)
> Shouldn't the JNI library be in %{_libdir}/java/, or 
> %{_libdir}/java-ext/?

What makes you think that?
Comment 29 Paul F. Johnson 2006-08-30 10:17:27 EDT
#26 - fair enough.

Once the other aspects are fixed and new srpm is uploaded (complete with any
sub-package tarballs), I'll review it and hopefully, it'll pass and the world
will carry one, but with a lot of people on the OLPC enjoying the benefits :-)
Comment 30 Patrice Dumas 2006-08-30 10:56:55 EDT
(In reply to comment #28)
> (In reply to comment #27)
> > Shouldn't the JNI library be in %{_libdir}/java/, or 
> > %{_libdir}/java-ext/?
> 
> What makes you think that?

It is a wild guess that dlopened files are searched for by the JVM
in these directories, but I may be wrong, since I don't know java 
packaging at all... Isn't there a directory more suitable that 
%_libdir? perl and python don't drop the similar object files in 
%_libdir, I guessed it was the same with java, but I may be missing 
something...


(as a side note I couldn't rebuild csound, I would have liked to
investigate more, but rpmbuild -ba stops at the very beginning with:

Checking for C header file stdio.h... no
 *** Failed to compile a simple test program. The compiler is
 *** possibly not set up correctly, or is used with invalid flags.
 *** Check config.log to find out more about the error.

trying the gcc command line that appears in config.log by hand, there
is no error???

I am on up to date rawhide.)

Comment 31 Dan Williams 2006-08-30 14:13:48 EDT
Created attachment 135230 [details]
specfile v6

Latest specfile; changes in changelog entry at bottom
Comment 32 Dan Williams 2006-08-30 14:15:22 EDT
All the latest files are also at:

http://people.redhat.com/dcbw/csound/

Comment 33 Paul F. Johnson 2006-08-30 20:53:34 EDT
#30 - I've only seen this once and that was when my system was seriously screwed
up (mixed x86 and x86_64 rpms causing conflicts). Are you on a 64 bit box? If
you are, get rid of the i386 versions and see if that's happier.

#32 - Have you merged the two tarballs [source + manual] together? If so, you
either need to document it or preferably have Source0 for the package and
Source1 for the docs - I can't really verify md5sums if they're merged here and
not upstream. You never did say if there were any translations for the package.

Building the packages now...

rpmlint comes up with the usual no-docs for a couple of packages and two
warnings for the java package (symlink and so-name).

I'll build it in mock and if all goes well, will do the full review on it
tomorrow at some point.

I do need the point above about if you have merged the tarballs together
clarifying though.
Comment 34 Dan Williams 2006-08-31 10:10:57 EDT
No, tarballs haven't been merged.  They are separate Source lines in the
specfile, since they are separate tarballs upstream.  There appear to be no
translations at this time, I'm still trying to get word from upstream about that
though.
Comment 35 Paul F. Johnson 2006-08-31 12:47:30 EDT
failed to build in mock

RPM builde errors:

File not found:
%{buildroot}/%{name}-%{version}.fc6-root-mockbuild/usr/lib/gcj/csound

I'm also concerned with the file attached to the next entry when building the
manual.
Comment 36 Paul F. Johnson 2006-08-31 12:52:12 EDT
Created attachment 135307 [details]
mock build log

This log is repeated for most of the chapters with some having more errors than
others, but in general, they follow the format here.

It shouldn't constitute a problem, but it may make the manual look a bit odd
Comment 37 Anthony Green 2006-08-31 12:56:26 EDT
(In reply to comment #35)
> failed to build in mock
> 
> RPM builde errors:
> 
> File not found:
> %{buildroot}/%{name}-%{version}.fc6-root-mockbuild/usr/lib/gcj/csound

You need to update your java-1.4.2-gcj-compat-devel to the -104 release.

Comment 38 Anthony Green 2006-08-31 13:20:38 EDT
(In reply to comment #36)
> This log is repeated for most of the chapters with some having more errors than
> others, but in general, they follow the format here.
> 
> It shouldn't constitute a problem, but it may make the manual look a bit odd

The manual build process requires that we build it multiple times.   Forward
references are only resolved in the last build.  The errors your pointing out
happen prior to the final build, so they are ignoreable.
Comment 39 Paul F. Johnson 2006-08-31 18:38:00 EDT
Right, it's happy in mock (i386) and I'm not overly concerned about the manual.

Review time:

Good : 

Consistent use of macros throughout the spec file
Clear
US-English
builds cleanly in mock
main package md5 matches upstream (cvs cannot be checked)
Main package has docs, others report no docs (not worried)
No problems with file permissions
No dupes in the rpms
Licences fine
Doesn't have any locale stuff or desktop icon
Compiles fine (x86)
No ownership problems

Needs work
CVS archive doesn't match the guidelines (see
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#NonNumericRelease). It
should reflect the date on which it was grabbed (so it really should be
csound-manual-CVS20060816-disttag
The files section can be globbed

%{_bindir}/c* grabs all starting with c (etc)

makes the spec file a lot simpler to read (IMO)

Fix the CVS datestamp for the manual and I'm happy to let this one go in. I'd be
happy if the file globbing in the spec file was done, but it's not a blocker if
you don't
Comment 40 Anthony Green 2006-08-31 19:45:17 EDT
(In reply to comment #39)
> Needs work
> CVS archive doesn't match the guidelines (see
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#NonNumericRelease). It
> should reflect the date on which it was grabbed (so it really should be
> csound-manual-CVS20060816-disttag

This applies to pre-release snapshots, but not in this case.  In this case we
pulled the sources out using the release tag upstream used in their cvs
repository, as descripted in the spec file.  The only reason we had to pull it
out of cvs is because they only made the resulting PDF/html available, not the
original source.
Comment 41 Paul F. Johnson 2006-09-01 10:11:55 EDT
In that case, I'm happy :-)

APPROVED
Comment 42 Dan Williams 2006-09-05 14:13:05 EDT
imported.  Thanks!
Comment 43 Peter Robinson 2010-06-10 17:09:24 EDT
Package Change Request
======================
Package Name: csound
New Branches: EL-6
Owners: pbrobinson sdz
Comment 44 Kevin Fenzi 2010-06-11 00:15:18 EDT
cvs done.

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