[Bug 126] Review Request: dvdrip - Graphical DVD ripping and encoding tool

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=126


Orcan Ogetbil <orcanbahri at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |orcanbahri at yahoo.com
             Blocks|2                           |3
         AssignedTo|rpmfusion-package-          |orcanbahri at yahoo.com
                   |review at rpmfusion.org        |




--- Comment #7 from Orcan Ogetbil <orcanbahri at yahoo.com>  2008-12-05 10:25:06 ---
I finished the review. Here are my notes:

* rpmlint says
    dvdrip.src:27: W: non-break-space line 27
    dvdrip.src:41: W: non-break-space line 41
    dvdrip.src:49: W: non-break-space line 49
    dvdrip.x86_64: W: file-not-utf8 /usr/share/doc/dvdrip-0.98.9/Changes
    dvdrip.x86_64: W: file-not-utf8 /usr/share/doc/dvdrip-0.98.9/Credits
    dvdrip.x86_64: E: script-without-shebang
/usr/lib/perl5/vendor_perl/5.10.0/Video/DVDRip/Cluster/Webserver.pm
    dvdrip.x86_64: W: file-not-utf8 /usr/share/doc/dvdrip-0.98.9/README
    dvdrip.x86_64: W: file-not-utf8 /usr/share/doc/dvdrip-0.98.9/COPYRIGHT
    dvdrip.x86_64: W: file-not-utf8 /usr/share/doc/dvdrip-0.98.9/TODO
All of these can be fixed easily

* Please package the Changes.0.46, lib/Video/DVDRip/license.txt and 
lib/Video/DVDRip/translators.txt files

* Guidelines say that you need to add BR: gettext when dealing with locales:
    http://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files
There are binary .mo files inside the script. They need to be removed and
rebuilt from .po files by gettext.

? You can also remove the perl-modules directory in %prep

* You should make use of the %{name} macro extensively.

* What is wrong with the parallel make? I'm asking for both C side and perl
side.

* These BR's seem unnecessary:
     BuildRequires: perl(ExtUtils::MakeMaker)
     BuildRequires: perl(Gtk2) >= 1.081
     BuildRequires: perl(Locale::TextDomain) >= 1.16
The first two are pulled up by other ones. The last one isn't used during the
build, as far as I can tell.

* All patches should be explained in the SPEC file (and submitted upstream if
they are not Fedora-specific and links should be provided from their tracking
system). The patch #2 uses a hard-coded link. Is that the best solution?

* %define _default_patch_fuzz 2
Can't you just provide better patches to eliminate this?

* Please make a %check section and run the test script.

* The license field should be (GPL+ or Artistic) and CC-BY-SA 2.5
Please have a look at the COPYRIGHT file. Any reason you used GPLv2+ ?

? It would be quite useful for users to make the dependency detection script
inside the software display Fedora RPM names for optional packages. for
instance 
     subtitle2pgm -> subtitleripper.
But this is just an advice and not a blocker, as it needs some patching. I
leave it up to you


-- 
Configure bugmail: http://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.