On 10:48 Mon 30 Jul , Jakub Wilk wrote:
> This is only a very rudimentary review. I don't have time to review
> this properly for the time being. Anybody else is welcome to do it
> for me. :)
Thanks.. Wonder how many more silly stuffs show up on actual review
;-)
>
> * Vasudev Kamath <kamathvasudev@gmail.com>, 2012-07-29, 22:27:
> > dget -x http://mentors.debian.net/debian/pool/main/s/suckless-tools/suckless-tools_39-1.dsc
> >
> > More information about hello can be obtained from http://www.example.com.
> >
> > Changes since the last upload:
> >
> >suckless-tools (39-1) unstable; urgency=low
>
> It doesn't look like it's suitable for wheezy, so please make it
> s/unstable/experimental/.
Done! When it should be moved to unstable? After wheezy release? It
does contains some new version of tools so asking.
>
> > [ Michael Stummvoll ]
> > * New Maintainer (Closes: #647090)
>
> [0]
>
> > [ Vasudev Kamath ]
> > * Imported new version of slock (Closes: #667796)
>
> This fixes a security issue, so please mention CVE number in the
> changelog.
Done
>
> > + Added myself as maintainer and Michael Stummvoll as Uploader
>
> I'd merge this item with [0].
Done
>
> > + Added dependency on dpkg-dev >= 1.16.1.1
>
> It'd nice to mention why it's needed.
Did it :-)
>
> >+This package contains a set of tools from suckless community as
> >+single package. To build the package you need to create source
> >+tarballs of individual tool component involved. This can be done
> >+by running following command from suckless-tools folder
> >+
> >+ fakeroot debian/rules get-orig-source
>
> Why fakeroot?
Well by habit wrote it :-).. Now fixed
>
> >+Forwarded: <no|not-needed|url proving that it has been forwarded>
>
> Please choose one. :)
Ouch.. I will end up doing one or other copy paste error. Fixed
>
> >+-$ $(tabbed -d >/tmp/tabbed.xid); urxvt -embed $(</tmp/tabbed.xid);
> >++$ $(tabbed \-d >/tmp/tabbed.xid); urxvt \-embed $(</tmp/tabbed.xid);
>
> If you're fixing this, please also fix the security hole (insecure
> use of temporary files).
Done too
>
> >+override_dh_installdocs:
> >+ dh_installdocs
> >+ for TOOL in $(TOOLS); \
> >+ do \
> >+ cp $${TOOL}/README $(D)/usr/share/doc/suckless-tools/README.$${TOOL}; \
> >+ done
>
> This for loop needs a "set -e"; see Policy §4.6. I see other parts
> of debian/rules has the same problem.
As discussed in IRC the SHELL := sh -e on top of rule file should
handle this.
>
> >+ @cd /tmp
> >+ @tar -cvf - suckless-tools_$(CURRENT_VERSION) 2> /dev/null | gzip -9 > ../suckless-tools_$(CURRENT_VERSION).orig.tar.gz
> >+ @rm -rf /tmp/suckless-tools_$(CURRENT_VERSION)
>
> This creates temporary files insecurely.
Fixed.
Instead of pushing new package to mentors I've pushed my changes to
collab-maint repository [1] Hope that is fine with you if not let me know
[1] git.debian.org:/git/collab-maint/suckless-tools.git
Thanks for the review.
--
Vasudev Kamath
http://copyninja.info
Connect on ~friendica: copyninja@{frndk.de | vasudev.homelinux.net}
IRC nick: copyninja | vasudev {irc.oftc.net | irc.freenode.net}
GPG Key: C517 C25D E408 759D 98A4 C96B 6C8F 74AE 8770 0B7E
Attachment:
signature.asc
Description: Digital signature