[buildd-tools-devel] Bug#551311: Bug#551311: Updated patches for debuild like functionality in sbuild.
Roger Leigh
rleigh at codelibre.net
Thu Dec 10 00:54:46 UTC 2009
On Wed, Dec 09, 2009 at 05:10:58PM -0500, Andres Mejia wrote:
> On Tuesday 08 December 2009 20:50:12 Roger Leigh wrote:
> > On Thu, Nov 12, 2009 at 11:30:18PM -0500, Andres Mejia wrote:
> > > On Tuesday 27 October 2009 01:22:14 Andres Mejia wrote:
> > > > On Friday 23 October 2009 19:23:59 Andres Mejia wrote:
> > > > > Here are a new set of patches for some debuild like functionality
> > > > > implemented directly in sbuild. Details of what's new is in the
> > > > > patch for the man page but in short, sbuild would now be able to
> > > > > build from a Debianized source package, run lintian after a build,
> > > > > and run external commands before and after a build.
> > > > >
> > > > > With this, the sbuild-debuild script and manpage can be removed.
> > > >
> > > > Something I forgot to include was cleaning the source directory before
> > > > building the source packages. Here's a patch that fixes that.
> > >
> > > Were these patches reviewed? No rush, just wondering.
> >
> > Right, I've spent a few hours reviewing them tonight. It's the first
> > free time I've had in a while, so apologies for the delay.
> >
> > Firstly, I've attached an updated copy of the patches. These are
> > exactly the same as yours, but merge conflicts with current git
> > are fixed. I've then gone through the patches and my comments
> > follow for each patch in turn.
> >
> > Before I go into the criticism in detail, I just want to say that
> > overall I'm really happy with the idea behind the patches, and
> > would have applied them straight away. However, there are a few
> > issues which need working on first. These are mostly simple to
> > resolve, so it shouldn't be hard to fix them to go in. The major
> > issue is shell quoting which can cause potential problems due to
> > assuming that options and command paths never contain spaces,
> > which is easy to fix.
> >
> > 1) Improve the parse_file utility subroutine
> >
> > a) No checking of the return value of Sbuild::Utility::parse_file
> >
> > Previously, failure would result in die() being called. These
> > were replaced with returning a value, but the value isn't
> > checked. This means a failure will allow the program to
> > silently continue where it would have previously aborted.
>
> I didn't particularly want the program to just die. Also, any part of the code
> that calls parse_file() should expect a hashref. Whatever part of the program
> that uses parse_file() should be handling the case where a hashref is not
> returned.
Yes, it's just not doing that at the moment.
> > 2) Supply more options to be used with sbuild
> >
> > a) $dpkg_source_opts and $dpkg_source_opt are scalar
> >
> > This isn't safe, since unlike all other options passing code, it
> > can't handle spaces in options. Please see how --debbuildopts/
> > --debbuildopt are used to set DPKG_BUILDPACKAGE_USER_OPTIONS.
> > Here, we store all the options as separate array elements. If
> > a scalar string is provided, we split it and store the separate
> > elements.
>
> Right, will change this to cope with spaces in options.
Already done in my earlier patch, unless you want to make any
further changes.
> Will do the same here
>
> > c) @pre_build_commands and @post_build_commands are arrays
> >
> > Due to storing the command plus its arguments as a single array
> > element, this also introduces quoting issues. I suggest moving
> > to a slightly more complex, but safe, alternative. Two
> > suggestions:
> >
> > i) hash of arrayrefs
> >
> > my $post_build_commands = {
> > lintian => ['/usr/bin/lintian', '-i'],
> > piuparts => ['/usr/bin/piuparts']
> > };
> >
> > Here we have a shorthand name for the command as the key, and
> > the full command and any options as the value in the form of
> > an array reference.
>
> I think an array of arrayrefs is better so the order that the commands are run
> won't be random.
OK.
> This can be done for an array of arrayrefs as well.
>
> > Also, consider the need for your % escaping in the command
> > arguments. Since before the build we have a .dsc and after
> > the build we have an additional .changes, it could just
> > always add the .dsc as the last argument of the pre-build
> > and .changes as the last argument of the post-build command.
> > Would you ever need anything different?
>
> I would rather leave it to the user how to set the command they want to run,
> rather than messing with the command. For example, suppose a user has a script
> that's run like 'somescript --arg1=%c --arg2'. Just adding the .changes file at
> the end of the command will most likely be a problem in this script.
For the kind of specialised use this will be used for, the user can write
wrapper scripts for esoteric cases where it doesn't just take a filename
(as all the standard tools do).
My thinking here is: if we run scripts at defined points during the
build, those defined points should have a specific purpose. E.g.,
after a successful build, we always give the .changes as the last
argument, because checking the built packages is the only sensible
option at that point. Likewise we can give the build directory on
failure so the scripts can look at what went wrong and do any
cleanup or diagnosis. If you want something other than that then
you can either (1) add a new hook point or (2) use a wrapper.
> > Also, would you want to run a command inside the build chroot
> > in addition to on the host system? Approach (ii) above would
> > give the flexibility to do either.
> >
> > Would we want to run commands at points other than before and
> > after the build? Approach (ii) above would allow easy addition
> > of other points e.g. after a successful or failed build,
> > before chroot cleanup, or at any other points of our choosing
> > in the build process. This could be a very nice way of making
> > sbuild hookable. In fact, hard-coded logic in sbuild could
> > be moved into script fragments.
>
> Don't want to use a hash as the ordering of the commands can be important. I
> would add more array of arrayrefs with the commands to run at specific points
> of the build process inside and outside the chroot.
We can do this, certainly. The other approach would be to add the array
of arrayrefs to a top-level hash to split them up into categories. This
saves using lots of names and means the code can just say "run all the
scripts in $category" without needing to know what the variable is
named.
> Now of course a hash can be used with Tie::IxHash or Tie::Hash::Indexed,
> though I'm not sure if we should be pulling more dependencies for sbuild.
I'd rather not unless it's unavoidable.
> I think the problem was that set_version doesn't allow for a ':' when the
> upstream version had one. Debian policy does allow for a ':' in the upstream
> version, but only when an epoch is supplied.
In the DSC filename, this is true. set_version however will deal with
epochs just fine. The attached (untested) patch shows how this might be
done.
> > d) Is the build log now opened too early?
> >
> > I'll need to check more closely, but there was a reason the
> > build log was opened at the specific point it was at. Will
> > anything break by moving it to an earlier point?
>
> Haven't seen anything break with running sbuild, or with output to the logs.
I think historically this is to avoid spamming the buildd owner since
any non-empty log gets mailed. The placement at this point is needed
for that, but I'll need to double-check.
> > e) Addition of run_command function
> >
> > This function is redundant. See use of ChrootRoot in
> > the lib/WannaBuild and lib/Buildd modules. This provides
> > access to the host system using the Sbuild::Chroot interface,
> > which handles stream logging and quoting issues running
> > commands. As for all other quoting comments, it should not be
> > running commands with space-separated commands and arguments; it
> > must use an array. This is automatically handled by ChrootRoot.
>
> The run_command() subroutine in Sbuild::Build was needed to run commands
> outside the chroot.
That's exactly what ChrootRoot is intended for. See the attached patch
for an example. In particular, it supports
- running in a particular directory (no getcwd/chdir messing)
- logging (ties directly into the Build log streams)
- piping in either direction as needed
It's doing everything run_command does, plus more.
> > a) The return value of the clean command is not checked
> >
> > dpkg-buildpackage would immediately abort if the clean failed,
> > and we should do the same.
>
> Will fix.
Partially done in the attached patch.
diff --git a/lib/Sbuild/Build.pm b/lib/Sbuild/Build.pm
index 9930afb..b624483 100644
--- a/lib/Sbuild/Build.pm
+++ b/lib/Sbuild/Build.pm
@@ -41,6 +41,7 @@ use Sbuild::Base;
use Sbuild::ChrootSetup qw(update upgrade);
use Sbuild::ChrootInfoSchroot;
use Sbuild::ChrootInfoSudo;
+use Sbuild::ChrootRoot;
use Sbuild::Sysconfig qw($version $release_date);
use Sbuild::Conf;
use Sbuild::LogBase qw($saved_stdout);
@@ -64,6 +65,88 @@ sub new {
my $self = $class->SUPER::new($conf);
bless($self, $class);
+ $self->set('Invalid Source', 0);
+ $self->set('Arch', undef);
+ $self->set('Chroot Dir', '');
+ $self->set('Chroot Build Dir', '');
+ $self->set('Max Lock Trys', 120);
+ $self->set('Lock Interval', 5);
+ $self->set('Srcdep Lock Count', 0);
+ $self->set('Pkg Status', 'pending');
+ $self->set('Pkg Status Trigger', undef);
+ $self->set('Pkg Start Time', 0);
+ $self->set('Pkg End Time', 0);
+ $self->set('Pkg Fail Stage', 0);
+ $self->set('Build Start Time', 0);
+ $self->set('Build End Time', 0);
+ $self->set('This Time', 0);
+ $self->set('This Space', 0);
+ $self->set('This Watches', {});
+ $self->set('Toolchain Packages', []);
+ $self->set('Sub Task', 'initialisation');
+ $self->set('Session', undef);
+ $self->set('Additional Deps', []);
+ $self->set('Changes', {});
+ $self->set('Dependencies', {});
+ $self->set('Have DSC Build Deps', []);
+ $self->set('Log File', undef);
+ $self->set('Log Stream', undef);
+ $self->set('Debian Source Dir', undef);
+
+ # Check if the DSC given is a directory on the local system. This
+ # means we'll build the source package with dpkg-source first.
+ if (-d $dsc) {
+ my $host = Sbuild::ChrootRoot->new($conf);
+
+ my $pipe = $host->pipe_command(
+ { COMMAND => [$Sbuild::Sysconfig::programs{'DPKG_PARSECHANGELOG'},
+ "-l$dsc/debian/changelog"],
+ USER => $self->get_conf('USERNAME'),
+ PRIORITY => 0,
+ });
+
+ if (! $pipe) {
+ $self->log_error("Could not parse $dsc/debian/changelog: $!");
+ $self->set('Invalid Source', 1);
+ goto version_init;
+ }
+
+ my $stanzas = parse_file($pipe);
+
+ if (! close $pipe) {
+ $self->log_error("Could not close pipe: $!");
+ $self->set('Invalid Source', 1);
+ goto version_init;
+ }
+
+ my $stanza = @{$stanzas}[0];
+ my $package = ${$stanza}{'Source'};
+ my $version = ${$stanza}{'Version'};
+
+ if (!defined($package) || !defined($version)) {
+ $self->log_error("Missing Source or Version in $dsc/debian/changelog");
+ $self->set('Invalid Source', 1);
+ goto version_init;
+ }
+
+ $version = $self->strip_epoch($version);
+ my $dir = getcwd();
+ # Note: need to support cases when invoked from a subdirectory
+ # of the build directory, i.e. $dsc/foo -> $dsc/.. in addition
+ # to $dsc -> $dsc/.. as below.
+ if ($dir eq abs_path($dsc)) {
+ # We won't attempt to build the source package from the source
+ # directory so the source package files will go to the parent dir.
+ $dir = abs_path("$dir/..");
+ $self->set_conf('BUILD_DIR', $dir);
+ }
+ $self->set('Debian Source Dir', abs_path($dsc));
+
+ $self->set_version("${package}_${version}");
+ $dsc = "$dir/" . $self->get('Package_OSVersion') . ".dsc";
+ }
+
+version_init:
# DSC, package and version information:
$self->set_dsc($dsc);
my $ver = $self->get('DSC Base');
@@ -79,7 +162,6 @@ sub new {
($self->get('Debian Source Dir'))); # Debianized source directory
# Can sources be obtained?
- $self->set('Invalid Source', 0);
$self->set('Invalid Source', 1)
if ((!$self->get('Download')) ||
(!($self->get('DSC Base') =~ m/\.dsc$/) && # Use apt to download
@@ -106,32 +188,6 @@ sub new {
debug("Download = " . $self->get('Download') . "\n");
debug("Invalid Source = " . $self->get('Invalid Source') . "\n");
- $self->set('Arch', undef);
- $self->set('Chroot Dir', '');
- $self->set('Chroot Build Dir', '');
- $self->set('Max Lock Trys', 120);
- $self->set('Lock Interval', 5);
- $self->set('Srcdep Lock Count', 0);
- $self->set('Pkg Status', 'pending');
- $self->set('Pkg Status Trigger', undef);
- $self->set('Pkg Start Time', 0);
- $self->set('Pkg End Time', 0);
- $self->set('Pkg Fail Stage', 0);
- $self->set('Build Start Time', 0);
- $self->set('Build End Time', 0);
- $self->set('This Time', 0);
- $self->set('This Space', 0);
- $self->set('This Watches', {});
- $self->set('Toolchain Packages', []);
- $self->set('Sub Task', 'initialisation');
- $self->set('Session', undef);
- $self->set('Additional Deps', []);
- $self->set('Changes', {});
- $self->set('Dependencies', {});
- $self->set('Have DSC Build Deps', []);
- $self->set('Log File', undef);
- $self->set('Log Stream', undef);
-
return $self;
}
@@ -139,41 +195,10 @@ sub set_dsc {
my $self = shift;
my $dsc = shift;
- # Check if argument given is a directory on the local system. This means
- # we'll be building the source package via dpkg-source first.
- if (-d $dsc) {
- my $dpkg_parsechangelog =
- $Sbuild::Sysconfig::programs{'DPKG_PARSECHANGELOG'};
- my $command = "$dpkg_parsechangelog -l$dsc/debian/changelog";
- my $fh;
- if (! open $fh, '-|', $command) {
- $self->log_error("Could not parse $dsc/debian/changelog: $!");
- return 0;
- }
- my $stanzas = parse_file($fh);
- if (! close $fh) {
- $self->log_error("Could not close filehandle: $!");
- return 0;
- }
- my $stanza = @{$stanzas}[0];
- my $package = ${$stanza}{'Source'};
- my $version = $self->strip_epoch(${$stanza}{'Version'});
- my $dir = getcwd();
- if ($dir eq abs_path($dsc)) {
- # We won't attempt to build the source package from the source
- # directory so the source package files will go to the parent dir.
- $dir = abs_path("$dir/..");
- $self->set_conf('BUILD_DIR', $dir);
- }
- $self->set('Debian Source Dir', abs_path($dsc));
- $dsc = "$dir/$package" . "_$version.dsc";
- }
-
debug("Setting DSC: $dsc\n");
$self->set('DSC', $dsc);
$self->set('Source Dir', dirname($dsc));
-
$self->set('DSC Base', basename($dsc));
}
@@ -184,6 +209,8 @@ sub set_version {
debug("Setting package version: $pkgv\n");
my ($pkg, $version) = split /_/, $pkgv;
+ return if (!defined($pkg) || !defined($version));
+
# Original version (no binNMU or other addition)
my $oversion = $version;
# Original version with stripped epoch
@@ -229,6 +256,10 @@ sub set_status {
sub run {
my $self = shift;
+ my $host = Sbuild::ChrootRoot->new($self->get('Config'));
+ $self->set('Host', $host);
+ $host->set('Log Stream', $self->get('Log Stream'));
+
$self->set_status('building');
$self->set('Pkg Start Time', time);
@@ -255,21 +286,38 @@ sub run {
# Build the source package if given a Debianized source directory
if ($self->get('Debian Source Dir')) {
+ $self->set('Pkg Fail Stage', 'pack-source');
$self->log_subsection("Build Source Package");
- my $clean_command = $self->get_conf('FAKEROOT') . " debian/rules clean";
- my $dpkg_source = $self->get_conf('DPKG_SOURCE');
- my $dpkg_source_command = "$dpkg_source -b";
- $dpkg_source_command .= " " . join(" ", @{$self->get_conf('DPKG_SOURCE_OPTIONS')})
- if ($self->get_conf('DPKG_SOURCE_OPT'));
- $dpkg_source_command .= " " . $self->get('Debian Source Dir');
- my $curdir = getcwd(); # To get back to the directory we were in
- chdir($self->get('Debian Source Dir'));
- $self->log_subsubsection("$clean_command");
- $self->run_command($clean_command, 1, 1);
- chdir($self->get_conf('BUILD_DIR'));
- $self->log_subsubsection("$dpkg_source_command");
- $self->run_command($dpkg_source_command, 1, 1);
- chdir($curdir);
+
+ $self->log_subsubsection('clean');
+ $self->get('Host')->run_command(
+ { COMMAND => [$self->get_conf('FAKEROOT'),
+ 'debian/rules',
+ 'clean'],
+ USER => $self->get_conf('USERNAME'),
+ DIR => $self->get('Debian Source Dir'),
+ PRIORITY => 0,
+ });
+ if ($?) {
+ $self->log_error("Failed to clean source directory");
+
+ goto cleanup_skip;
+ }
+
+ $self->log_subsubsection('dpkg-source');
+ $self->get('Host')->run_command(
+ { COMMAND => [$self->get_conf('DPKG_SOURCE'),
+ '-b',
+ @{$self->get_conf('DPKG_SOURCE_OPTIONS')},
+ $self->get('Debian Source Dir')],
+ USER => $self->get_conf('USERNAME'),
+ DIR => $self->get_conf('BUILD_DIR'),
+ PRIORITY => 0,
+ });
+ if ($?) {
+ $self->log_error("Failed to build source package");
+ goto cleanup_skip;
+ }
}
my $chroot_info;
@@ -392,17 +440,26 @@ sub run {
# Run lintian.
my $lintian = $self->get_conf('LINTIAN');
if (($self->get_conf('RUN_LINTIAN')) && (-x $lintian)) {
- my $command = "$lintian";
- $command .= " " . join(" ", @{$self->get_conf('LINTIAN_OPT')})
- if ($self->get_conf('LINTIAN_OPT'));
- $command .= " " . $self->get('Changes File');
- $self->log_subsubsection("$command");
- my $return = $self->run_command($command, 1, 1);
+ $self->log_subsubsection("lintian");
+
+ $self->get('Host')->run_command(
+ { COMMAND => [$lintian,
+ @{$self->get_conf('LINTIAN_OPT')},
+ $self->get('Changes File')],
+ USER => $self->get_conf('USERNAME'),
+ PRIORITY => 0,
+ });
+ my $status = $? >> 8;
+
$self->log("\n");
- if (!$return) {
- $self->log_error("Lintian failed to run.\n");
- } else {
+ if (! $?) {
$self->log_info("Lintian run was successful.\n");
+ } else {
+ my $why = "unknown reason";
+ $why = "runtime error" if ($status == 2);
+ $why = "policy violation" if ($status == 1);
+ $why = "received signal " . $? & 127 if ($? & 127);
+ $self->log_error("Lintian run failed ($why)");
}
}
@@ -586,7 +643,7 @@ sub fetch_source_files {
$self->log($self->get_conf('APT_GET') . " for sources failed\n");
return 0;
}
- $self->set_dsc((grep { /\.dsc$/ } @fetched)[0]);
+ $self->set_dsc((grep { /\.dsc$/ } @fetched)[0]) || return 0;
}
if (!open( F, "<$build_dir/$dsc" )) {
--
.''`. 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091210/9030598b/attachment-0001.pgp>
More information about the Buildd-tools-devel
mailing list