This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 456353 - Review Request: libffado - Free firewire audio driver library
Review Request: libffado - Free firewire audio driver library
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jeff Garzik
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-22 23:18 EDT by Jarod Wilson
Modified: 2013-07-02 22:35 EDT (History)
13 users (show)

See Also:
Fixed In Version: libffado-2.0.1-3.20100706.svn1864.fc12
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-05 19:28:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
jgarzik: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Updated .spec file (3.37 KB, text/plain)
2010-01-24 07:01 EST, John Haxby
no flags Details

  None (edit)
Description Jarod Wilson 2008-07-22 23:18:49 EDT
Spec URL: http://wilsonet.com/packages/libffado/libffado.spec
SRPM URL: http://wilsonet.com/packages/libffado/libffado-2.0-0.1.beta6.fc10.src.rpm

Description:

The FFADO project aims to provide a generic, open-source solution for the
support of FireWire based audio devices for the Linux platform. It is the
successor of the FreeBoB project.

Has only been very briefly smoke-tested with an M-Audio FireWire Solo, but so far, so good...

-------
174917631659: Debug (devicemanager.cpp)[ 983] showDeviceInfo: ===== Device Manager =====
174917631668: Debug (Element.cpp)[ 109] show: Element DeviceManager
174917631676: Debug (devicemanager.cpp)[ 991] showDeviceInfo: --- IEEE1394 Service  0 ---
Iso handler info:
Dumping IsoHandlerManager Stream handler information...
 State: 2
174917631714: Debug (devicemanager.cpp)[1001] showDeviceInfo: --- Device  0 ---
174917631720: Debug (bebob_avdevice.cpp)[ 370] showDevice: Device is a BeBoB device
174917631729: Debug (ffadodevice.cpp)[ 205] showDevice: Attached to port.......: 0 (/dev/fw0)
174917631735: Debug (ffadodevice.cpp)[ 206] showDevice: Node...................: 0
174917631740: Debug (ffadodevice.cpp)[ 208] showDevice: Vendor name............: M-Audio
174917631744: Debug (ffadodevice.cpp)[ 210] showDevice: Model name.............: FW Solo
174917631750: Debug (ffadodevice.cpp)[ 212] showDevice: GUID...................: 000d6c0b007a3b48
174917631759: Debug (ffadodevice.cpp)[ 217] showDevice: Assigned ID....: dev0
174917631781: Debug (devicemanager.cpp)[1004] showDeviceInfo: Clock sync sources:
174917631845: Debug (devicemanager.cpp)[1013] showDeviceInfo:  Type: Compound Syt Match, Id: 36, Valid: 1, Active: 0, Locked 1, Slipping: 0, Description: Syt Match
174917631854: Debug (devicemanager.cpp)[1013] showDeviceInfo:  Type: Internal          , Id: 33, Valid: 1, Active: 1, Locked 1, Slipping: 0, Description: Internal (CSP)
174917631883:  (ffado-dbus-server.cpp)[ 298] main:  Starting DBUS service...
174917645981:  (ffado-dbus-server.cpp)[ 314] main:  Running... (press ctrl-c to stop & exit)
174917646004: Debug (ffado-dbus-server.cpp)[ 317] main: dispatching...
Found device 0: 000d6c0b007a3b48
 Found (000d6c0b007a3b48, D6C, 10062) M-Audio FW Solo
show dialog...
Init ffado reg window
-----
Comment 1 Brian Pepple 2008-08-10 19:03:01 EDT
Good:
* Source URL is canonical
* Upstream source tarball verified
* Package name conforms to the Fedora Naming Guidelines
* Group Tag is from the official list
* Valid license tag
* Buildroot has all required elements
* All paths begin with macros
* Files have appropriate permissions and owners

Bad:
* Fails to build in Mock.  Your missing a BuildRequires on expat-devel.

* rpmlint produces the following:
ffado.x86_64: W: no-documentation
ffado.x86_64: E: arch-dependent-file-in-usr-share /usr/share/libffado/tests/test-dbus
ffado.x86_64: E: arch-dependent-file-in-usr-share /usr/share/libffado/tests/test-isorecv-1
ffado.x86_64: E: arch-dependent-file-in-usr-share /usr/share/libffado/tests/test-isoxmit-1
ffado.x86_64: E: arch-dependent-file-in-usr-share /usr/share/libffado/tests/teststreaming3
ffado.x86_64: E: arch-dependent-file-in-usr-share /usr/share/libffado/tests/test-dbus-server
ffado.x86_64: E: arch-dependent-file-in-usr-share /usr/share/libffado/tests/test-ffado
libffado.x86_64: E: zero-length /usr/share/doc/libffado-2.0/NEWS
libffado.x86_64: E: zero-length /usr/share/doc/libffado-2.0/TODO
libffado.x86_64: W: no-soname /usr/lib64/libffado.so
libffado-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 8 errors, 3 warnings.

The documentation warning can be ignored, since you've got the relevant 
documention in the main package, though I would drop the NEWS & TODO files since they don't contain anything.
Comment 2 Jarod Wilson 2008-08-10 23:45:31 EDT
Got the BR fixed, dropped the empty doc files. Not quite sure offhand where to put the files that are triggering arch-dependent-file-in-usr-share... Suggestions?
Comment 3 Alex Kanavin 2008-11-06 05:24:23 EST
There is a new upstream release (beta7) available. Also, it seems to require a newer version of jack than what's available in rawhide.
Comment 4 Jarod Wilson 2008-11-06 14:32:05 EST
Gah. I completely forgot all about this... New build tossed together.

Spec URL: http://wilsonet.com/packages/libffado/libffado.spec
SRPM URL: http://wilsonet.com/packages/libffado/libffado-2.0-0.3.beta7.fc10.src.rpm

I didn't see any reference to a newer version of jack being required though, and it built just fine on a rawhide x86_64 system... Ah. Then again, I'm running a local build of jack that is probably sufficiently new...
Comment 5 Alex Kanavin 2008-11-06 14:43:17 EST
Er, it's straight in the README:

jackd (>= 0.109.12), http://jackaudio.org
[NOTE: at the time of writing, this is the development (SVN) version.]

http://subversion.ffado.org/ffado/tags/2.0-beta7/README


Rawhide seems to have 0.109.2.
Comment 6 Jarod Wilson 2008-11-06 15:17:57 EST
Ah, I was looking at the beta6 to beta7 changelog. So that was already a requirement for beta6 too, I believe. I've got a 0.112.x svn snap I'm running here, completely forgot about that. This should probably target F11 rawhide, once its open, and hopefully, jack will get the necessary update to go with it...
Comment 7 John Haxby 2009-05-16 04:43:01 EDT
2.0-rc1 is available now.

Is there any chance of this making it into F11?
Comment 8 Jarod Wilson 2009-05-18 09:33:14 EDT
As of yesterday, now -rc2 is available. Doubt this will make it into the F11 release media, but at least we have a new enough jack now, so it could easily be an F11 update... Updated rc2 packages here:

http://wilsonet.com/packages/libffado/libffado.spec
http://wilsonet.com/packages/libffado/libffado-2.0-0.4.rc2.fc11.src.rpm

Builds on F11-x86_64 just fine, haven't tested anything beyond that yet.
Comment 9 Alex Kanavin 2009-11-07 05:56:28 EST
What's the current status? libffado is still not in rawhide as far as I can see.
Comment 10 Jay Fenlason 2009-11-09 11:03:13 EST
It doesn't work yet.  ffado does not yet work with the new firewire stack.  I've written some patches for libffado's sources and libraw1394, which get it closer to working, but no success yet.  As of Friday I had the streaming subsystem starting, but libffado does not recognize that streaming is running and shuts down.  Stay tuned.
Comment 11 Jay Fenlason 2010-01-07 15:40:04 EST
Rawhide now has a sufficiently new libraw1394 (2.0.5-1.fc13) to work with ffado.  There's at least a small chance we can get this into f13, if we hurry.
Comment 12 John Haxby 2010-01-24 07:01:02 EST
Created attachment 386419 [details]
Updated .spec file

In F12, scons can be used to install libffado.  Apart from simplifying the .spec file this also means that the ffado mixers are install and I can use the monitor mixer for my FA-101.

It would be great to be able to get away from having to install an old firewire module to get this to be usable in F13.  This will be all the excuse I need to install the beta, but if you'd like me to install a test rawhide system please let me know.
Comment 13 Lennart Poettering 2010-05-09 08:57:55 EDT
Hmm, I'd love to see this in Fedora, so that I can add support for firewire cards to PulseAudio. What's the latest on this package?
Comment 14 Stefan Richter 2010-05-09 13:07:53 EDT
Re comment 13:  You probably know that, but to be sure:  libffado is not so much a library in the classic sense but a jack backend driver module.

Debian's package maintainer of FFADO currently packages ffado-trunk, as the current upstream release 2.0.0 does not fully work with the newer firewire kernel driver stack AFAIU.  It sounded as if there is going to be a respective 2.0.1 release any day now.  ffado-trunk OTOH also brings considerably expanded device support, to be released in FFADO 2.1.
Comment 15 Jarod Wilson 2010-05-10 10:38:36 EDT
I'm personally not able to dedicate any time to getting this package squared away right now, so if anyone else wants to take on the task of submitting an updated package for review, they should feel free to do so.
Comment 16 Lennart Poettering 2010-05-10 17:53:28 EDT
(In reply to comment #14)
> Re comment 13:  You probably know that, but to be sure:  libffado is not so
> much a library in the classic sense but a jack backend driver module.

It's a library too. I have been discussing this in detail at LAC with Pieter Palmers from ffado, who even gave me a firewire soundcard so that i can implement PA support for ffado.
Comment 17 Orcan Ogetbil 2010-06-05 00:29:43 EDT
I'll work on this if nobody minds. I want to have this on F-14, since we are updating jack to jack2, and it's good to keep compatibility with CCRMA, which ships ffado for a while now.
Comment 18 Orcan Ogetbil 2010-06-05 15:28:10 EDT
A little talk with a ffado developer at IRC revealed that 2.0.0 is fully compatible with the new kernel stack. He also says that the svn is preferable as it has better support fer sapphire line of products. svn has the same API/ABI as 2.0.0 so I will go with packaging the svn.
Comment 19 Orcan Ogetbil 2010-06-05 22:02:11 EDT
Here we go:

SPEC: http://oget.fedorapeople.org/review/libffado.spec
SRPM: http://oget.fedorapeople.org/review/libffado-2.0.0-1.20100605.svn1845.fc13.src.rpm

Not perfect, but I cleaned most of the lints. I didn't send any of the patches upstream yet. I'll do that after we get things settled.
Comment 20 Orcan Ogetbil 2010-06-22 13:51:04 EDT
Brian, are you still reviewing this package as you have set the fedora‑review flag to "?" a while ago. Or shall I try to find another reviewer? We want to have this package in by F-14 since we want to build jack2 with ffado support.
Comment 21 Brian Pepple 2010-06-22 15:01:18 EDT
(In reply to comment #20)
> Brian, are you still reviewing this package as you have set the fedora‑review
> flag to "?" a while ago. Or shall I try to find another reviewer? We want to
> have this package in by F-14 since we want to build jack2 with ffado support.    

I'm fairly busy with work, and probably won't have time to review this so you should look for another reviewer. Sorry.
Comment 23 Orcan Ogetbil 2010-07-07 16:52:46 EDT
Removing bpepple from the assignee slot.
Comment 24 Jeff Garzik 2010-07-08 00:29:52 EDT
> libffado.src: W: spelling-error Summary(en_US) firewire -> fire wire, fire-wire, firework
> libffado.src: W: invalid-url Source0: libffado-2.0.1-svn1864.tar.bz2
> ffado.x86_64: W: spelling-error Summary(en_US) firewire -> fire wire, fire-wire, firework

ignore

> ffado.x86_64: W: no-documentation
> ffado.x86_64: W: no-manual-page-for-binary ffado-diag
> ffado.x86_64: W: no-manual-page-for-binary ffado-fireworks-downloader
> ffado.x86_64: W: no-manual-page-for-binary ffado-mixer
> ffado.x86_64: W: no-manual-page-for-binary ffado-dbus-server
> ffado.x86_64: W: no-manual-page-for-binary ffado-bridgeco-downloader

some documentation would be nice.  not even a basic list of "this program does <this>" exists.

> libffado.x86_64: W: spelling-error Summary(en_US) firewire -> fire wire, fire-wire, firework

ignore

> libffado.x86_64: W: shared-lib-calls-exit /usr/lib64/libffado.so.2.999.0 exit@GLIBC_2.2.5

explanation?

> libffado-devel.x86_64: W: spelling-error Summary(en_US) firewire -> fire wire, fire-wire, firework

ignore

> libffado-devel.x86_64: W: spurious-executable-perm /usr/share/doc/libffado-devel-2.0.1/tests/dbus_test.py
> libffado-devel.x86_64: W: doc-file-dependency /usr/share/doc/libffado-devel-2.0.1/tests/dbus_test.py /usr/bin/python

seems a bit strange as a %doc?
Comment 25 Orcan Ogetbil 2010-07-08 00:53:01 EDT
Thanks!

(In reply to comment #24)
> > ffado.x86_64: W: no-documentation
> > ffado.x86_64: W: no-manual-page-for-binary ffado-diag
> > ffado.x86_64: W: no-manual-page-for-binary ffado-fireworks-downloader
> > ffado.x86_64: W: no-manual-page-for-binary ffado-mixer
> > ffado.x86_64: W: no-manual-page-for-binary ffado-dbus-server
> > ffado.x86_64: W: no-manual-page-for-binary ffado-bridgeco-downloader
> 
> some documentation would be nice.  not even a basic list of "this program does
> <this>" exists.
> 

Yeah. I'll do what every good packager should do. Steal it from Deb... uhm, I mean ask upstream.

> 
> > libffado.x86_64: W: shared-lib-calls-exit /usr/lib64/libffado.so.2.999.0 exit@GLIBC_2.2.5
> 
> explanation?
> 

I searched for this one but couldn't find the exit() call with a grep. Maybe it gets in via some weird #include. I'll dig further. Let me know if you can find it.

> > libffado-devel.x86_64: W: spurious-executable-perm /usr/share/doc/libffado-devel-2.0.1/tests/dbus_test.py
> > libffado-devel.x86_64: W: doc-file-dependency /usr/share/doc/libffado-devel-2.0.1/tests/dbus_test.py /usr/bin/python
> 
> seems a bit strange as a %doc?    

What is the best place to put these? Is %{_datadir}/%{name} good?
Comment 26 Jeff Garzik 2010-07-08 18:09:18 EDT
(In reply to comment #25)
> (In reply to comment #24)
> > > libffado.x86_64: W: shared-lib-calls-exit /usr/lib64/libffado.so.2.999.0 exit@GLIBC_2.2.5
> > 
> > explanation?
> > 
> 
> I searched for this one but couldn't find the exit() call with a grep. Maybe it
> gets in via some weird #include. I'll dig further. Let me know if you can find
> it.

'nm src/libffado.so' seems to find at least one call to exit() in the area where the following symbols are found:

00114d8f t csr1212_remove_cache
0011376a t csr1212_rom_cache_malloc
002d1484 b dtor_idx.5965
002d1464 d err_array_elem_type
002d1468 d err_duplicate_setting
         U exit@@GLIBC_2.0
0010604c t exitfunc
         U fabsf@@GLIBC_2.0
         U fclose@@GLIBC_2.1
         U ferror@@GLIBC_2.0
00106097 T ffado_get_api_version
00106081 T ffado_get_version
0014e604 T ffado_ringbuffer_create
0014e6b8 T ffado_ringbuffer_free
0014eb03 T ffado_ringbuffer_get_read_vector


> > > libffado-devel.x86_64: W: spurious-executable-perm /usr/share/doc/libffado-devel-2.0.1/tests/dbus_test.py
> > > libffado-devel.x86_64: W: doc-file-dependency /usr/share/doc/libffado-devel-2.0.1/tests/dbus_test.py /usr/bin/python
> > 
> > seems a bit strange as a %doc?    
> 
> What is the best place to put these? Is %{_datadir}/%{name} good?    

A more basic question, does it need to be shipped at all?  Unit tests are typically run during the make-dist-tarball process upstream, and also, during the Fedora package build process.

If the end user has no need for dbus_test.py (used for testing FFADO dbus interface?), then why ship it?  We don't ship the gcc testsuite with the gcc package, as documentation, for example.

If the end user does have a use for dbus_test.py, it seems that an executable-binaries directory would be more appropriate.
Comment 27 Jeff Garzik 2010-07-08 18:51:29 EDT
(category, pkg naming guidelines)

* Package version: instructions could be improved to describe how to produce that exact commit and tarball shipped with the SRPM, rather than general instructions about how the tarball was created when the snapshot was taken.

(category, pkg guidelines)

* The following seems to indicate your devel package needs some additional Requires:
https://fedoraproject.org/wiki/Packaging:Guidelines#Requires

* "All patches in Fedora spec files SHOULD have a comment above them about their upstream status."

(category, pkg/python, https://fedoraproject.org/wiki/Packaging:Python)

* "when building a package which ships python3 files, you need

BuildRequires: python3-devel"

(ditto for python2)

(category, review guidelines)

OK, seems to pass everything here, including test builds, though I'm unable to actually test this due to lack of firewire audio hardware.
Comment 28 Orcan Ogetbil 2010-07-08 22:30:10 EDT
I'll do the changes you asked, but I have two questions:

> 'nm src/libffado.so' seems to find at least one call to exit() in the area
> where the following symbols are found:

I know that the exit() symbol is in the library. I just can't find what source file it originates from. Doesn't the U in 
         U exit@@GLIBC_2.0
mean "undefined"?

> * The following seems to indicate your devel package needs some additional
> Requires:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requires

Can you be more specific?

Thanks.
Comment 29 Jeff Garzik 2010-07-08 22:49:35 EDT
(In reply to comment #28)
> I'll do the changes you asked, but I have two questions:
> 
> > 'nm src/libffado.so' seems to find at least one call to exit() in the area
> > where the following symbols are found:
> 
> I know that the exit() symbol is in the library. I just can't find what source
> file it originates from. Doesn't the U in 
>          U exit@@GLIBC_2.0
> mean "undefined"?

Correct.  Undefined, for that .o module.  'U' basically means an external reference that was not found in the source code at compile time.  For example, this C file

    #include <stdlib.h>
    #include <stdio.h>

    int foo(int blahbla)
    {
	printf("blahblah %d\n", blahbla);
	return 0;
    }

produces

    0000000000000000 T foo
                     U printf

thus "U exit@@GLIBC_2.0" is an external reference, and the symbols "close" to it are typically found in the same .o file at link time as the external reference... making that a clue to help you track down the area of code calling exit(3).


> > * The following seems to indicate your devel package needs some additional
> > Requires:
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Requires
> 
> Can you be more specific?
> 
> Thanks.    

This part:  "Typically, the requirements for -devel packages need yet another look. They're not usually picked up automatically by rpm. [...] This means that gtk+-devel should contain

     Requires: glib-devel libXi-devel libXext-devel libX11-devel"

For example, will building libffado applications require libiec61883-devel or libraw1394-devel?  ffado.h appears to lack external dependencies, so I am guessing the answer is "no."

But I just wanted to check and make sure.  If the answer is "no" this item can be crossed off.
Comment 30 Orcan Ogetbil 2010-07-09 00:57:53 EDT
I found that the exit() call comes from the external library external/libconfig. I need to patch out the library and use the Fedora copy. But I want to consult upstream first.

As for the devel package, only one header file gets installed and it contains only one #include which is <stdlib.h> so I don't think we need other Requires.
Comment 31 John Haxby 2010-07-11 06:20:56 EDT
I re-compiled the F13 jack-audio-connection-kit with freebob replaced by ffado but unfortunately it doesn't work with my Edirol FA-101.  I have had this working with the old firewire stack and a previous version of ffado on F12.

ffado-diag still reports that it wants the old version of the firewire stack, but I suspect that's more to do with ffado-diag than anything else.

Do I need something more?  Do you want any diagnostics from me?
Comment 32 Stefan Richter 2010-07-11 06:47:41 EDT
Re comment 31:
John, please open a separate bug for this, or let us discuss this on ffado-user (requires subscription, http://sourceforge.net/mail/?group_id=192582) or linux1394-user (open for posting without prior subscription, http://sourceforge.net/mail/?group_id=2252).
Comment 33 John Haxby 2010-07-11 07:00:43 EDT
The non-functional parts might be to do with libavc1394-devel not being installed (and not required, but ffado-diag moans about it).

I'll take other problems to the ffado-user list and open bugs as needed.
Comment 34 Orcan Ogetbil 2010-07-11 10:56:59 EDT
John, I'll add the ENABLE_ALL flag on the next revision of the package. This will enable all devices, including those supported by libavc1394. Thanks for the heads up.

Meanwhile I am still waiting for a response from ffado developers about using the system versions of dbus and libconfig libraries, instead of the bundled ones. Stay tuned.
Comment 35 Stefan Richter 2010-07-11 13:15:52 EDT
I see libavc1394 only being used in tests/test-echo.cpp (which is probably not important to ffado users, in contrast to ffado developers) and in src/bounce/bounce_slave_avdevice.cpp.  The latter does not work on top of the new firewire kernel drivers at all since it also attempts to replace the local node's Configuration ROM for which we don't have an ioctl in firewire-core and never will.  (The bounce device seems to implement a rudimentary audio device on the local node.)

Hence I do not see any harm done by a build without libavc1394.  But admittedly I have not actually tested a build after libavc1394 headers being removed from the system yet.
Comment 36 Orcan Ogetbil 2010-07-13 00:37:10 EDT
Here is an update:

SPEC: http://oget.fedorapeople.org/review/libffado.spec
SRPM: http://oget.fedorapeople.org/review/libffado-2.0.1-2.20100706.svn1864.fc13.src.rpm

I am working with the Debian maintainer to patch out the external libraries. The discussion is on the upstream devel ML.

Changelog: 2.0.1-2.20100706.svn1864
- Add ENABLE_ALL flag to support more devices
- Don't bundle tests
- Include some preliminary documentation for the tools until the manpages arrive
- Patch out bundled libraries. Also fixes some rpmlints
- Improve the instructions how to create the tarball


I hope I got everything.
Comment 37 Jay Fenlason 2010-07-13 09:06:15 EDT
The configuration file needs to be installed in /etc/libffado/configuration and the configure options at build time need to be modified t use it.
Comment 38 Jeff Garzik 2010-07-13 17:41:27 EDT
Looks pretty good.  I see the following when running on libffado installed pkg (didn't know you could do that):

libffado.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libffado.so.2.999.0 /lib64/libexpat.so.1
libffado.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libffado.so.2.999.0 /usr/lib64/libxml2.so.2
libffado.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libffado.so.2.999.0 /lib64/libgobject-2.0.so.0
libffado.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libffado.so.2.999.0 /usr/lib64/libsigc-2.0.so.0
libffado.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libffado.so.2.999.0 /lib64/libglib-2.0.so.0
libffado.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libffado.so.2.999.0 /usr/lib64/libavc1394.so.0

I guess this, and Jay's comment, are the last remaining items?
Comment 39 Orcan Ogetbil 2010-07-13 23:32:26 EDT
(In reply to comment #38)
> Looks pretty good.  I see the following when running on libffado installed pkg
> (didn't know you could do that):
> 
> libffado.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libffado.so.2.999.0 /lib64/libexpat.so.1

ffado uses either expat or libxml++ for XML serialization. libxml++ is the default, which I didn't change. Removing BR: expat-devel should clear the above rpmlint.


> libffado.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libffado.so.2.999.0 /usr/lib64/libxml2.so.2
> libffado.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libffado.so.2.999.0 /lib64/libgobject-2.0.so.0
> libffado.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libffado.so.2.999.0 /usr/lib64/libsigc-2.0.so.0
> libffado.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libffado.so.2.999.0 /lib64/libglib-2.0.so.0

The above 4 are dragged in by
   $ pkg-config --libs libxml++-2.6
Not much I can do about them.

> libffado.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libffado.so.2.999.0 /usr/lib64/libavc1394.so.0
> 

As Stefan pointed out, libavc1394 is not really supported for the time being. I'll get rid of the ENABLE_ALL for now. I imagine if it is not on by default, there must be a reason.

> I guess this, and Jay's comment, are the last remaining items?    

Yeah, I have one more question. ffado wants to use the flags -fomit-frame-pointer, -ffast-math, -funroll-loops for optimization. Will these cause any harm? Is it okay if I add them to the build flags?
Comment 40 Jeff Garzik 2010-07-14 01:17:03 EDT
(In reply to comment #39)
> Yeah, I have one more question. ffado wants to use the flags
> -fomit-frame-pointer, -ffast-math, -funroll-loops for optimization. Will these
> cause any harm? Is it okay if I add them to the build flags?    

-ffast-math is generally fine.  It's a program choice which enables additional optimizations by deviating slightly from the IEEE standard, in ways most programs won't notice.  It cannot be enabled by default build-system-wide because it affects strict correctness, but it is fine if knowledgeable persons OK it on a per-pkg basis.

-fomit-frame-pointer should IMO be avoided in standard Fedora packages.  It is typically an over-optimization left over from i386 days, where registers are such a precious resource.  Although it varies from platform to platform, sometimes this can decrease ability to debug, and in Fedora, we want to generate valid debug info (stored in -debuginfo package).

-funroll-loops unrolls loops whose iteration count can be determined at compile time.  The idea is to increase pipelining at the expense of larger generated code size.  If the developers really insist it's necessary, go ahead and keep it.  But my default recommendation is to remove -funroll-loops, and let the compiler figure out what's best on its own.  Sometimes, it makes more sense to keep the code compact and code size small, using fewer cachelines.

So, unless there are vigorous objections, I would only add -ffast-math to the standard RPM build flags.
Comment 41 Orcan Ogetbil 2010-07-14 16:42:08 EDT
Latest fixes:

SPEC: http://oget.fedorapeople.org/review/libffado.spec
SRPM: http://oget.fedorapeople.org/review/libffado-2.0.1-3.20100706.svn1864.fc13.src.rpm

Changelog:
- Remove ENABLE_ALL
- Improve the libffado-dont-use-bundled-libs.patch
- Drop BR: expat-devel libavc1394-devel
- Move configuration file to the library package
- Minor enhancement in the .desktop file
- Add links to upstream tickets for patches
- Add -ffast-math to the compiler flags
- Add patch to compile against libconfig-1.4.5


I didn't move the configuration file to /etc. Well, I first made a patch and asked upstream about it. They say that the configuration file is not a traditional config file, it contains factory provided list of supported devices. However they will discuss this further. I am keeping it in /usr/share for now, and will act upon their decision.
Comment 42 Jeff Garzik 2010-07-14 17:07:01 EDT
Review approved, looks good.
Comment 43 Orcan Ogetbil 2010-07-14 20:14:52 EDT
Thanks for the review.

New Package CVS Request
=======================
Package Name: libffado
Short Description: Free firewire audio driver library
Owners: oget
Branches: F-12 F-13
InitialCC:
Comment 44 Kevin Fenzi 2010-07-15 01:27:32 EDT
CVS done (by process-cvs-requests.py).
Comment 45 Fedora Update System 2010-07-18 17:31:07 EDT
libffado-2.0.1-3.20100706.svn1864.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/libffado-2.0.1-3.20100706.svn1864.fc13
Comment 46 Fedora Update System 2010-07-18 17:31:54 EDT
libffado-2.0.1-3.20100706.svn1864.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/libffado-2.0.1-3.20100706.svn1864.fc12
Comment 47 Jeff Garzik 2010-07-18 18:33:37 EDT
Per review process, now that packages are built, I think you (as Contributor) can set this bug to CLOSED - NEXTRELEASE:
https://fedoraproject.org/wiki/PackageReviewProcess#Contributor
Comment 48 Orcan Ogetbil 2010-07-18 18:38:07 EDT
Hi Jeff,
No worries, bodhi will close the ticket automatically when it is pushed to stable. Please read the line numbered with "10" in the link you gave.
Comment 49 Fedora Update System 2010-07-20 18:47:12 EDT
libffado-2.0.1-3.20100706.svn1864.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update libffado'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/libffado-2.0.1-3.20100706.svn1864.fc12
Comment 50 Fedora Update System 2010-08-05 19:28:47 EDT
libffado-2.0.1-3.20100706.svn1864.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 51 Fedora Update System 2010-08-05 19:42:30 EDT
libffado-2.0.1-3.20100706.svn1864.fc12 has been pushed to the Fedora 12 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.