[buildd-tools-devel] Bug#545215: Bug#545215: Bug#545215: [PATCH] Please support saving chroot to tarball in sbuild-createchroot

[Roger Leigh]

On Sun, Sep 20, 2009 at 01:43:41PM -0400, Andres Mejia wrote:
> From: Andres Mejia <andres at andres-desktop.hsd1.va.comcast.net>
> Date: Sun, 20 Sep 2009 13:21:02 -0400
> Subject: [PATCH 3/5] Allow sbuild-update to support perform apt-get update, upgrade and distupgrade.
>  This also allows sbuild-update to perform any two or all three commands at once,
>  useful when using file type chroots.
> 
> ---
>  bin/sbuild-update |  119 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 110 insertions(+), 9 deletions(-)
> 
> diff --git a/bin/sbuild-update b/bin/sbuild-update
> index d6dac48..33dae2a 100755
> --- a/bin/sbuild-update
> +++ b/bin/sbuild-update
> @@ -21,19 +21,98 @@
>  use strict;
>  use warnings;
>  
> +use Sbuild::ChrootSetup qw(update upgrade distupgrade);
> +
> +use Sbuild::Conf;
> +
> +BEGIN {
> +    use Exporter ();
> +    our (@ISA, @EXPORT);
> +
> +    @ISA = qw(Exporter Sbuild::Conf);
> +
> +    @EXPORT = qw();
> +}
> +
> +sub init_allowed_keys {
> +    my $self = shift;
> +
> +    $self->SUPER::init_allowed_keys();
> +
> +    my %update_keys = (
> +	'UPDATE'				=> {
> +	    DEFAULT => 0
> +	},
> +	'UPGRADE'				=> {
> +	    DEFAULT => 0
> +	},
> +	'DISTUPGRADE'				=> {
> +	    DEFAULT => 0
> +	},
> +    );
> +
> +    $self->set_allowed_keys(\%update_keys);
> +}

I think that this is fine.  I would, however, perhaps additionally
add configuration keys to sbuild conf immediately after this part
in order to allow sensible defaults to be set.  In this case, I
would suggest something along the lines of

$apt_update = 1;
$apt_upgrade = 0;
$apt_distupgrade = 0;

for backward compatibility and the principle of least surprise.

> +	"uu" => sub {
> +	    $self->set_conf('UPDATE', 1);
> +	    $self->set_conf('UPGRADE', 1);
> +	},
> +	"ud" => sub {
> +	    $self->set_conf('UPDATE', 1);
> +	    $self->set_conf('DISTUPGRADE', 1);

I'm not totally enamoured by these abbreviations.  It should be
possible to simply use --update --distupgrade together.  If you
add short options in addition to the long options, -ud will
just work without this extra option (upgrade might need to be
"-g" making "-ug" the same as "--uu" above).

> From 1205711f9d813c60dca38802a352ed2d78f77836 Mon Sep 17 00:00:00 2001
> From: Andres Mejia <andres at andres-desktop.hsd1.va.comcast.net>
> Date: Sun, 20 Sep 2009 13:23:26 -0400
> Subject: [PATCH 4/5] Have sbuild-upgrade and sbuild-distupgrade run sbuild-update for their functions instead.
> diff --git a/bin/sbuild-distupgrade b/bin/sbuild-distupgrade
> +print "$0 is deprecated. Use sbuild-update --dist-upgrade directly instead.\n";
> +system("/usr/bin/sbuild-update", "--dist-upgrade", @ARGV) == 0 or
> +    die "Exiting from sbuild-update with exit status $?";

Note: you must use exec() rather than system() here in order to get
the error status correctly returned to the shell.  You could also
return $? following the system() call, but IMO you should just
replace system with exec, and die with an error
"Can't run sbuild-update: $?" if it fails.


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.