Bug 225639

Since i'm upstream, i'll take on this review.

What is the rationale for building against the version of cdrtools that's part
of the tarball ? That version if particularly old, and I left it there mainly
for non-linux builds...

I guess we should also merge this package with gcdmaster in Extras, once the
build system merge is complete.

other issues:

- libvorbis-devel Require is not necessary, it's only used if you build gcdmaster

- xcdrdao.desktop source is unused

- make invocation should use %{?_smp_mflags}

- the --with-pccts* configure options are unnecessary

- use make install DESTDIR=$RPM_BUILD_ROOT instead of %makeinstall

> What is the rationale for building against the version of cdrtools that's part
of the tarball?

Removing/Replacing cdrtools from Fedora.

The rest was in there for historical reasons :)

Should I link statically against a GPL version of cdrtools?

> The rest was in there for historical reasons :)

That was my guess :-)

Yes, I would suggest to compile statically against cdrecord-devel. This implies
adding a BuildRequires on cdrecord-devel, and using the '--with-scglib=sys'
configure option. As i said, the partial cdrtools snapshot that is shipped with
cdrdao is pretty old, and Fedora's cdrtools contains many patches that cdrdao
can benefit from.

If cdrtools is removed completly, I have to pull in the cdrtools source in the
cdrdao src.rpm.
Did you consider using cdrkit?
http://people.redhat.com/harald/downloads/cdrkit/cdrkit-1.1.2-3/

Is the plan for FC-7 to remove cdrtools completely then ? If that's the case, it
might make sense to indeed compile against the cdrtools snapshot shipped with
cdrdao, at least as a temporary solution.

I have not tried to build cdrdao against cdrkit, at least not yet. It's unclear
to me how much work would be required to make this happen, and i think it would
fall outside the scope of this review anyway (unless it's a straightforward
patch). As soon as i have a little time, i'll try to release a new upstream
version with cdrkit support. I was also thinking about libburn support.
Unfortunately RL work is heavy right now, so I can't really give an ETA for this :-(

Ping? Now that cdrtools has been replaced, maybe this can easily be sorted out?

Yes, sorry for slacking off on this one. Will finalize review tonight or tomorrow.

I've been thinking about the future of cdrdao lately. I've worked a little on
gcdmaster, but done very little maintenance work on cdrdao. I looked at a port
to either cdrkit and libburn, but cdrkit doesn't have anything close to a
development API, and libburn lives at a higher layer (and is not very stable
yet), so it looks as though using the packaged cdrtools library is still the top
option as of now...

Created attachment 290225 [details]
Remove bogus error checking

Ok, let's get this over with :-)

Looking good. 2 things:

first, I'd add '--with-scglib=pkg' to the configure line to make it explicit
that we're building with the scsi library shipped with cdrdao. Ok it's a minor
point.

second, could you add the patch i just attached ? I fixes the scanbus command
on F-8 and devel. Patch should be applied at the end of current patch list.

Otherwise package looks good.

Harald, ping ?

I've checked in GCC 4.3 fixes upstream, or you can use the patch from gcdmaster F-9.

Cdrdao is no longer actively maintained (the last feature
enhancement was 3 years ago).


Cdrdao uses an extremely old libscg variant and if you care about
cdrdao, this libscg needs to be updated with recent original
software from:

ftp://ftp.berlios.de/pub/cdrecord/alpha/

Note that you cannot use the library from cdrtools fork distributed 
by readhat as this fork is in conflict with GPL and Copyright and cannot 
be legally distributed. The original software however has been approved
by Sun legal as free software without known legal problems.

BTW: Another reason for not using the fork is that the fork is not
actively maintained since nearly two years and as the fork is full 
of well documented bugs.

Cdrdao is essentially in "patch acceptance only" mode. We can't use libscg, for the well known licensing issues which I don't want to get into, but also because of the broken way it does device scanning.

Anyways, this is about the cdrdao merge review, and cdrdao now has to be compiled without libscg with the '--without-scg' option. It then uses the native sg interface and works just fine (although you'll want to use the current CVS trunk).

Please do not spread unproven claims.

There are no "well known license issues", there hoewever
is a deffamation campaign against free software run by some
Debian guys.

Cdrtools had a full in depth license review done by the Sun
legal department in Autum 2008 and no license problem was found.
The Sun legal department found that there is no problem from 
letting GPLd software use CDDLs libraries.

On the other side, the cdrtools fork currently distributed by
redhat cannot be distributed legally as it is in conflict 
with GPL and Copyright.

This needs to move somewhere else, I don't want to get into the whole licensing debate nonsense, and it's irrelevant to this review.

The fact remains: libscg can't be used on linux distros for purely technical reasons (device scanning, ConsoleKit support), and I don't even know why cdrdao used it in the first place. The current cdrdao CVS trunk can be compiled to use the native SG interface, and if something doesn't work I'll commit the necessary patches.

Do not try to discuss techical stuff if you miss the needed background.
information.

libscg works perfectly on Linux _because_ of device scanning and all
people who reported problems with the illegal software distributed with
various linix distributions are happy with the unmodified original
software.

Libscg is the oldest and best supported platform independend
SCSI pass through library.

> Do not try to discuss techical stuff if you miss the needed background.
> information.

Please stop polluting this BZ. You also know nothing about my technical background.

ping?

I'll take this review, as I have time to do it today.

[???] source0
Fedora guideline https://fedoraproject.org/wiki/Packaging:SourceURL says, that
links to SourceForge should use http://downloads.sourceforge.net/, but this 
package uses http://prdownloads.sourceforge.net
[YES] source files match upstream:
70d6547795a1342631c7ab56709fd1940c2aff9f  cdrdao-1.2.3.tar.bz2
[YES] package meets naming and versioning guidelines
[YES] specfile is properly named, is cleanly written and uses macros
consistently
[YES] dist tag is present
[YES] build root is correct
[???] license field matches the actual license
The license field says GPLv2, but the source code files in directories dao,
trackdb, xdao, and utils states that the software is under GPLv2+.

Jörg mentioned some legal problems -- as far as I can tell, those are outside of scope of this review. All files in the source package seems to be released under open source license.
[YES] license is open source-compatible
[YES] license text included in package
[YES] latest version is being packaged
[YES] BuildRequires are proper
[???] configure flags are appropriate
Parameter --with-mp3-support is used, but configure script does not find
libmad, so MP3 support is (correctly) disabled at the end. Also the option
with_mp3_support is enabled by default, even without the parameter.

IMHO it would be better to use --without-mp3-support

[YES] compiler flags are appropriate
[YES] %clean is present
[YES] package builds in mock
[YES] debuginfo package looks complete
[NO] rpmlint is silent

$ rpmlint *.rpm
cdrdao.i686: W: file-not-utf8 /usr/share/doc/cdrdao-1.2.3/CREDITS
gcdmaster.i686: W: non-conffile-in-etc /etc/gconf/schemas/gcdmaster.schemas
4 packages and 0 specfiles checked; 0 errors, 2 warnings.

The rpmlint output seems ok to me.

[YES] final provides and requires look sane
[OK] no %check is present
[OK] no shared libraries are added to the regular linker search paths in app
package
[OK] owns the directories it creates
[OK] doesn't own any directories it shouldn't
[YES] no duplicates in %files
[YES] file permissions are appropriate
[YES] scriptlets ok
[YES] code, not content
[YES] documentation is small, so no -docs subpackage is necessary
[YES] %docs are not necessary for the proper functioning of the package
[YES] no headers
[YES] no pkgconfig files
[YES] no libtool .la droppings

diff -u -r1.51 cdrdao.spec
--- cdrdao.spec	11 Jan 2010 13:42:16 -0000	1.51
+++ cdrdao.spec	13 Jan 2010 15:54:34 -0000
@@ -1,8 +1,8 @@
 Summary:   Writes audio CD-Rs in disk-at-once (DAO) mode
 Name:      cdrdao
 Version:   1.2.3
-Release:   3%{?dist}
-License:   GPLv2
+Release:   4%{?dist}
+License:   GPLv2+
 Group:     Applications/Multimedia
 URL:       http://cdrdao.sourceforge.net/
 Source0:   http://prdownloads.sourceforge.net/%{name}/%{name}-%{version}.tar.bz2
@@ -53,7 +53,6 @@
 %configure \
         --with-xdao \
         --without-scglib \
-        --with-mp3-support \
         --without-lame
 
 make %{?_smp_mflags}
@@ -128,6 +127,11 @@
 
 
 %changelog
+* Wed Jan 13 2010 Nikola Pajkovsky <npajkovs> - 1.2.3-4
+- Merge review #225639
+- no need option --with-mp3-support. it needs libmad(don't ship)
+- change license to GPLv2+
+
 * Mon Jan 11 2010 rrakus 1.2.3-3
 - Fixed typo