Bug#779559: dpkg-source: Add test dependencies to .dsc

[Guillem Jover]

Hey!

On Sun, 2016-05-01 at 11:43:03 -0500, Martin Pitt wrote:
> wow, this took a full year to actually implement, sorry for that. Long
> plain rides are sometimes useful :-)

Bah, a year is nothing! :)

> Guillem Jover [2015-03-10  5:39 +0100]:
> > So given all the above, I'd say:
> > 
> >   Testsuite-Triggers: foo, bar, baz
> > 
> > from the union of all testsuites test depends, minus @ and @builddeps@,
> > without versions, and with alternatives split (i.e. a simple comma
> > separated package list). If the field is present then it overrides the
> > automatically extracted value.
> 
> The attached patch against current git does that now, plus the
> additional "drop binary packages produced by your own source". I'm not
> really familiar with the dpkg code nor Perl, so I'm sure you have a
> ton of simplifications, style nitpicks, and others.

Let's see. :)

> In set_testsuite_triggers_field() I currently do:
> 
> +    return if $fields->{'Testsuite-Triggers'} || $fields->{'XS-Testsuite-Triggers'} ;
> 
> But I'm not sure at which point the Xs- prefix disappears, nor when the new
> field would become official -- is it necessary to check for it here? Or just
> for 'Testsuite-Triggers'?

This depends on whether the field has had usage before it was known to
dpkg-dev (in the form XS-Field), otherwise if it has just been added
now then there's no need to check for the export markers.

If the field has been used in the wild already then we'd use instead:

  return if $fields->{'Testsuite-Triggers'} ||
            $fields->get_custom_field('Testsuite-Triggers');

> Conversely, how do I say that the field should only aperar in the
> .dsc, not in the .changes?

You already did with the entry in Dpkg::Control::FieldsCore!

> This actually behaves correctly, and I
> assume dpkg-genchanges has a whitelist of which fields to include, but
> it'd be nice if you could confirm that.

This is specified with the “allowed” key in the above mentioned change.

> I tested this against the following synthetic d/t/control which I
> think covers all cases:
> 
>     -------- 8< ------------
>     Tests: a
>     Depends: @, pmount
> 
>     Tests: b
>     Depends: gzip,
>       coreutils,
>       @builddeps@,
>       blergh-dev,
> 
>     Tests: c
> 
>     Tests: d
>     Depends: foo (>= 4) | bar (<< 5)
>     -------- 8< ------------
> 
> This gives
> 
>     Testsuite-Triggers: bar, blergh-dev, coreutils, foo, pmount
> 
> in the .dsc: "gzip" got filtered out, all dependencies flattened and
> finally sorted for a predictable/reproducible result.
> 
> I also tested it against the autopkgtest source package, a source
> package with an explicit "XS-Testsuite-Triggers:" in d/control, and a
> package without a test suite.

Ah perfect, although this tells me we should probably eventually move
the testsuite related function(s) into a module so that they can be
properly unit tested.

> dpkg with this patch applied still builds and succeeds its tests
> (although that doesn't say much as AFAICS dpkg-source.pl itself is not
> covered by tests). I installed the built dpkg binaries and re-checked
> dpkg-buildpackage -S on the above source packages.

There are very minimal test in the dpkg-test.git repo.

> Thanks for considering!

Thanks for implementing and testing it!

> diff --git a/scripts/dpkg-source.pl b/scripts/dpkg-source.pl
> index 1cde71c..ca34f2a 100755
> --- a/scripts/dpkg-source.pl
> +++ b/scripts/dpkg-source.pl
> @@ -510,6 +511,56 @@ sub set_testsuite_field
>      $fields->{'Testsuite'} = join ', ', sort keys %testsuite;
>  }
>  
> +# recursively walk Dpkg::Deps tree, flatten AND and OR dependencies, add
> +# package names to the given hash ref
> +sub collect_test_deps
> +{
> +    my $dep = shift;
> +    my $pkghash = shift;
> +    return if $dep->is_empty();
> +    if ($dep->isa('Dpkg::Deps::Simple')) {
> +        $pkghash->{$dep->{package}} = 1;
> +    } else {
> +        foreach my $d ($dep->get_deps()) {
> +            collect_test_deps($d, $pkghash);
> +        }
> +    }
> +}

You can use deps_iterate() from Dpkg::Deps instead.

> +sub set_testsuite_triggers_field
> +{
> +    my $fields = shift;
> +    my @binarypackages = shift;
> +    my $tc_path = "$dir/debian/tests/control";
> +    my %testdeps;
> +
> +    # never overwrite a manually defined field
> +    return if $fields->{'Testsuite-Triggers'} || $fields->{'XS-Testsuite-Triggers'} ;
> +
> +    # autopkgtest is the only test we can parse
> +    return unless -e $tc_path;
> +
> +    # parse Tests: from debian/tests/control

You could use Dpkg::Index here instead which can parse the control
file and store a Dpkg::Control per paragraph. Eventually we might want
to add something like Dpkg::Control::Tests(uite) or something like that.

> +    my $control = Dpkg::Control::HashCore->new(
> +        drop_empty => 1,
> +        allow_duplicate => 1,
> +        name => 'autopkgtest control');
> +    open(my $tc_fh, '<', $tc_path)
> +        or syserr(g_('cannot read %s'), $tc_path);
> +    while($control->parse($tc_fh, $tc_path)) {
> +        # strip out autopkgtest macro deps, dpkg cannot parse them
> +        $control->{'Depends'} =~ s/(^|,)\s*(@|\@builddeps@)([[:space:],]|$)/$1$3/g;

I didn't like this part which seems a bit fragile, and has to be kept
in sync with dependency parsing if something changes in that sense. So
I've added a tests_dep option to deps_parse() for 1.18.7, that will
allow package names with @ in them, then you can filter them out with
the rest.

> +        my $deps = deps_parse($control->{'Depends'}, use_arch => 0);
> +        collect_test_deps($deps, \%testdeps) if $deps;
> +    }
> +
> +    # remove our own binaries
> +    foreach my $p (@binarypackages) {
> +        delete $testdeps{$p};
> +    }
> +    $fields->{'Testsuite-Triggers'} = join(', ', sort keys %testdeps) if %testdeps;

No need for the conditional, the dumper will ignore undef fields.

Thanks,
Guillem