Bug#662955: review
On Fri, 9 Mar 2012 01:02:42 David Banks wrote:
> Hi, I'm not a DD so I can't sponsor your package, but I would be using this
> package if it was in the archive. Thanks for packaging it.
Hi, wow thanks for the great detailed review!
> * You don't need the substvar ${shlibs:Depends} in the control file for
> the binary packages, since they are not compiled code. This gives a
> warning currently.
Okay, I will remove this.
> * Why is the package priority extra? It should probably be optional.
After re-reading the Policy Manual I see you are correct.
> * Consider switching off DH_VERBOSE in debian/rules before release, or at
> least remove the comment above it.
Yes, I missed that.
> * The translations seem to be being compiled twice, once during
> dh_auto_build and once during dh_auto_install. This is probably
> unnecessary.
Hmm, I hadn't really noticed that but you're correct. I'll have to patch the
configure script to remove the 'all' prerequisite from the makefile's
'install' target. It seems to be there because upstream's installation
instructions don't require a separate 'make' and 'make install'.
> * You may want to Recommend or Suggest the 'vorbisgain' and
> 'mp3gain' package, if rubyripper can use them. The same goes for
> 'normalize' but this may require a patch, since Debian uses the name
> 'normalize-audio' for this command. These were mentioned by the configure
> script.
> * Grepping the source it seems that rubyripper can also use 'cdrdao' and
> 'sox' for certain things -- consider the same for them.
Yes, I will Suggest these packages. Strange that the last two aren't mentioned
in any documentation that I've read (they're in the changelog though). I'll
file a bug report with upstream about it.
> * Consider changing your DEP-5 format URL now that this has become
> official.
> <http://www.debian.org/doc/packaging-manuals/copyright-format/1.0/>
Yep, I need to run lintian with -pedantic.
> * Do
> you need to patch the prefix '/usr' into the configure script? It seems
> you already specify the /usr prefix by an override in debian/rules anyway,
> so that seems redundant.
This was left-over from my attempts to fix a build issue. I'll remove it.
> * You might consider splitting your patch into multiple patches, which
> would allow you to give a more detailed rationale for each change. For
> instance, your patch removes some code from the configure script probably
> to solve some build issue, but the patch header doesn't explain this.
There were actually multiple build issues, which I didn't fully understand
until I had patched them.. so yes, I will split these patches out properly.
> * Since you use the same man page for both rrip_cli and rrip_gui, you may
> want to remove "(command-line interface)" from the NAME section of the
> manual page.
Totally missed that, thanks!
>
> Minor bugs that should probably be fixed by upstream:
> * Consider fixing the example copyright headers on the .po files, they
> have pasted in boilerplate.
I haven't filed a bug report with upstream over this, but it's on my TODO.
> * I get this message when running configure:
> "'gettext/utils.rb' is deprecated. Use gettext/tools.rb."
Yep, that can be patched.
> * Many translations are generating warnings like:
> - "Obsolete msgid exists"
> - "Fuzzy message was ignored"
>
This is another issue I have to raise with upstream. I did notice this, but
translations seemed to work fine anyway. Do you know what these warnings
actually mean? 'info gettext' doesn't provide any documentation for warnings.
> Thanks for your work! I will certainly be using this when it is uploaded.
>
> Cheers,
> David
No problem, thanks for taking the time to review the package! I should be able
to upload another version with the changes sometime this weekend..
--
Regards,
Scott.
Reply to: