Bug 440597 - Review Request: olpcsound - OLPC subset of csound 5
Review Request: olpcsound - OLPC subset of csound 5
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
i386 Linux
low Severity urgent
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-04 05:43 EDT by Victor Lazzarini
Modified: 2008-06-21 11:27 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-06-20 14:11:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Victor Lazzarini 2008-04-04 05:43:04 EDT
Spec URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound.spec
SRPM URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound-5.08.91-0.src.rpm
Description: This a subset of Csound 5 tailored specifically for the
OLPC project and the XO platform. The main purpose of this package is to
substitute the full Csound 5 package currently being used by
OLPC with this subset (which is in sync with the full Csound 5 releases).
This subset has been created by the Csound 5 team and will be maintained by
them. I was chosen to become the package maintainer and I will need to
be sponsored, as this is my first package.
Comment 1 Mamoru TASAKA 2008-04-18 21:34:19 EDT
Well, olpc2 seems to have only i386, however rebuild fails on
dist-f9 x86_64 and I think this is worth fixing.

http://koji.fedoraproject.org/koji/taskinfo?taskID=569171
Comment 2 Mamoru TASAKA 2008-04-19 14:29:27 EDT
Well, from a very quick glance:
* Please consider to use %?dist tag:
  http://fedoraproject.org/wiki/Packaging/DistTag
* License tag "LGPL" is invalid for current Fedora license policy:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
  http://fedoraproject.org/wiki/Licensing
* "Requires: liblo libsndfile" is redundant and should be removed.
  These types of library dependency are automatically detected and
  added to rebuilt binary rpms.
* Don't specify %python_site_dir in the way you write now and
  please refer to
  http://fedoraproject.org/wiki/Packaging/Python
* Remove long Authors entry from %description. If you want this,
  write them in a file and include it as %doc.
* Fedora specific compilation flags are not honored (you can check
  what flags are used by "$rpm --eval %optflags")
* Please make it sure that all directories created when installing a
  rpm are owned by the rpm. For example,
  %{_libdir}/csound/ is not owned by any package.
Comment 3 Victor Lazzarini 2008-04-19 17:45:48 EDT
(In reply to comment #1)
> Well, olpc2 seems to have only i386, however rebuild fails on
> dist-f9 x86_64 and I think this is worth fixing.
> http://koji.fedoraproject.org/koji/taskinfo?taskID=569171

I am not sure this olpc version of Csound can be built on
x86_64. I don't think anything can be done about that. Is it
a problem?
Comment 4 Mamoru TASAKA 2008-04-20 01:55:26 EDT
Fedora rawhide csound srpm has a patch named
csound-5.03.0-64-bit-plugin-path.patch so please investigate the
patch.
Comment 5 Mamoru TASAKA 2008-04-20 01:59:11 EDT
(In reply to comment #4)
> Fedora rawhide csound srpm has a patch named
> csound-5.03.0-64-bit-plugin-path.patch so please investigate the
> patch.

Ummm.. It seems that on 64 bit csound does some tricky method
to install plugins corretly. As olpc2 seems to be supporting i386
only, I guess we can ignore it for now. So please fix the rest issues.
Comment 6 Victor Lazzarini 2008-04-20 04:21:49 EDT
I will do. Thanks for the review.
Comment 7 Victor Lazzarini 2008-05-02 10:24:47 EDT
I have now addressed all the points raised in the comments above. 
Could someone take another look? I would like to continue the review 
process, as the Csound developers for OLPC are depending on this.
I also need sponsoring...

Spec URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound.spec
SRPM URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound-5.08.91-0.fc7.src.rpm

The source rpm name has changed due the use of the dist tag.
Comment 8 Mamoru TASAKA 2008-05-07 12:41:57 EDT
From next time please change the EVR (Epoch-Version-Release)
of your spec/srpm every time you modify your spec/srpm to
avoid confusion.

For (2nd) 5.08.91-0:
* License
-------------------------------------------------------
Whole:		LGPLv2+

OOps/random.c			BSD
Opcodes/Loris/lorisgens5.C	GPLv2+
Opcodes/Loris/lorisgens5.h	GPLv2+
Opcodes/Loris/morphdemo.py	GPLv2+
Opcodes/py/pycall-gen.py	GPLv2+
Opcodes/scansyn.c		NON-FREE
Opcodes/scansyn.h		NON-FREE
Opcodes/scansynx.c		NON-FREE
SDIF/sdif-mem.c			MIT
SDIF/sdif-mem.h			MIT
SDIF/sdif.c			MIT
SDIF/sdif.h			MIT
examples/cscore/		GPLv2+
frontends/CsoundX/AudioCode/	NON-FREE
util/*.{c,h}			GPLv2+
util/sortex/			GPLv2+
-------------------------------------------------------
  - To follow http://fedoraproject.org/wiki/Packaging/LicensingGuidelines :
    * libscansyn.so is non-free and cannot be allowed for Fedora
      so please remove this.
    * libstdutil.so is under GPLv2+. So the license tag of olpcsound
      rpm should be "LGPLv2+ and GPLv2+". Also some explanation is
      needed on spec file. Please refer to the section
      "Multiple Licensing Scenarios" of "LicensingGuidelines" wiki.

* Requires
  - Requires for -devel subpackage is wrong for now as:
-------------------------------------------------------
$ rpm -qp --requires olpcsound-devel-5.08.91-0.olpc2.i386.rpm 
libcsnd.so.5.1  
libcsound.so.5.1  
olpcsound=5.08.91-0.olpc2  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
-------------------------------------------------------
    * This shows that -devel subpackage now Requies the rpm
      named "olpcsound=5.08.91-0.olpc2", not "olpcsound" with
      EVR 5.08.91-0.olpc2.

* Optflags
  - Would you explain why you want "-ffast-math"? This option
    changes (reduces) precision and may render debugging difficult.

* Macros
  - Use macros properly. For example /usr/bin should be %_bindir.
    (by the way why do you want to call scons by full path?)

* Directory ownership issue
  - Again please make it sure all directories created when installing
    a rpm are owned by the rpm.
    * For example, %_libdir/csound is not owned by any package.

* Build working directoryy issue
---------------------------------------------------------
%files -f %{_builddir}/%{name}-%{version}/csound5.lang
---------------------------------------------------------
  - %{_builddir}/%{name}-%{version}/ part is redundant because
    at this stage the working directory is the directory.

* debuginfo rpm issue
  - build.log says:
---------------------------------------------------------
   808  + /usr/lib/rpm/find-debuginfo.sh /builddir/build/BUILD/olpcsound-5.08.91
   809  extracting debug info from
/var/tmp/olpcsound-5.08.91-0.olpc2-root-mockbuild/usr/lib/libcsound.so.5.1
   810  extracting debug info from
/var/tmp/olpcsound-5.08.91-0.olpc2-root-mockbuild/usr/lib/libcsnd.so.5.1
   811  extracting debug info from
/var/tmp/olpcsound-5.08.91-0.olpc2-root-mockbuild/usr/lib/python2.5/site-packages/_csnd.so
   812  0 blocks
---------------------------------------------------------
    This means that these binaries are stripped before %install
    stage ends. Make it sure that these binaries are _not_ stripped
    to create debuginfo rpm properly.

* Documents
  - Please add the following files to %doc.
---------------------------------------------------------
AUTHORS
---------------------------------------------------------
  - The file "INSTALL" is for people who want to build and install
    a software by themselves and is not needed for people who use
    rpm system.

* rpmlint issue
---------------------------------------------------------
olpcsound.i386: W: file-not-utf8 /usr/share/doc/csound/readme-csound5.txt
olpcsound.i386: E: description-line-too-long olpcsound .....
olpcsound.i386: W: no-version-in-last-changelog
---------------------------------------------------------
  - Change the encoding of %_docdir/%name/readme-csound5.txt to UTF-8.
  - Make it sure that all lines in %desctiption should have less than
    80 characters.
  - Add EVR info to %changelog
Comment 9 Victor Lazzarini 2008-05-07 12:59:04 EDT
I can take care of everything bar this one:

*debuginfo rpm issue: the install script strips the binaries. 
I can change that, but then where do I do the stripping? Should
I do it explicitely in the rpm somewhere?
Besides, the csound spec (approved by Fedora) does seem to do the
same thing, so if it's wrong here it is wrong there too.

(That goes also for the ownership of directories. In the approved
csound spec, no one owns the %_libdir/csound directory, just the
%_libdir/csound/plugins. So it's wrong there too)

Comment 10 Mamoru TASAKA 2008-05-07 13:10:29 EDT
(In reply to comment #9)
> I can take care of everything bar this one:
> 
> *debuginfo rpm issue: the install script strips the binaries. 
> I can change that, but then where do I do the stripping? Should
> I do it explicitely in the rpm somewhere?

You should _not_ strip those binaries. rpmbuild automatically
strips those binaries properly at last.
Some explanation is written in
http://fedoraproject.org/wiki/Packaging/Debuginfo

> Besides, the csound spec (approved by Fedora) does seem to do the
> same thing, so if it's wrong here it is wrong there too.
I have not checked csound spec file, from your comment csound
spec file is also wrong.

> (That goes also for the ownership of directories. In the approved
> csound spec, no one owns the %_libdir/csound directory, just the
> %_libdir/csound/plugins. So it's wrong there too)
Same above. 

Comment 11 Victor Lazzarini 2008-05-07 13:20:58 EDT
Ok that's easy, just take the strip out of the installer 
script. Where does it say that rpmbuild does the stripping?
I remember seeing that the binary package built without the 
stripping was about 5 times bigger than the one using the
stripping installer (which tells me it was not stripped).
But if you say it's done automatically, I am OK with it
(but I will check).

Someone ought to look at the fedora project csound.spec. I could
do it once I get this done and am sponsored.

Thanks.
Comment 12 Mamoru TASAKA 2008-05-07 13:56:05 EDT
(In reply to comment #11)
> Ok that's easy, just take the strip out of the installer 
> script. Where does it say that rpmbuild does the stripping?

I said this on my comment 8. Or you can check what __spec_install_post
does.
Comment 13 Mamoru TASAKA 2008-05-15 11:13:13 EDT
ping?
Comment 14 Victor Lazzarini 2008-05-15 11:21:09 EDT
sorry had no time to make the needed changes. I am hoping to
have time early next week, from my university work. There is
one morning's worth of work there, checking everything, so
I should have it by Monday afternoon. 
Comment 15 Victor Lazzarini 2008-05-19 08:35:45 EDT
I think I have addressed everything now. Here is the
latest src rpm and spec:

Spec URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound.spec
SRPM URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound-5.08.92-0.fc7.src.rpm

Thanks again
Comment 16 Mamoru TASAKA 2008-05-19 10:57:29 EDT
$ LANG=C rpm -K olpcsound-5.08.92-0.fc7.src.rpm 
error: olpcsound-5.08.92-0.fc7.src.rpm: headerRead failed

It seems that your srpm itself is broken...
Comment 17 Victor Lazzarini 2008-05-19 11:24:36 EDT
sorry, something went wrong in the upload. It seems to be fixed now,
same links as above.
Comment 18 Mamoru TASAKA 2008-05-19 14:07:14 EDT
For 5.08.92-0:

* License
------------------------------------------------
Opcodes/Loris/		GPLv2+
examples/cscore/	GPLv2+
------------------------------------------------
  - Now libstdutil.so is also under LGPLv2+
    (also checked 2008-05-08 on ChangeLog)
    License tag can be "LGPLv2+" now.

* Compiler flags
------------------------------------------------
   164  gcc -o Engine/auxfd.os -c -O3 -mtune=k6 -DGNU_GETTEXT -DUSE_LRINT -O2 -g
-pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables -g -fomit-frame-pointer -DLINUX -DPIPES
-fvisibility=hidden -fPIC -DHAVE_LIBSNDFILE=1016 -DBETA
-DENABLE_OPCODEDIR_WARNINGS=0 -DOLPC -DHAVE_SOCKETS -DHAVE_FCNTL_H
-DHAVE_UNISTD_H -DHAVE_STDINT_H -DHAVE_SYS_TIME_H -DHAVE_SYS_TYPES_H
-DHAVE_TERMIOS_H -DHAVE_SOCKETS -DHAVE_DIRENT_H -D__BUILDING_LIBCSOUND -I. -IH
-I/usr/include/fltk-1.1 -I/usr/local/include -I/usr/include -I/usr/include
-I/usr/X11R6/include Engine/auxfd.c
------------------------------------------------
  - -fomit-frame-pointer is strictly forbidden for Fedora as
    this makes debugging almost impossible.

* Directory ownership issue
  - %_includedir/csound is not owned by any packages.

* rpmlint issue
------------------------------------------------
olpcsound.i386: E: description-line-too-long olpcsound is a subset of the Csound
sound and music synthesis system, tailored specifically for  XO platform.
------------------------------------------------
  - Please check $ rpmlint -I description-line-too-long

Then:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 19 Victor Lazzarini 2008-05-20 09:30:32 EDT
OK these are the fixes

Spec URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound.spec
SRPM URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound-5.08.92-1.fc7.src.rpm

Did you say you are waiting yourself to be sponsored? Does this mean I
need to get someone else to do an official review?
Thanks
Comment 20 Mamoru TASAKA 2008-05-20 10:10:47 EDT
No, what I am saying is that you must
- sumbit another review request
- or do a pre-review of other person's review request
so that I or another sponsor can sponsor you (I am sponsor members).
Comment 21 Victor Lazzarini 2008-05-20 10:17:28 EDT
Ok. Sorry I misunderstood you. I will try and submit another
package. This might take a few days to prepare. Thanks
Comment 22 Victor Lazzarini 2008-05-28 06:58:03 EDT
I have submitted a new review request, here is the bug number: 448702
Thanks.
Comment 23 Mamoru TASAKA 2008-05-28 09:45:46 EDT
* Source file
----------------------------------------------------
5687217 2008-05-19 21:03 olpcsound-5.08.92-0.fc7/olpcsound-5.08.92.tar.bz2
5660600 2008-05-20 18:29 olpcsound-5.08.92-1.fc7/olpcsound-5.08.92.tar.bz2
  - What happened?
----------------------------------------------------

* rpmlint
  - Also now rpmlint complains about spurious-executable-perm
    on -debuginfo rpm:
----------------------------------------------------
olpcsound-debuginfo.i386: W: spurious-executable-perm
/usr/src/debug/olpcsound-5.08.92/util/mixer.c
olpcsound-debuginfo.i386: W: spurious-executable-perm
/usr/src/debug/olpcsound-5.08.92/Opcodes/hrtfopcodes.c
----------------------------------------------------

* Changelog
----------------------------------------------------
olpcsound-debuginfo.i386: W: no-version-in-last-changelog
----------------------------------------------------
  - Please write %changelog in the format like:
----------------------------------------------------
* Mon May 19 2008 Victor.Lazzarini <vlazzarini@nuim.ie>  - 5.08.92-1
  - fixed license
  - removed -fomit-frame-pointer from SConstruct
  - fixed description
----------------------------------------------------
Comment 24 Victor Lazzarini 2008-05-28 11:49:49 EDT
Thanks, I realised only today about the changelog format
as I was doing the othe package. 
As for the source code, I have removed some files that were
not being built, so the source is smaller now.
Not sure about the spurious exec perm, will check and fix.
Thanks again for your help.
Comment 25 Mamoru TASAKA 2008-05-28 12:44:49 EDT
(In reply to comment #24)
> As for the source code, I have removed some files that were
> not being built, so the source is smaller now.

Please don't modify the tarball used as the source of srpm by yourself
but use the upstream released tarball as it is (except for some special
case).
Or are you one of the upstream developer and 5.08.92 tarball is
not a formally released one?

> Not sure about the spurious exec perm, will check and fix.
Usually the permissions of the files in source tarball are wrong.
Comment 26 Victor Lazzarini 2008-05-28 13:30:41 EDT
> Or are you one of the upstream developer and 5.08.92 tarball is
not a formally released one?

I am one of the upstream developers and the 5.08.92 is a formally
released one, which has been updated. Is that a problem?
Comment 27 Mamoru TASAKA 2008-05-28 13:47:46 EDT
The problem is that the tarball downloaded from the URL
and the tarball in your srpm differ, which is very confusing.

We don't allow to modify the upstream tarball _itself_ except
for some special case (of course I am not saying that we should not
apply any patches or so, just saying that the tarball itself
must coincide with what is on upstream URL).
If you are the upstream and want to modify the tarball _itself_,
please release the new version of tarball.
Comment 28 Victor Lazzarini 2008-05-28 13:57:38 EDT
yes, that is my plan: the tarball that goes in the srpm should
be the same one release in the URL. In my hurry to submit fixes
I might have forgot to update the release. But when I submit the
fixes tomorrow, I will also release a matching tarball. Would
that be OK?
Thanks
Comment 29 Mamoru TASAKA 2008-05-28 14:06:39 EDT
Yes :)
Comment 30 Victor Lazzarini 2008-05-29 12:53:32 EDT
I think the problems above are now corrected:

Spec URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound.spec
SRPM URL: http://music.nuim.ie/vlazzarini/tmp/olpcsound-5.08.92-2.fc7.src.rpm

I have also updated the sourceforge release olpcsound tarball

Thanks
Comment 31 Mamoru TASAKA 2008-05-29 13:24:32 EDT
This time your srpm does not build.
http://koji.fedoraproject.org/koji/taskinfo?taskID=635125

By the way;
(In reply to comment #30)
> I have also updated the sourceforge release olpcsound tarball

Please don't! Please do not change the released tarball itself
without changing the version.

5661154 2008-05-30 01:35 new/olpcsound-5.08.92.tar.bz2
5687217 2008-05-19 21:19 old/olpcsound-5.08.92.tar.bz2

(In reply to comment #27)
> If you are the upstream and want to modify the tarball _itself_,
> please release the *new version* of tarball.

Comment 32 Victor Lazzarini 2008-05-29 14:23:26 EDT
build log says

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.

This seems to indicate that the compiler is not set up properly,
as stdio.h is not found.
I have seen this happen in fc7 systems using ccache, which causes
problems for scons. There is nothing I can do, because the same
code builds elsewhere. Could you suggest anything?
Thanks 
Comment 33 Mamoru TASAKA 2008-05-30 02:04:39 EDT
(In reply to comment #31)
> This time your srpm does not build.
> http://koji.fedoraproject.org/koji/taskinfo?taskID=635125

Sorry I built this not for the target olpc-3 but for the target
dist-f10. Actually on i386 (i.e. on olpc-3) build succeeds.
Will check later. 
Comment 34 Mamoru TASAKA 2008-05-30 02:44:55 EDT
Okay, good

----------------------------------------------------------------------
    This package (olpcsound) is APPROVED by me
----------------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor. At the stage, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.
Comment 35 Victor Lazzarini 2008-05-30 07:07:53 EDT
Thanks! 
My FAS name is veplaini

Comment 36 Mamoru TASAKA 2008-05-30 07:17:45 EDT
Now I should be sponsoring you. Please follow "Join" wiki
again.
Comment 37 Victor Lazzarini 2008-05-30 07:46:58 EDT
New Package CVS Request
=======================
Package Name: olpcsound
Short Description: olpc subset of csound 5
Owners: veplaini
Branches: olpc-3
InitialCC: veplaini
Cvsextras Commits: yes
Comment 38 Kevin Fenzi 2008-05-30 15:26:49 EDT
cvs done.
Comment 39 Mamoru TASAKA 2008-06-04 13:07:27 EDT
Please try to build on koji.
Comment 40 Victor Lazzarini 2008-06-04 13:20:08 EDT
I will. I just have been unable to work on this, as I have been
busy with University stuff admin and marking. Thanks.
Comment 41 Victor Lazzarini 2008-06-20 13:31:50 EDT
I finally was able to come back to it. Two horrid weeks
of admin work at my Uni. Anyway, all seemed to have gone
well: http://koji.fedoraproject.org/koji/buildinfo?buildID=53372

One question: At work, I am behind a firewall and the only way
to get out is through the proxy. I use a socks proxy with
cvs and curl, but I could not figure out how to make koji connect
(I tried tsocks which is what I use with cvs, to no avail). Do
you have any suggestions? As it is I am building from home, which
is not ideal.

Should I close this ticket?

Thanks for all your help, it really made a difference.
Comment 42 Victor Lazzarini 2008-06-20 13:55:12 EDT
Package Change Request
======================
Package Name: olpcsound
New Branches: olpc-2
Comment 43 Mamoru TASAKA 2008-06-20 14:11:14 EDT
(In reply to comment #41)
> One question: At work, I am behind a firewall and the only way
> to get out is through the proxy. I use a socks proxy with
> cvs and curl, but I could not figure out how to make koji connect
> (I tried tsocks which is what I use with cvs, to no avail). Do
> you have any suggestions? As it is I am building from home, which
> is not ideal.

Well, I am not a expert of koji and currently I cannot tell you
how to deal with that soon. Maybe it is better that you ask on
fedora-devel-list.
 
> Should I close this ticket?

Yes, now I am closing.

Comment 44 Kevin Fenzi 2008-06-21 11:27:46 EDT
cvs done.

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