[buildd-tools-devel] Bug#551311: Bug#551311: Updated patches for debuild like functionality in sbuild.

[Andres Mejia]

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.

>       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.

Using 'return 0' rather than 'return undef' is recommended by Perl::Critic, 
hence the reason I use 'return 0'. 'defined 0' and 'defined undef' will both 
result as false.

> 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.

>    b) $lintian_opts and $lintian_opt are scalar
> 
>       Same rationale and fix as (a).

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.

>       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.

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.

>       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.

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.

> 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.

Will fix.

>    b) Quoting issues calling dpkg-parsechangelog
> 
>       As above.

Will fix.

>    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.

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.

>    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.

>    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.

> 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.

Will fix.

>    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');

Will fix.

> 
> 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
> 

-- 
Regards,
Andres