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.

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?

There is a new upstream release (beta7) available. Also, it seems to require a newer version of jack than what's available in rawhide.

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...

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.

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...

2.0-rc1 is available now.

Is there any chance of this making it into F11?

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.

What's the current status? libffado is still not in rawhide as far as I can see.

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.

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.

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.

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?

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.

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.

(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.

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.

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.

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.

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.

(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.

Updated to newest trunk, post 2.0.1.

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

Removing bpepple from the assignee slot.

> 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.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?

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.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?

(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.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.

(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.

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.

(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.

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.

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?

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).

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.

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.

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.

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.

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.

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?

(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?

(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.

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.

Review approved, looks good.

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:

CVS done (by process-cvs-requests.py).

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

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

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

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.

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

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.

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.