On Sat, May 09, 2009 at 04:05:14PM -0700, Kees Cook wrote: > Here's my proposed patch to implement --setup-hook, which exports the build > settings, as well as the build Config into the environment.
It looks nice. Some comments about a couple of nits: - Is the hook script present on the host or in the chroot? From the pipe_command CHROOT=>1 it looks like it runs inside the chroot, but is this the intention? This can just the documented in the manpage if it is the case. One possibility to allow clean separation would be to pipe the script to schroot which could just invoke /bin/sh to read the script from stdin. This would require the script to be Bourne shell only, however. - Should get_env be in Sbuild::Base rather than Sbuild::Build? This is the base class for all objects in the Sbuild:: and Buildd:: hierarchies. Is it something that's going to be generally usable in other contexts or does it just have the one use? If this is the only intended use, I'd prefer it to be a Sbuild::Build method. It could also be a general function in Sbuild.pm. And this one isn't a problem at all, just a thought: - The Config object can contain scalars, arrayrefs and hashrefs (and other references such as filehandles). I think it only makes sense to export simple scalars into the environment. The code does this which is great. Do you see any potential use for exporting arrays and hashes as comma- and key=value comma-separated lists for example. I can't myself, I'm just thinking if there's any reasonable use for those variables, and if so, can they be easily used by scripts in this format. Regards, Roger -- .''`. Roger Leigh : :' : Debian GNU/Linux http://people.debian.org/~rleigh/ `. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/ `- GPG Public Key: 0x25BFB848 Please GPG sign your mail. -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
