[git-buildpackage] [PATCH] gbp-buildpackage-rpm: pass commands to mock

[Guido Günther]

On Sat, Jul 09, 2016 at 05:35:53PM +0200, Tzafrir Cohen wrote:
> On Thu, Jun 30, 2016 at 11:11:33PM +0200, Guido Günther wrote:
> > On Thu, Jun 30, 2016 at 10:59:17PM +0200, Tzafrir Cohen wrote:
> > > Fix gbp-buildpackage-rpm and the mock builder script to allow passing
> > > extra command-line parameters to the mock wrapper build script.
> > > 
> > > Also fixes support for --git-mock-options. However this cannot pass any
> > > option that has spaces in it (such as --define).
> > > 
> > > Signed-off-by: Tzafrir Cohen <tzafrir at debian.org>
> > > ---
> > >  bin/gbp-builder-mock                    | 16 +++++++++++-----
> > >  docs/manpages/gbp-buildpackage-rpm.sgml |  1 +
> > >  gbp/scripts/buildpackage_rpm.py         |  9 +++++----
> > >  3 files changed, 17 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/bin/gbp-builder-mock b/bin/gbp-builder-mock
> > > index 1e93493..0e3453c 100755
> > > --- a/bin/gbp-builder-mock
> > > +++ b/bin/gbp-builder-mock
> > > @@ -1,4 +1,4 @@
> > > -#!/bin/sh
> > > +#!/bin/bash
> > >  #
> > >  # Helper to invoke mock from 'gbp buildpackage-rpm'
> > >  #
> > > @@ -36,14 +36,21 @@ usage() {
> > >  	exit $EXIT
> > >  }
> > >  
> > > +declare -a EXTRA_ARGS
> > > +
> > >  while [ $# != 0 ]; do
> > >  	case "$1" in
> > > -		--help|-h|-?) usage 0;;
> > > +		--help|-h) usage 0;;
> > >  		*.spec) SPEC="$1";;
> > > +		*) EXTRA_ARGS+=("$1");;
> > >  	esac
> > >  	shift
> > >  done
> > >  
> > > +if [ ! -z "$GBP_BUILDER_MOCK_OPTIONS" ]; then
> > > +	EXTRA_ARGS+=($GBP_BUILDER_MOCK_OPTIONS) # unquoted
> > > +fi
> > 
> > Could catch, we weren't setting them at all.
> 
> It should just be removed. Adding a patch to remove it.
> 
> > 
> > > +
> > >  # Make sure we have the necessary tools.
> > >  if [ ! -x /usr/bin/mock ]; then
> > >      echo "mock not found; you need to install the mock package" >&2
> > > @@ -71,16 +78,15 @@ gbp_builder_mock() {
> > >  	local resultdir="$export_dir/$pat"
> > >  	local mock="mock -r $root --resultdir=$srpms --spec=$spec --sources=$sources"
> > >  
> > > -	$mock --buildsrpm
> > > +	$mock "${EXTRA_ARGS[@]}" --buildsrpm
> > >  	# Assuming that nothing was built in this directory since the previous command:
> > >  	local srpm=`ls -t $PWD/SRPMS/*.src.rpm 2>/dev/null| head -n1`
> > >  	if [ -z $srpm ]; then
> > >  		echo >&2 "$0: failed to create srpm"
> > >  		exit 1
> > >  	fi
> > > -	$mock --no-cleanup-after --resultdir $resultdir --rebuild "$srpm"
> > > +	$mock "${EXTRA_ARGS[@]}" --no-cleanup-after --resultdir $resultdir --rebuild "$srpm"
> > >  }
> > >  
> > > -
> > >  fix_arch
> > >  gbp_builder_mock
> > > diff --git a/docs/manpages/gbp-buildpackage-rpm.sgml b/docs/manpages/gbp-buildpackage-rpm.sgml
> > > index ac020f8..d5aa8f7 100644
> > > --- a/docs/manpages/gbp-buildpackage-rpm.sgml
> > > +++ b/docs/manpages/gbp-buildpackage-rpm.sgml
> > > @@ -63,6 +63,7 @@
> > >        <arg><option>--git-arch</option>=<replaceable>ARCHITECTURE</replaceable></arg>
> > >        <arg><option>--git-mock-options</option>=<replaceable>OPTIONS</replaceable></arg>
> > >        <arg><option>--git-mock-root</option>=<replaceable>ROOT</replaceable></arg>
> > > +      <arg rep="repeat"><option>OPTION_PASSED_TO_BUILD_CMD</option></arg>
> > >      </cmdsynopsis>
> > >    </refsynopsisdiv>
> > >    <refsect1>
> > > diff --git a/gbp/scripts/buildpackage_rpm.py b/gbp/scripts/buildpackage_rpm.py
> > > index 1dc1ab2..1cff73e 100644
> > > --- a/gbp/scripts/buildpackage_rpm.py
> > > +++ b/gbp/scripts/buildpackage_rpm.py
> > > @@ -258,10 +258,11 @@ def setup_builder(options, builder_args):
> > >      if options.builder == 'rpmbuild':
> > >          if len(builder_args) == 0:
> > >              builder_args.append('-ba')
> > > -        builder_args.extend([
> > > -            '--define "_topdir %s"' % os.path.abspath(options.export_dir),
> > > -            '--define "_specdir %%_topdir/%s"' % options.export_specdir,
> > > -            '--define "_sourcedir %%_topdir/%s"' % options.export_sourcedir])
> > > +        if not options.use_mock:
> > > +            builder_args.extend([
> > > +                '--define "_topdir %s"' % os.path.abspath(options.export_dir),
> > > +                '--define "_specdir %%_topdir/%s"' % options.export_specdir,
> > > +                '--define "_sourcedir %%_topdir/%s"' % options.export_sourcedir])
> > 
> > It think this would be better done by swapping the setup_mock and
> > setup_builder invocations like (in case mock can run without any of the
> > above defines, can it?):
> > 
> >    # Setup builder opts
> >    if options.use_mock:
> >      setup_mock(options)
> >    setup_builder(options, builder_args)
> > 
> > instead of the current:
> > 
> >    # Setup builder opts
> >    setup_builder(options, builder_args)
> >      if options.use_mock:
> >        setup_mock(options)
> > 
> > What do you think? Could you check that out?
> 
> I don't follow. Under mock those are not set. Mock runs rpmbuild in the
> chroot and looks for the results in the default locations. The order of
> calling those two functions doesn't seem to matter.

setup_mock() sets options.builder in which case the setup_builder() call
becomes a noop. So swapping the calls makes the code more readable and
does aways with another if / else in 1/3 as well (see inline comments).
Cheers,
 -- Guido