Bug 375871 (libdvdnav) - Review Request: libdvdnav - A library for reading DVD video discs based on Ogle code
Summary: Review Request: libdvdnav - A library for reading DVD video discs based on Og...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: libdvdnav
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Alex Lancaster
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 213597
TreeView+ depends on / blocked
 
Reported: 2007-11-11 12:30 UTC by Dominik 'Rathann' Mierzejewski
Modified: 2007-12-12 19:33 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-11-21 21:16:14 UTC
Type: ---
Embargoed:
alex: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Dominik 'Rathann' Mierzejewski 2007-11-11 12:30:39 UTC
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.

Comment 1 Mike A. Harris 2007-11-11 12:35:25 UTC
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)


Comment 2 Dominik 'Rathann' Mierzejewski 2007-11-11 12:38:54 UTC
So was libdvdread, but it turned out to be OK for Fedora.

Comment 3 Mike A. Harris 2007-11-11 13:06:02 UTC
Would be nice if the same turns out for this one too then.  The more the merrier
I say. ;o)

Comment 4 Dominik 'Rathann' Mierzejewski 2007-11-11 13:25:50 UTC
Raising FE-Legal due to legal concerns. I think it's safe, but IANAL.

Comment 5 Alex Lancaster 2007-11-11 22:28:45 UTC
(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

Comment 6 Alex Lancaster 2007-11-11 22:34:58 UTC
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

Comment 7 Dominik 'Rathann' Mierzejewski 2007-11-11 22:53:06 UTC
(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).


Comment 8 Dominik 'Rathann' Mierzejewski 2007-11-11 22:54:09 UTC
(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.


Comment 9 Alex Lancaster 2007-11-11 22:55:47 UTC
(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.)


Comment 10 Tom "spot" Callaway 2007-11-12 01:44:33 UTC
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.

Comment 11 Alex Lancaster 2007-11-12 04:36:31 UTC
(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.



Comment 12 Tom "spot" Callaway 2007-11-12 18:40:50 UTC
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.

Comment 13 Dominik 'Rathann' Mierzejewski 2007-11-12 20:51:49 UTC
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.


Comment 14 Alex Lancaster 2007-11-12 22:50:47 UTC
(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



Comment 15 Alex Lancaster 2007-11-12 22:54:50 UTC
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).

Comment 16 Dominik 'Rathann' Mierzejewski 2007-11-12 23:07:21 UTC
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

Comment 17 Dominik 'Rathann' Mierzejewski 2007-11-13 17:17:15 UTC
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


Comment 18 Kevin Fenzi 2007-11-13 20:18:17 UTC
cvs done.

Comment 19 Dominik 'Rathann' Mierzejewski 2007-11-15 20:51:11 UTC
Built for devel. Updates for earlier branches will follow after testing it in devel.

Comment 20 Ville Skyttä 2007-11-16 19:19:04 UTC
Is it really necessary to cause soname bump woes for earlier branches?

Comment 21 Dominik 'Rathann' Mierzejewski 2007-11-16 21:52:14 UTC
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.

Comment 22 Dominik 'Rathann' Mierzejewski 2007-11-21 21:16:14 UTC
I've found a way to build against existing libdvdread, so no need for soname
bump in F-7/8. Patches submitted upstream. :)

Comment 23 Dominik 'Rathann' Mierzejewski 2007-12-12 10:57:02 UTC
Package Change Request
======================
Package Name: libdvdnav
New Branches: EL-5

Comment 24 Kevin Fenzi 2007-12-12 19:33:54 UTC
cvs done.


Note You need to log in before you can comment on or make changes to this bug.