GNOME Bugzilla – Bug 134276
cdda-method.c does not build under NetBSD
Last modified: 2006-04-21 14:10:12 UTC
cdda-method.c has a (preprocessor) conditional that checks for Linux and for FreeBSD. If the system is anything else, the build will fail (this includes NetBSD and others), as it will find an unbalanced if/else. Inspecting cdparanoia's headers, I found this in the cdrom_drive structure: #ifndef __FreeBSD__ char *cdda_device_name; char *ioctl_device_name; int cdda_fd; int drive_type; #else struct cam_device *dev; union ccb *ccb; #endif Therefore, the code in gnome-vfs' cdda-method.c must check for FreeBSD, but fallback to the other block in all other scenarios, not just Linux. The fix is simple: revert the conditional order, checking for __FreeBSD__ first, and defaulting to the other block with an #else. Note that this happens twice in the code.
Created attachment 24374 [details] [review] Sample patch for cdda-method.c
Which cdparanoia version do you have ? I don't have this #ifndef in /usr/include/cdda_interface.h here.
"Ooops". Sorry for the long, long delay. I didn't have time to verify and reply at the moment when you sent the message, and forgot since then. Well, I've just checked what's happening. The FreeBSD support in the cdda_interface.h is _not_ in cdparanoia's mainstream version. That file is actually patched by FreeBSD's ports system, as you can see in the following link: http://www.freebsd.org/cgi/cvsweb.cgi/ports/audio/cdparanoia/files/patch-interface-cdda_interface.h?rev=1.1&content-type=text/x-cvsweb-markup In NetBSD's pkgsrc, we use the same patch to let cdparanoia build in FreeBSD (you know, pkgsrc is portable to several systems ;). But note that NetBSD does _not_ require this patch to get cdparanoia working; it could work the same way without the patch. So, gnome-vfs' cdda-method.c file actually has a workaround for a FreeBSD-specific change. I don't say it shouldn't be there; the problem is just that it's incorrectly done because it will make all non-Linux and non-FreeBSD systems fail. (It's clear that it'll be broken because of incorrectly balanced braces). Hope this clarifies the problem...
new comment with details, reopening
Thanks for your efforts Julio! Maybe you could update your patch against HEAD? The code sections you're trying to patch were recently modified to match against more FreeBSD flavor systems.
Created attachment 52942 [details] [review] Updated patch Sorry again for the long delay. The new patch applies correctly against 2.12.0. Hope it helps now.
Just to let you know: the attached patch still applies to CVS HEAD, which is 2.14.0 at the moment (modules/cdda-method.c is at revision 1.24).
Thinking a bit more about this issue, it seems dangerous to me to rely on FreeBSD (or DragonFly) applying custom, non-standard patches to cdparanoia's code. gnome-vfs can (and should) do a better work in detecting if those patches were applied, instead of just assuming that some operating systems have them (that is, avoiding the ugly #ifdef __FreeBSD__ machinery). I'm updating the patch with one that adds a new check to configure.in to detect if cdparanoia contains the FreeBSD-specific patch. The advantage of this approach is that if some other system ever applies the cdparanoia patch too (because, e.g., they import libcam), gnome-vfs will not need to be fixed. In fact, this happened when DragonFly appeared, needing to extend the #ifdef conditionals in the code.
Created attachment 62452 [details] [review] Patch to automatically detect FreeBSD-specific cdparanoia changes
Thanks for your efforts, I like this patch. Maybe you could submit it to the GnomeVFS mailing list [1] for review? [1] http://mail.gnome.org/mailman/listinfo/gnome-vfs-list
Commited.