[buildd-tools-devel] Bug#551311: Bug#551311: Updated patches for debuild like functionality in sbuild.
Roger Leigh
rleigh at codelibre.net
Wed Dec 9 01:50:12 UTC 2009
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.
It's actually returning 0 rather than undef. Since we return
a hashref, IMO undef is appropriate here, since 0 is a scalar
and we are not returning the correct type. So, I think you
should return undef, and the callers should check if the return
value is defined or else handle it or die themselves.
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.
b) $lintian_opts and $lintian_opt are scalar
Same rationale and fix as (a).
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.
ii) hash of hashes
my $build_commands = {
lintian => {
COMMAND => ['/usr/bin/lintian', '-i'],
TYPE => 'post-build',
CHROOT => 0
},
piuparts => {
COMMAND => ['/usr/bin/piuparts'],
TYPE => 'post-build',
CHROOT => 0
}
checkdsc => {
COMMAND => ['/my/checking/program'],
TYPE => 'pre-build',
CHROOT => 1
}
}
Note that in both of these cases, the --pre-build-commands and
--post-build-commands are much simpler; since the arguments and
details are all in the configuration file, one could just have
a single --build-commands=lintian,piuparts option which makes
everyday use much simpler. It also means you can have two set
of commands: a complete list of available commands, and a list
of commands actually in use.
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?
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.
3) Implement support for supplying directories…
a) Quoting issues calling dpkg-source
Use of $command rather than @command array. Note system() can
be passed an array rather than single string, and this avoids
quoting issues entirely. See most existing use of system in
Build.pm. See also 2(a) for a related part of the problem.
b) Quoting issues calling dpkg-parsechangelog
As above.
c) Does stripping the epoch always result in a correct version?
Do we really need strip_epoch? It's already done in set_version.
Just call set_version("${package}_${version}") and use the
'OSVersion' (original epoch-stripped version) stored in the Build
object.
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?
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.
4) Have sbuild process current directory if no argument…
OK.
5) Update sbuild man page for new features
OK. I would suggest some small amount of rewording and tidying to
make the documentation a bit more understandable for users new to
sbuild, but this can wait until the other issues are fixed.
6) Update build system and Debian packaging
OK.
7) Clean the source dir before building…
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.
b) Quoting issues calling clean command
As for previous commands, the arguments are space-separated and
it should be an array. Example:
my $clean_command = $self->get_conf('FAKEROOT') . " debian/rules clean";
would be
my @clean_command = ($self->get_conf('FAKEROOT'), 'debian/rules', 'clean';
and calling would change from
system($clean_command);
to
system(@clean_command);
or
system($self->get_conf('FAKEROOT'), 'debian/rules', 'clean');
That's it! Hopefully the rationale for each of these points makes
sense. If not, please ask and I'll try to clarify. If you don't have
time to make these corrections, I'm happy to do so, but as I'm
currently pushed for free time this might not be too soon.
So overall I think it's really promising, and with these changes I
think it will be a really great addition to sbuild. Many thanks for
all your work!
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Improve-the-parse_file-utility-subroutine-by-being-a.patch
Type: text/x-diff
Size: 2893 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091209/e89f56b0/attachment-0007.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Supply-more-options-to-be-used-with-sbuild.patch
Type: text/x-diff
Size: 7670 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091209/e89f56b0/attachment-0008.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Implement-support-for-supplying-directories-as-argum.patch
Type: text/x-diff
Size: 10809 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091209/e89f56b0/attachment-0009.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Have-sbuild-process-current-directory-if-no-argument.patch
Type: text/x-diff
Size: 1132 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091209/e89f56b0/attachment-0010.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Update-sbuild-man-page-for-new-features.patch
Type: text/x-diff
Size: 7389 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091209/e89f56b0/attachment-0011.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Update-build-system-and-Debian-packaging.patch
Type: text/x-diff
Size: 1273 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091209/e89f56b0/attachment-0012.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Clean-the-source-dir-before-building-a-the-source-pa.patch
Type: text/x-diff
Size: 1728 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20091209/e89f56b0/attachment-0013.patch>
-------------- 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/20091209/e89f56b0/attachment-0001.pgp>
More information about the Buildd-tools-devel
mailing list