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

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Debian BTS Submit <submit@bugs.debian.org>

Cc: autopkgtest-devel@lists.alioth.debian.org, Adam Conrad <adconrad@debian.org>

Subject: dpkg-source: Add test dependencies to .dsc

Date: Mon, 2 Mar 2015 11:15:22 +0100

Package: dpkg
Version: 1.17.24
User: autopkgtest-devel@lists.alioth.debian.org
Usertags: autopkgtest

Hello,

Adam and I just discussed how to improve DEP-8 autopkgtest regression
detection on uploading packages. Right now we only run tests for
reverse binary dependencies of a package that got uploaded, but we ran
into several cases where also running reverse test dependencies (i. e.
Depends: in debian/tests/control) would have been useful to catch
regressions before landing them [1]. For example, a recent binutils
upload broke the glibc build/test (but libc6 doesn't depend on
binutils), and updating linux from 3.18 to 3.19 broke the legacy
nvidia driver.

Originally I thought we should have a "test control index" similar to
the archive's Packages.gz and Sources.gz. But that is very expensive
to generate, as you would have to unpack thousand(s) of source
packages, extract debian/tests/control, and do that often enough to be
useful. Also, it would require changing DAK, Launchpad, and all the
other tooling around archives.

A simpler idea would be to make dpkg-source -b add a new
XS-Autopkgtest-Depends: header, which aggregates all Depends: lines of
debian/tests/control, possibly filter out '@' (which are a packages'
own binaries, not reverse dependencies) for cleanliness, and put them
into the .dsc that way, so that they appear in Sources.gz. That way
they would always be current.

The obvious downside is that this brings some extra clutter. Right now
we have some 1300 packages with tests, but their number has grown
fast. On the other hand, a lot of packages just use "Depends: @", i.
e. just test-depend on themselves, so for those we don't need to add
anything. Most just have a few extra dependencies, but I've also seen
some which depend on 10 packages. But even if we assume 3000 packages
with actual reverse test dependencies, and one screen line on average
worth of Depends: (both of which are rather big overestimates), the
total impact on the Sources index would be +240 kB uncompressed, or
perhaps 30 kB compressed. Compared to sid's current Sources.gz which
is 34 MB uncompressed and 7.4 MB xz'ed that by and large gets lost in
the noise.

Adam or I will be happy to work on a patch, but I'd like to discuss
this first before we start coding.

Thanks!

Martin

[1] "landing": In Debian that would mean unstable → testing, in Ubuntu
    it would mean devel-proposed → devel.
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Message #12 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: Martin Pitt <mpitt@debian.org>, 779559@bugs.debian.org

Cc: autopkgtest-devel@lists.alioth.debian.org, Adam Conrad <adconrad@debian.org>

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

Date: Wed, 4 Mar 2015 09:57:10 +0100

Hi!

On Mon, 2015-03-02 at 11:15:22 +0100, Martin Pitt wrote:
> Package: dpkg
> Version: 1.17.24
> User: autopkgtest-devel@lists.alioth.debian.org
> Usertags: autopkgtest

> A simpler idea would be to make dpkg-source -b add a new
> XS-Autopkgtest-Depends: header, which aggregates all Depends: lines of
> debian/tests/control, possibly filter out '@' (which are a packages'
> own binaries, not reverse dependencies) for cleanliness, and put them
> into the .dsc that way, so that they appear in Sources.gz. That way
> they would always be current.

Heh, that reminds me a bit of
<http://www.hadrons.org/~guillem/debian/docs/test.proposal>.

In any case, yeah, this sounds fine. I've conflicting thoughts about
the field name, on one hand Testsuite-Depends (to match the existing
field) seems nicer and more neutral, on the other it would take over
that field for a specific testsuite implementation, as the Testsuite
field already supports different values, even at the same time.

But then, the test runners will need to parse the specific test case
dependencies, as in this case @ is omitted, so we might as well just
coalesce any other dependencies in the same field. Hmmm. The problem
with this is that, if in the future we require the information to be
distinct, that's pretty hard to detangle. Merging it, is pretty easy
because it can be done at run-time while source packages get rebuilt.

> The obvious downside is that this brings some extra clutter. Right now
> we have some 1300 packages with tests, but their number has grown
> fast. On the other hand, a lot of packages just use "Depends: @", i.
> e. just test-depend on themselves, so for those we don't need to add
> anything. Most just have a few extra dependencies, but I've also seen
> some which depend on 10 packages. But even if we assume 3000 packages
> with actual reverse test dependencies, and one screen line on average
> worth of Depends: (both of which are rather big overestimates), the
> total impact on the Sources index would be +240 kB uncompressed, or
> perhaps 30 kB compressed. Compared to sid's current Sources.gz which
> is 34 MB uncompressed and 7.4 MB xz'ed that by and large gets lost in
> the noise.

Personally I don't see any problem in an increase in Sources, even of
few MiB if it provides useful information. IMO if you are using Sources
indices, you have the resources to deal with compilation, and the
requirements there can be asymmetric WRT binary package installation.

> Adam or I will be happy to work on a patch, but I'd like to discuss
> this first before we start coding.

Perfect. :)

Thanks,
Guillem

Message #17 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Guillem Jover <guillem@debian.org>, 779559@bugs.debian.org, autopkgtest-devel@lists.alioth.debian.org, Adam Conrad <adconrad@debian.org>

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

Date: Wed, 4 Mar 2015 10:06:42 +0100

Hey Guillem,

Guillem Jover [2015-03-04  9:57 +0100]:
> Heh, that reminds me a bit of
> <http://www.hadrons.org/~guillem/debian/docs/test.proposal>.

Wow, 2006! :-)

> In any case, yeah, this sounds fine. I've conflicting thoughts about
> the field name, on one hand Testsuite-Depends (to match the existing
> field) seems nicer and more neutral, on the other it would take over
> that field for a specific testsuite implementation, as the Testsuite
> field already supports different values, even at the same time.

Indeed XS-Testsuite-Depends: would match Testsuite: better. Other test
implementations could use that as well, though?

> But then, the test runners will need to parse the specific test case
> dependencies, as in this case @ is omitted, so we might as well just
> coalesce any other dependencies in the same field. Hmmm.

Correct, the field is fairly useless for autopkgtest itself (or
similar implementations); not only because of the missing @ or
@builddeps@, but also because it's just the union of all dependencies
of all individual tests. I. e. if you have

  Tests: foo
  Depends: foo1, foo2

  Tests: bar
  Depends: @, bar1

then Testsuite-Depends: will be "foo1, foo2, bar1".

So maybe there is a better field name to express the intended meaning.
We don't want it as a primary specification of test deps (that's still
in debian/tests/control or on another place for different kinds of
package tests), just as an indication to "run the tests for this
package if any of these listed packages changes". So something that
expresses XS-Run-Tests-When-Packages-Change: ..  Perhaps

  XS-Testsuite-Triggers:

?

It's even conceivable that package maintainers want to maintain this
field manually, like "run the tests for the nvidia graphics driver
whenever the kernel changes". But auto-generating it in the described
manner would still be an useful default IMHO.

> The problem with this is that, if in the future we require the
> information to be distinct, that's pretty hard to detangle. Merging
> it, is pretty easy because it can be done at run-time while source
> packages get rebuilt.

You mean distinct per individual test-case (see above)? If you mean
something else, then I don't understand, I'm afraid.

Thanks,

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Message #22 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: Martin Pitt <mpitt@debian.org>, 779559@bugs.debian.org

Cc: autopkgtest-devel@lists.alioth.debian.org, Adam Conrad <adconrad@debian.org>

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

Date: Wed, 4 Mar 2015 10:42:15 +0100

Hi!

On Wed, 2015-03-04 at 10:06:42 +0100, Martin Pitt wrote:
> Guillem Jover [2015-03-04  9:57 +0100]:
> > But then, the test runners will need to parse the specific test case
> > dependencies, as in this case @ is omitted, so we might as well just
> > coalesce any other dependencies in the same field. Hmmm.
> 
> Correct, the field is fairly useless for autopkgtest itself (or
> similar implementations); not only because of the missing @ or
> @builddeps@, but also because it's just the union of all dependencies
> of all individual tests. I. e. if you have
> 
>   Tests: foo
>   Depends: foo1, foo2
> 
>   Tests: bar
>   Depends: @, bar1
> 
> then Testsuite-Depends: will be "foo1, foo2, bar1".
> 
> So maybe there is a better field name to express the intended meaning.
> We don't want it as a primary specification of test deps (that's still
> in debian/tests/control or on another place for different kinds of
> package tests), just as an indication to "run the tests for this
> package if any of these listed packages changes". So something that
> expresses XS-Run-Tests-When-Packages-Change: ..  Perhaps
> 
>   XS-Testsuite-Triggers:
> 
> ?

Ah, indeed, making it clear that this is strictly not for "dependency"
purposes, seems better. It just crossed my mind that it might make sense
to strip the version information from those dependencies, and only list
package names, but given that those dependencies can have alternatives
«foo (>= 1.0) | bar», they should probably be preserved?

> It's even conceivable that package maintainers want to maintain this
> field manually, like "run the tests for the nvidia graphics driver
> whenever the kernel changes". But auto-generating it in the described
> manner would still be an useful default IMHO.

Right. So then we either need to replace it with the information
from the testsuite, or extend it in a similar way as how the Testsuite
field is currently being handled. I'd tend to think the latter.

> > The problem with this is that, if in the future we require the
> > information to be distinct, that's pretty hard to detangle. Merging
> > it, is pretty easy because it can be done at run-time while source
> > packages get rebuilt.
> 
> You mean distinct per individual test-case (see above)? If you mean
> something else, then I don't understand, I'm afraid.

When I said coalesce any other dependencies I was thinking about the
following situation. Say we have a package with an "autopkgtest"
testsuite with dependencies «foo (>= 1.0), bar», and another "newchecks"
testsuite with dependencies «quux, frob, bar». And then the
Testsuite-Triggers ends up being «foo (>= 1.0), bar, quux, frob».

If I'm only interested in running either autopkgtest or newchecks for
the whole archive, I might get triggered for packages that I'm not
interested in, which would be wasteful. And to get accurate
information we are back to having to unpack the source packages. This
might get worse the more testsuites we get.

(But in any case this is all very hypothetical, because we might
possibly never get a new testsuite alternative, but I like to think
ahead of time, and consider all these "corner-cases" before they
possibly bite us.)

Thanks,
Guillem

Message #27 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Guillem Jover <guillem@debian.org>, 779559@bugs.debian.org, autopkgtest-devel@lists.alioth.debian.org, Adam Conrad <adconrad@debian.org>

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

Date: Wed, 4 Mar 2015 11:00:08 +0100

Hello Guillem,

Guillem Jover [2015-03-04 10:42 +0100]:
> Ah, indeed, making it clear that this is strictly not for "dependency"
> purposes, seems better. It just crossed my mind that it might make sense
> to strip the version information from those dependencies, and only list
> package names, but given that those dependencies can have alternatives
> «foo (>= 1.0) | bar», they should probably be preserved?

Stripping versions sounds good to me. For alternatives I'd either
split them out as separate triggers (if we want to trigger tests on
alternative changes too), or just ignore them completely (if we don't
want the extra test runs).

> > It's even conceivable that package maintainers want to maintain this
> > field manually, like "run the tests for the nvidia graphics driver
> > whenever the kernel changes". But auto-generating it in the described
> > manner would still be an useful default IMHO.
> 
> Right. So then we either need to replace it with the information
> from the testsuite, or extend it in a similar way as how the Testsuite
> field is currently being handled. I'd tend to think the latter.

I'd say, if debian/control already has that field, dpkg-source
shouldn't touch it at all. That way you can avoid unnecessary reverse
dependency runs if the maintainer decides so?

> When I said coalesce any other dependencies I was thinking about the
> following situation. Say we have a package with an "autopkgtest"
> testsuite with dependencies «foo (>= 1.0), bar», and another "newchecks"
> testsuite with dependencies «quux, frob, bar». And then the
> Testsuite-Triggers ends up being «foo (>= 1.0), bar, quux, frob».
> 
> If I'm only interested in running either autopkgtest or newchecks for
> the whole archive, I might get triggered for packages that I'm not
> interested in, which would be wasteful. And to get accurate
> information we are back to having to unpack the source packages. This
> might get worse the more testsuites we get.

OK, agreed. So putting that together, how about:

  XS-Autopkgtest-Triggers: foo, bar, baz

which gets the union of all test depends, minus @, without versions,
and with alternatives either expanded or ignored?

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Message #32 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Adam Conrad <adconrad@debian.org>

To: Guillem Jover <guillem@debian.org>, Martin Pitt <mpitt@debian.org>, 779559@bugs.debian.org, autopkgtest-devel@lists.alioth.debian.org

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

Date: Thu, 5 Mar 2015 06:34:31 -0700

On Wed, Mar 04, 2015 at 10:42:15AM +0100, Guillem Jover wrote:
> 
> When I said coalesce any other dependencies I was thinking about the
> following situation. Say we have a package with an "autopkgtest"
> testsuite with dependencies «foo (>= 1.0), bar», and another "newchecks"
> testsuite with dependencies «quux, frob, bar». And then the
> Testsuite-Triggers ends up being «foo (>= 1.0), bar, quux, frob».

I'm not convinced this is actuall a problem.  What we're trying to
approximate here with auto-filling Testsuite-Triggers (rather than
someone specifying it manually in debian/control) is a set of packages
that we feel it would be reasonable to force a retest for.

Sure, maybe it wastes a few cycles if TestA doesn't actually depend
on or use DepB, but we run it when DepB changes anyway, but I'm not
sure optimising for that is worth having ${test}-Triggers instead of
just one concatenated field.

I suspect my opinion here is partially colored by issues of prettiness,
and I should probably not care about that, so if everyone else thinks
the shed should be red, I can go paint it that colour, and just pretend
it's blue in my mind.

... Adam

Message #37 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: Martin Pitt <mpitt@debian.org>, 779559@bugs.debian.org, Adam Conrad <adconrad@debian.org>

Cc: autopkgtest-devel@lists.alioth.debian.org

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

Date: Tue, 10 Mar 2015 05:39:38 +0100

Hi!

On Wed, 2015-03-04 at 11:00:08 +0100, Martin Pitt wrote:
> Guillem Jover [2015-03-04 10:42 +0100]:
> > Ah, indeed, making it clear that this is strictly not for "dependency"
> > purposes, seems better. It just crossed my mind that it might make sense
> > to strip the version information from those dependencies, and only list
> > package names, but given that those dependencies can have alternatives
> > «foo (>= 1.0) | bar», they should probably be preserved?
> 
> Stripping versions sounds good to me. For alternatives I'd either
> split them out as separate triggers (if we want to trigger tests on
> alternative changes too), or just ignore them completely (if we don't
> want the extra test runs).

Ok, if we just want a list of packages that should trigger test reruns
then yes, splitting alternatives and stripping versions would be best,
because having thought about it, I think it does make sense to trigger
on changes for the alternatives too (and ideally the test runner would
then use, if satisfiable, the alternatives that have changed!).

> > > It's even conceivable that package maintainers want to maintain this
> > > field manually, like "run the tests for the nvidia graphics driver
> > > whenever the kernel changes". But auto-generating it in the described
> > > manner would still be an useful default IMHO.
> > 
> > Right. So then we either need to replace it with the information
> > from the testsuite, or extend it in a similar way as how the Testsuite
> > field is currently being handled. I'd tend to think the latter.
> 
> I'd say, if debian/control already has that field, dpkg-source
> shouldn't touch it at all. That way you can avoid unnecessary reverse
> dependency runs if the maintainer decides so?

Ok, this gives more control, as it allows to reset the field. If the
maintainer wants to extend it, those can always be added to some of
the specific test dependencies instead.

On Thu, 2015-03-05 at 06:34:31 -0700, Adam Conrad wrote:
> On Wed, Mar 04, 2015 at 10:42:15AM +0100, Guillem Jover wrote:
> > When I said coalesce any other dependencies I was thinking about the
> > following situation. Say we have a package with an "autopkgtest"
> > testsuite with dependencies «foo (>= 1.0), bar», and another "newchecks"
> > testsuite with dependencies «quux, frob, bar». And then the
> > Testsuite-Triggers ends up being «foo (>= 1.0), bar, quux, frob».
> 
> I'm not convinced this is actuall a problem.  What we're trying to
> approximate here with auto-filling Testsuite-Triggers (rather than
> someone specifying it manually in debian/control) is a set of packages
> that we feel it would be reasonable to force a retest for.
> 
> Sure, maybe it wastes a few cycles if TestA doesn't actually depend
> on or use DepB, but we run it when DepB changes anyway, but I'm not
> sure optimising for that is worth having ${test}-Triggers instead of
> just one concatenated field.

Ok, that makes sense. Also considering it an approximation doesn't
disallow the test runner to skip the package by checking the more
fine-grained dependencies from the test metadata, even for different
testsuite implementations.

On Wed, 2015-03-04 at 11:00:08 +0100, Martin Pitt wrote:
> On Wed, Mar 04, 2015 at 10:42:15AM +0100, Guillem Jover wrote:
> > If I'm only interested in running either autopkgtest or newchecks for
> > the whole archive, I might get triggered for packages that I'm not
> > interested in, which would be wasteful. And to get accurate
> > information we are back to having to unpack the source packages. This
> > might get worse the more testsuites we get.
> 
> OK, agreed. So putting that together, how about:
> 
>   XS-Autopkgtest-Triggers: foo, bar, baz
> 
> which gets the union of all test depends, minus @, without versions,
> and with alternatives either expanded or ignored?

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.

Does this seem fine?

Thanks,
Guillem

Message #42 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Guillem Jover <guillem@debian.org>, 779559@bugs.debian.org, Adam Conrad <adconrad@debian.org>, autopkgtest-devel@lists.alioth.debian.org

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

Date: Tue, 10 Mar 2015 06:56:11 +0100

Hey Guillem,

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.
> 
> Does this seem fine?

+1 for me, that's simple and clean.

Thanks!

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

Message #47 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Guillem Jover <guillem@debian.org>, 779559@bugs.debian.org, Adam Conrad <adconrad@debian.org>, autopkgtest-devel@lists.alioth.debian.org

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

Date: Sun, 1 May 2016 11:43:03 -0500

[Message part 1 (text/plain, inline)]

Hello Guillem, Adam, all,

wow, this took a full year to actually implement, sorry for that. Long
plain rides are sometimes useful :-)

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.

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

Conversely, how do I say that the field should only aperar in the
.dsc, not in the .changes? 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.

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.

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.

Thanks for considering!

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[0001-dpkg-source-Generate-Testsuite-Triggers-field-from-t.patch (text/x-diff, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #52 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: Martin Pitt <mpitt@debian.org>

Cc: 779559@bugs.debian.org, Adam Conrad <adconrad@debian.org>, autopkgtest-devel@lists.alioth.debian.org

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

Date: Mon, 9 May 2016 03:34:51 +0200

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

Message #57 received at 779559@bugs.debian.org (full text, mbox, reply):

From: Martin Pitt <mpitt@debian.org>

To: Guillem Jover <guillem@debian.org>, 779559@bugs.debian.org, Adam Conrad <adconrad@debian.org>, autopkgtest-devel@lists.alioth.debian.org

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

Date: Fri, 13 May 2016 14:04:46 +0200

[Message part 1 (text/plain, inline)]

Hey Guillem,

Guillem Jover [2016-05-09  3:34 +0200]:
> > 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.

This has never been used, so I dropped the Xs-* thing.

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

Done.

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

I did this, but kept it as a separate patch. It does not make the
logic significantly easier, and requires gems like

  foreach my $rec (values %{$index->{items}}) {

(Dpkg::Index does not have any documented way of iterating over all
fields). It also needs a non-trivial get_key_func() -- while I don't
actually use the keys, not having a key_func causes nasty perl
warnings.

If you prefer this, rebase -i 'f'ixup this into the first patch.

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

Very nice! Did that now.

> > +    $fields->{'Testsuite-Triggers'} = join(', ', sort keys %testdeps) if %testdeps;
> 
> No need for the conditional, the dumper will ignore undef fields.

It'll actually be an empty string, but indeed the dumper ignores that
too, so I dropped it.

Updated patches attached, rebased against master from today.  This
looks a lot more succinct and elegant now, thanks for these hints!

As before, I tested various d/t/control scenarios, with/without manual
T-T: field in d/control, package build, etc.

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)

[0001-dpkg-source-Generate-Testsuite-Triggers-field-from-t.patch (text/x-diff, attachment)]

[0002-dpkg-source.pl-Use-Dpkg-Index-to-parse-debian-tests-.patch (text/x-diff, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #60 received at 779559-submitter@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: 779559-submitter@bugs.debian.org

Subject: Bug#779559 in package dpkg marked as pending

Date: Sun, 03 Jul 2016 21:29:20 +0000

Control: tag 779559 pending

Hi!

Bug #779559 in package dpkg reported by you has been fixed in
the dpkg/dpkg.git Git repository. You can see the changelog below, and
you can check the diff of the fix at:

    https://anonscm.debian.org/cgit/dpkg/dpkg.git/diff/?id=90324cf

---
commit 90324cfa942ba23d5d44b28b1087fbd510340502
Author: Guillem Jover <guillem@debian.org>
Date:   Fri May 13 17:26:01 2016 +0200

    dpkg-source: Generate Testsuite-Triggers field from test dependencies
    
    Sometimes autopkgtests regress due to change in a package which is only
    a test dependency (Depends: in debian/tests/control), not a build or
    binary one. It is useful to trigger a test if such a test dependency
    changes.
    
    Record the union of all test dependency packages in a new
    Testsuite-Triggers field in the .dsc, so that they will be recorded in
    the Sources package index. Ignore versions and flatten OR dependencies
    as they are not interesting for determining reverse test dependencies
    and should not be (ab)used for replacing debian/tests/control parsing.
    
    Closes: #779559
    LP: #1491145
    Based-on-patch-by: Martin Pitt <martin.pitt@ubuntu.com>
    Signed-off-by: Guillem Jover <guillem@debian.org>

diff --git a/debian/changelog b/debian/changelog
index 1d1e334..63de0d3 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -9,6 +9,9 @@ dpkg (1.18.8) UNRELEASED; urgency=medium
     under either the dpkg system or user configuration directories.
     Closes: #539692, #765494
   * Check that debian/tests/control is a regular file before parsing it.
+  * Generate Testsuite-Triggers field from test dependencies in dpkg-source
+    into .dsc files. Based on a patch by Martin Pitt <martin.pitt@ubuntu.com>.
+    Closes: #779559
   * Perl modules:
     - Use warnings::warnif() instead of carp() for deprecated warnings.
     - Add new format_range() method and deprecate dpkg() and rfc822() methods

Message #67 received at 779559-close@bugs.debian.org (full text, mbox, reply):

From: Guillem Jover <guillem@debian.org>

To: 779559-close@bugs.debian.org

Subject: Bug#779559: fixed in dpkg 1.18.8

Date: Sun, 03 Jul 2016 22:41:47 +0000

Source: dpkg
Source-Version: 1.18.8

We believe that the bug you reported is fixed in the latest version of
dpkg, which is due to be installed in the Debian FTP archive.

A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 779559@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Guillem Jover <guillem@debian.org> (supplier of updated dpkg package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@ftp-master.debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Format: 1.8
Date: Sun, 03 Jul 2016 19:01:56 +0200
Source: dpkg
Binary: libdpkg-dev dpkg dpkg-dev libdpkg-perl dselect
Architecture: source
Version: 1.18.8
Distribution: unstable
Urgency: medium
Maintainer: Dpkg Developers <debian-dpkg@lists.debian.org>
Changed-By: Guillem Jover <guillem@debian.org>
Description:
 dpkg       - Debian package management system
 dpkg-dev   - Debian package development tools
 dselect    - Debian package management front-end
 libdpkg-dev - Debian package management static library
 libdpkg-perl - Dpkg perl modules
Closes: 539692 745366 759886 759999 765494 779559 823167 823805 823877 824515 824542 824572 824873 824938 826161 826334 827265 827628 827633 828146
Changes:
 dpkg (1.18.8) unstable; urgency=medium
 .
   [ Guillem Jover ]
   * Do not disable PIE buildflags on */kFreeBSD anymore. Closes: #823877
     Thanks to Steven Chamberlain <steven@pyro.eu.org>.
   * Add new long options in dpkg-buildpackage for any short option that is
     a useful configurable candidate.
   * Add configuration file support to dpkg-buildpackage, as buildpackage.conf
     under either the dpkg system or user configuration directories.
     Closes: #539692, #765494
   * Check that debian/tests/control is a regular file before parsing it.
   * Generate Testsuite-Triggers field from test dependencies in dpkg-source
     into .dsc files. Based on a patch by Martin Pitt <martin.pitt@ubuntu.com>.
     Closes: #779559
   * Add new dpkg-source --no-overwrite-dir extraction option. Closes: #826334
   * Fix number of entries computation returned by sysctl() on */kFreeBSD in
     start-stop-daemon.
   * Set return buffer length for sysctl(2) calls on */kFreeBSD in
     start-stop-daemon.
   * Abstract ar archive handling behind a new struct dpkg_ar and functions.
   * On dpkg --force-chrootless only set changedir to instdir if defined.
     Thanks to Niall Walsh <niallwalsh@celtux.org>. Closes: #824542
   * Set primary group to 0 in dpkg when running as root.
     Reported by Stuart Prescott <stuart@debian.org>.
   * Activate file triggers for conffiles on purge, which has never happened
     before. Before dpkg 1.17.0, conffiles were triggered on removal, which
     was obviously wrong. Reported by Helmut Grohne <helmut@subdivi.de>.
   * Fix strtol() errno check when parsing the COLUMNS envvar in dpkg-query.
     Thanks to Sven Joachim <svenjoac@gmx.de>. Closes: #827265
   * Use new GNU tar --clamp-mtime option in dpkg-deb to make sure no file in
     binary packages has an mtime later than the given time. Closes: #759886
   * Use the same timestamp for the ar container as for tarball mtime clamping
     in dpkg-deb.
   * Set ar timestamp and tar mtime clamping to SOURCE_DATE_EPOCH if defined
     in dpkg-deb. Base on a patch by Jérémy Bobbio <lunar@debian.org>.
   * Preset build timestamp to latest changelog entry in dpkg-buildpackage,
     by setting SOURCE_DATE_EPOCH environment variable if it is not already
     defined. Based on a patch by Jérémy Bobbio <lunar@debian.org>.
     Closes: #759999
   * Do not use the debian/rules build target fallback when building both
     architecture independent and dependent packages in dpkg-buipdpackage.
   * Use architecture «all» as part of the .changes filename when building
     architecture independent binaries and no architecture dependent binaries.
     Closes: #826161
   * Do not emit warnings from dpkg-genchanges for automatic debug symbol
     packages that are not found in debian/control.
   * Export SOURCE_DATE_EPOCH from pkg-info.mk makefile snippet.
     Closes: #824572
   * Architecture support:
     - Add TILE-Gx support to cputable. Closes: #823167
       Thanks to Helmut Grohne <helmut@subdivi.de>.
   * Perl modules:
     - Use warnings::warnif() instead of carp() for deprecated warnings.
     - Add new format_range() method and deprecate dpkg() and rfc822() methods
       in Dpkg::Changelog.
     - Replace changelog program parsers with perl modules.
     - Add a getter for the Time::Piece object in Dpkg::Changelog.
     - Add new Timestamp field to Dpkg::Changelog output, which ends up on
       dpkg-parsechangelog's output.
     - Validate source version in set_version_substvars()'s Dpkg::Substvars
       method.
     - Revert "Dpkg::Conf: Switch implementation to be hash based", as this
       change broke backwards compatibility in multiple ways. The format_argv
       option was set by default, the order was not preserved, which was
       important for dpkg.cfg files, and duplicate option names stopped being
       supported. Add regression tests to avoid similar changes in the future.
       Closes: #824938
     - Add support for system and user config loading in Dpkg::Conf.
     - Add support for autopkgtest control files, with new CTRL_TESTS control
       type, new recognized fields to Dpkg::Control::Fields, and new modules
       Dpkg::Control::Tests and Dpkg::Control::Tests::Entry. Also update
       Dpkg::Index to support these.
     - Fix Dpkg::Deps so that architecture qualifiers only imply one another
       if they are the same. Closes: #745366, #827628
     - Add support for new environment variable DEB_BUILD_PATH to be able to
       control the path in the fixdebugpath feature in Dpkg::Vendor::Debian.
     - Preserve order when prepending shared library paths in Dpkg::Shlibs.
       This fixes the order of paths passed via dpkg-shlibdeps -l option.
       Closes: #823805
     - Check whether dependency restrictions are implied in Dpkg::Deps::Simple.
       Thanks to Ben Hutchings <ben@decadent.org.uk>. Closes: #827633
     - Disable upstream tar signature when building format 1.0 source packages
       in Dpkg::Source::Package::V1, as the current stable dpkg series do not
       support extracting them.
     - Preset Last-Update field in patch header template with current time in
       Dpkg::Source::Package::V2. Thanks to Daniel Shahaf <danielsh@apache.org>.
       Closes: #828146
   * Packaging:
     - Disable libmd usage in Debian and derivatives for now.
   * Build system:
     - Stop allowing to set deprecated bzip2 compressor as dpkg-deb default.
     - Use libmd automatically if available.
     - Uniformize library build options, from --with-zlib to --with-libz,
       --with-bz2 to --with-libbz2 and --with-selinux to --with-libselinux.
   * Test suite:
     - Bump perlcritic ValuesAndExpressions::RequireNumberSeparators minimum
       to 99999.
     - Add new pod-spell unit test.
     - Refactor common unit test checks for needed things into Test::Dpkg.
     - Accept perl's Lancaster Consensus AUTHOR_TESTING variable.
     - Add new minimum perl version unit test.
     - Add new synopsis unit test.
     - Add unit tests for dependency simplification with build profiles.
   * Documentation:
     - Improve dpkg-buildpackage(1) on environment expectations.
     - Clarify the format of the db:Status-Abbrev virtual field in
       dpkg-query(1). Closes: #824515
     - Document the tar entry size limitation for deb(5) format.
     - Document interaction between PIE and libraries in dpkg-buildflags(1).
       Based on text by Christian Seiler <christian@iwakd.de>.
     - Merge ENVIRONMENT sections in dpkg-buildflags(1).
     - Document various long options in dpkg-source --help output.
     - Move dpkg-source -q option from “Build options” to “General options”
       section in --help output.
     - Clarify shared library search order in dpkg-shlibdeps(1).
     - Remove most remaining AUTHOR sections from man an POD, as they are
       strongly discouraged, for being redundant, tending to get out-of-sync,
       and their format being inconsistent. In addition most got already
       removed in the past for the man pages.
     - Mark perlcritic as an optional author test dependency in the README.
     - Fix example code in Dpkg::Compression::FileHandle SYNOPSIS.
 .
   [ Updated programs translations ]
   * German (Sven Joachim).
   * Simplified Chinese (Zhou Mo). Closes: #824873
 .
   [ Updated scripts translations ]
   * German (Helge Kreutzmann).
 .
   [ Updated manpages translations ]
   * German (Helge Kreutzmann).
Checksums-Sha1:
 1976784ae227d550c4741ec17f4d747cf980cb48 2026 dpkg_1.18.8.dsc
 ecc3973037b85e6c7bc89928d7aa83ce6f13ec23 4633168 dpkg_1.18.8.tar.xz
Checksums-Sha256:
 965e50539d337897a74dd77ea9e66d2baec917ec6e089bc442320e7706abee5f 2026 dpkg_1.18.8.dsc
 0b5562578a46d5c54fe77c262cd0a13ad13f4ff4bda8ccc285757ad37a3f65b7 4633168 dpkg_1.18.8.tar.xz
Files:
 14737307588416c1747e78fbb69f379f 2026 admin required dpkg_1.18.8.dsc
 4729a5a9cd3755c0adf37e95e22f482e 4633168 admin required dpkg_1.18.8.tar.xz

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCgAGBQJXeXQCAAoJELlyvz6krlejIw8P/3dpr/kyAcHZtTyjiBIXeJSE
aZK4U8kR1W5R4/CydUQdxd6kanbvJxx+9ceDlViev4ndtmTz3QvQdejN9Knjt8OU
CZ4yPJhP5HHWE0kzI+sFhMeC6eK+ck2OD1U4Xthtkq/sKSEmowN7mJ64uvvScF5w
XXreI3qWCywl6zdiXyu+M+O6PlRoTjSBXO3F0XKPNub7X9j1wNyfAFbzd3UGhEAc
sBh5GMLIGjY+clmXjvbngNJhWszuV2TFcbIDDDZAi4axkE62hI99VQZfKWm+96ck
fXpimwavrTc2fje+9lLMupQ7FIFWCHTUV/JKdDA4qc4S8THhC7qIoGsEl/Hhg+Gr
8LeDwaMuuOliubK3vCP1Ksk5B+pNM8hgJyQ2Cc88a9FZttSeTSG1EtjN3Wh1g1MS
J1Ul3ZRJCzPwrgBGZXN7i9GGNxlh1JXGR/Bn4NZW1fLtOyfK16AKIC7erkQWAgan
HknHiuwpDljb6SxDscFeXIsl14JTS13iA3SVSLdb7CrY/KCfrQwq6XbItCfXb2EN
Ipg6aC5gIRJKfpq6FzJUztzBmkWFCeG5GqBZeKW0f5zct5W3tPWu7kazxjx0tg5J
D9hPFFFuxxdPPq5fbweA1lZuV3saPLcFAfuUB7adoN1urOL5QUoZysdSPUVCZGoM
mRF6sfICJS+WUQWoAibk
=jAm3
-----END PGP SIGNATURE-----

Send a report that this bug log contains spam.