Hi Daniel, Thanks for your work on this, and I apologize for the delay in response. On Fri, Apr 13, 2012 at 02:39:10PM -0500, Daniel Echeverry wrote: > Hi, > > This week I've been working on packaging a new version of irssi-scripts. I > closed some bugs and updated some of the scripts. I uploaded the package to > debian mentors. [1] > > Additionally, I added a copyright.new in debian/, This file is copyright > copyright DEP5 Update to 1.0 format. > > Could you check out the new version and the new copyright file? > I've reviewed your packaging work now and overall it looks really good. Though I do wonder, have you used a git repository to track your changes? If not, we currently use a repository located here: http://anonscm.debian.org/gitweb/?p=collab-maint/irssi-scripts.git This would be preferable because it helps to make collaborating and reviewing changes easier (this would also replace the need to build and upload the source package to mentors). I think it should be okay for us to allow you to push changes to the repository there, if you want...are you a member of collab-maint on alioth? Anyways, the only major issue I've noticed is that this detail got removed from debian/rules, but the executable permission is relied on by the script: # log2ansi.pl will run outside irssi, so it needs the executable bit chmod +x $D/usr/share/irssi/scripts/log2ansi.pl So, we need to set the executable permission on this file during build. Some other more minor things I noticed are: The debian/copyright.new file looks very good, however I think the GPL-2.0 license definition should replace the text about receiving a copy of the license with a reference to /usr/share/common-licenses, as you have done with the GPL-2.0+ block. Also, the "License:GPL-2.0+" is missing a space after the colon. I think it should be fine to replace debian/copyright with your debian/copyright.new once these small improvements are made. With regards to dependencies on perl and perl-modules, this is really not necessary since the base perl, which is all that is required for most scripts, is always installed. In debian/README.Debian there is an explanation of how the dependencies are chosen (at least 3 scripts = Suggests, at least 20 scripts = Recommends), and it looks like there are actually more scripts which depend on libwww-perl than perl or perl-modules, from the list in debian/README.Debian. I'm not opposed to changing how this is done if there is value added in doing so, but if we make that choice we will need to update debian/README.Debian as well (and should possibly treat the other dependencies consistently). It might be better to just explain the way we do this to the requestor of this change instead. There were a couple patches (go.pl-multiple-networks.diff and away.pl.diff) which used to have links to bug reports instead of proper headers, and you've replaced those with DEP5 patch headers (which is great, thanks!), but I think it could be useful to still have the old bug references included within the new DEP5 information. > [1]: http://mentors.debian.net/debian/pool/main/i/irssi-scripts/ > irssi-scripts_20120326.dsc > > Thank you very much!! > No, thank you! I will be able to sponsor this upload once we address the above mentioned items and I do a final review, but first I also want to hear back about if and how you want to make use of the git repository for collaboration. Cheers, Ryan
Attachment:
signature.asc
Description: Digital signature