[Bug 126] Review Request: dvdrip - Graphical DVD ripping and encoding tool
http://bugzilla.rpmfusion.org/show_bug.cgi?id=126
Orcan Ogetbil <orcanbahri(a)yahoo.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |orcanbahri(a)yahoo.com
Blocks|2 |3
AssignedTo|rpmfusion-package- |orcanbahri(a)yahoo.com
|review(a)rpmfusion.org |
--- Comment #7 from Orcan Ogetbil <orcanbahri(a)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.