Spec URL: http://rathann.fedorapeople.org/review/libdvdnav.spec SRPM URL: http://rathann.fedorapeople.org/review/libdvdnav-4.1.1-1.fc7.src.rpm Description: libdvdnav provides a simple library for reading DVD video discs. The code is based on Ogle and used in, among others, the Xine dvdnav plug-in. Note: this comes with bundled libdvdread (and hence obsoletes the old libdvdread), since one of MPlayer developers is a new upstream for both now.
The libdvdnav-0.1.10-4.20070503.lvn8 is already in the 3rd party "livna" repository, presumeably due to legal concerns, so it's probably unlikely to be accepted into the master Fedora repo I'd assume. ;o)
So was libdvdread, but it turned out to be OK for Fedora.
Would be nice if the same turns out for this one too then. The more the merrier I say. ;o)
Raising FE-Legal due to legal concerns. I think it's safe, but IANAL.
(Full disclosure: this is my first review, apologies for any inconsistencies in advance). Everything looks OK, except that this also provides libdvdread{-devel}, is this intended to replace the existing libdvdread{-devel}? It seems to provide a newer .so version. If this isn't intended to replace then it needs to change package name to libdvdread4 or somesuch. Anyway, this (and possible legal issues) are the only blockers. Full review follows: MUST (OK): • rpmlint rpmlint libdvd* libdvdnav.i386: W: no-url-tag libdvdnav.i386: W: no-url-tag libdvdnav-debuginfo.i386: W: no-url-tag libdvdnav-devel.i386: W: no-url-tag libdvdread.i386: W: no-documentation libdvdread.i386: W: no-url-tag libdvdread-devel.i386: W: no-documentation libdvdread-devel.i386: W: no-url-tag same for source package • package name: OK • spec file name: OK • package guidelines: OK, pending FE-Legal • license: OK GPLv2+ • license field: OK • license in package: OK • spec file in US English: OK • legible spec file: OK • md5sum: OK 91ff52cade291e79060f87fd431fa1ab libdvdnav-4.1.1.tar.gz 91ff52cade291e79060f87fd431fa1ab libdvdnav-4.1.1.tar.gz • builds on F-8, i386: OK • BuildRequires: OK • locales (none): OK • ldconfig: OK • relocatable: N/A • owns directories it creates: OK • no duplicate %files: OK • %defattr: OK • %clean: OK • macros consistent: OK • no content: OK • no large doc: OK • no runtime info in %doc: OK • header files in -devel: OK • no static libs: OK • no pkgconfig: OK • .so files only in -devel: OK • -devel packages require base: OK • no .la files: OK • no desktop files needed: OK • doesn't own other files: OK • %install removes buildroot: OK • filenames are valid UTF-8: OK MUST (to fix): • this package provides libdvdread{-devel}, is this intended to replace the existing libdvdread? if so need to add conflicts/provides and co-ordinate with existing maintainer of libdvdread to obsolete that package • may need to have legal (or spot) to check that libdvdnav is OK SHOULD (not blockers) • license in upstream: OK • no translations available: OK • no scriptlets (other than %post): OK • no files outside standard locations: OK • build in mock: not checked yet, but can be done later
Another MUST issue I forgot (not necessarily a blocker if this is an upstream issue): - why does the package run ./autogen.sh is this necessary? doesn't upstream create a configure package in the tarball. if not, can you file a bug upstream and ask them to generate it, if they don't distribute configure in tarball, it won't block the review, but please include a link to the upstream bug asking for a proper tarball with configure pre-generated in the spec file
(In reply to comment #5) > Everything looks OK, except that this also provides libdvdread{-devel}, is > this intended to replace the existing libdvdread{-devel}? It seems to > provide a newer .so version. If this isn't intended to replace then it > needs to change package name to libdvdread4 or somesuch. Yes, this is intended. The new upstream maintains both in the same tree (old libdvdnav also had libdvdread bundled).
(In reply to comment #6) > - why does the package run ./autogen.sh is this necessary? doesn't upstream > create a configure package in the tarball. if not, can you file a bug upstream > and ask them to generate it, if they don't distribute configure in tarball, it > won't block the review, but please include a link to the upstream bug asking for > a proper tarball with configure pre-generated in the spec file They don't distribute it. I'll ask them to do it.
(In reply to comment #0) > Note: this comes with bundled libdvdread (and hence obsoletes the old libdvdread), since one of MPlayer developers is a new upstream for both now. Sorry, I guess I didn't read this carefully enough, it appears that this will obsolete the old libdvdread, and it looks like you are already the package maintainer for libdvdread, so you'll just need to co-ordinate with yourself to make sure the update goes smoothly (and obsolete the old CVS branch etc.)
I have several concerns: 1. This package seems to have little functional use without libdvdcss. 2. Even though libdvdcss is doing the actual work which would be considered a violation of the DMCA, this library has code like this: /* Perform CSS key cracking for this title. */ fprintf( stderr, "libdvdread: Get key for %s at 0x%08x\n", filename, start ); if( dvdinput_title( dvd->dev, (int)start ) < 0 ) { fprintf( stderr, "libdvdread: Error cracking CSS key for %s (0x%08x)\n", filename, start); } IMHO, this is a "bullet, gun" relationship. The bullet causes the wound, but the gun fires the bullet. I'm going to NACK this. Sorry. :( I think we'd just be taking on too much risk with this package.
(In reply to comment #10) > I have several concerns: > > 1. This package seems to have little functional use without libdvdcss. > 2. Even though libdvdcss is doing the actual work which would be considered a > violation of the DMCA, this library has code like this: > > /* Perform CSS key cracking for this title. */ > fprintf( stderr, "libdvdread: Get key for %s at 0x%08x\n", > filename, start ); > if( dvdinput_title( dvd->dev, (int)start ) < 0 ) { > fprintf( stderr, "libdvdread: Error cracking CSS key for %s (0x%08x)\n", > filename, start); > } Hang on, is that code from the libdvdnav (or -devel) main package, or from the subpackage libdvdread{-devel}? If it's from the libdvdread package (which it looks like from your example), then that's already been approved (as a separate package) and is already in Fedora: $ rpm -q libdvdread libdvdread-0.9.7-3.fc8 if it passed there, then it should be a matter of checking the code in the libdvdnav package.
Ehhhh. Alright. I'm not happy about it, but the genie is out of that bottle already (and I'm the one who did it). Lifting FE-Legal.
Thanks! Now, since libdvdread from new upstream has soname libdvdread.so.4, some packages will have to be rebuilt against it. Namely: dvdauthor, k3b, lsdvd and python-kaa-metadata.
(In reply to comment #13) > Thanks! > > Now, since libdvdread from new upstream has soname libdvdread.so.4, some > packages will have to be rebuilt against it. Namely: dvdauthor, k3b, lsdvd and > python-kaa-metadata. Thanks for that, good to know. We should definitely rebuild all dependent and push to testing before pushing to stable (may need http://fedoraproject.org/wiki/ReleaseEngineering/SOP/BuildRootOverrides for the rebuilds to pick up the libdvdread before it goes to stable). Since FE-Legal has been removed and you have clarified that this package will also supercede the old separate libdvdread package (be sure to orphan/obsolete), then I'm marking this as: APPROVED
One more thing: it would be good if you could also include a link to the bug or mailing list thread where you ask upstream to build tarballs in the spec file (e.g. near where autogen.sh is called).
Thanks for the review. Here's the link. I'll put it in a comment after importing. http://lists.mplayerhq.hu/pipermail/dvdnav-discuss/2007-November/000269.html
New Package CVS Request ======================= Package Name: libdvdnav Short Description: A library for reading DVD video discs based on Ogle code Owners: rathann Branches: F-7 F-8 Cvsextras Commits: yes
cvs done.
Built for devel. Updates for earlier branches will follow after testing it in devel.
Is it really necessary to cause soname bump woes for earlier branches?
It is not really necessary. It's just that current MPlayer SVN requires this new libdvdread since a few days and so I won't be able to update it in livna for earlier branches.
I've found a way to build against existing libdvdread, so no need for soname bump in F-7/8. Patches submitted upstream. :)
Package Change Request ====================== Package Name: libdvdnav New Branches: EL-5