Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Control: tags -1 - moreinfo Control: clone -1 -2 Control: reassign -1 autopkgtest Control: retitle -1 autopkgtest: smarter autodep8 i.c.m. d/t/control Control: reassign -2 autodep8 Control: retitle -2 autodep8: append d/t/control if it exists Hi Guillem, Thank you for carefully thinking about this. On 06-09-18 00:20, Guillem Jover wrote: > I guess, the logic proposed above could be changed a bit to support > something closer to the current logic, and not introduce such a > global semantic change, so perhaps: > > * change autopkgtest to check the Testsuite field in debian/control: > - if it contains say an autopkgtest-autogen value then call autodep8, > even if debian/tests/control exists. autodep8 has already fieldnames like autopkgtest-pkg-python. I think those suffice. > - otherwise call autodep8 only if debian/tests/control does not > exist. > * change autodep8 to always cat debian/tests/control if present. > * then autodep8 would cat the autogenerated tests like now based on > the Testsuite field, and for backwards compat also the > control.autodep8 file if present. I think this looks sane indeed. Let me try to implement it. > I'm just still not convinced this should be handled in dpkg. :) Unless I get back at you, I think I agree. Paul signature.asc Description: OpenPGP digital signature
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Hi! On Wed, 2018-08-01 at 15:58:29 +0200, Paul Gevers wrote: > On 01-08-18 04:44, Guillem Jover wrote: > > On Mon, 2018-07-30 at 20:03:07 +0200, Paul Gevers wrote: > >> autopkgtest (the binary that is _an_ > >> implementation of the spec) only calls autodep8 when it doesn't find the > >> control file as a fall back. autopkgtest doesn't know why it is called, > >> it doesn't know about the Testsuite field, nor does anything in the CI > >> framework except for the one that requests the test. > > > > Right, but it seems autodep8 does check, and uses the Testsuite field > > to decide whether some of the precooked checks should be used. > > Yes, but it may be absent: autodep8 also works on packages that haven't > set any Testsuite field in debian/control. It tries to figure out if the > binary packages are of some type and cooks up the control stanza for > those. E.g. if a source or binary package starts with python-, it gives > us the python2.7 version, if you run autopkgtest (with auto_control > enabled) on them. This has been very useful on the ci.d.n infrastructure > to have large classes of packages tested. This isn't used for > influencing migration though. Sure. > > Wouldn't simply changing the logic to something like the following > > simplified stuff, do the trick? > > > > * change autopkgtest to always call autodep8 if auto_control is > > enabled. > > * change autodep8 to always cat debian/tests/control if present. > > * then autodep8 would cat the autogenerated tests like now based on > > the Testsuite field, and for backwards compat also the > > control.autodep8 file if present. > > What is currently missing is a way for the package maintainer to say: > DON'T add autodep8 stanza to my package (as it fails). Currently that is > implemented by having a file called debian/tests/control. So if we want > to go this route, we either need time to have all packages that need > autodep8 off to be able to migrate to a still to be defined way of > saying so (e.g. by explicitly adding "Testsuite: autopkgtest" to > d/control, which may break a lot of packages when autodep8 adds support > for another class) or we can have autopgktest behave differently for > different ways of calling the test, e.g. when run on .dsc files compared > to when run from an unbuilt source-tree. I don't like to go the route of > different results. The problems I see are: * This feels like an internal implementation detail that would get leaked upwards in the stack, so a layer violation. * The autodep8 stuff is optionally invoked so the information generated might not always be relevant, but the control.autodep8 file looks like it's something that one would not want to make dependant on autodep8 being run or not. * The auto generated stanzas are output into a temporary file so these would not be taken into account either, which might be the desired intention, but it feels weird. I guess, the logic proposed above could be changed a bit to support something closer to the current logic, and not introduce such a global semantic change, so perhaps: * change autopkgtest to check the Testsuite field in debian/control: - if it contains say an autopkgtest-autogen value then call autodep8, even if debian/tests/control exists. - otherwise call autodep8 only if debian/tests/control does not exist. * change autodep8 to always cat debian/tests/control if present. * then autodep8 would cat the autogenerated tests like now based on the Testsuite field, and for backwards compat also the control.autodep8 file if present. So this seems fully backwards compatible, and would make things more clear IMO. It would also make those control.autodep8 files non-optional based on autodep8 running or not. And the transition would be smooth. Any current package shipping a control.autodep8 would just need to rename it to control and add the “Testsuite: autopkgtest-autogen” field. Any package that does not have such value in the field and contains a debian/tests/control would not get autodep8 executed so would automatically opt-out like now. On Sun, 2018-08-19 at 21:38:26 +0200, Paul Gevers wrote: > This bug is still marked as moreinfo, are you still waiting for more > input from me? I'm just still not convinced this should be handled in dpkg. :) Thanks, Guillem
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Please consider this a polite ping. On 19-08-18 21:38, Paul Gevers wrote: > Hi Guillem, > > This bug is still marked as moreinfo, are you still waiting for more > input from me? > > Paul Paul signature.asc Description: OpenPGP digital signature
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Hi Guillem, This bug is still marked as moreinfo, are you still waiting for more input from me? Paul signature.asc Description: OpenPGP digital signature
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Hi Guillem, On 01-08-18 04:44, Guillem Jover wrote: > [ Please excuse some of the probably obvious questions, I probably > just do not have a clear picture of how the pieces fit together. ] Thanks for being critical. Some of these pieces are also looked upon by me for the first time. > On Mon, 2018-07-30 at 20:03:07 +0200, Paul Gevers wrote: >> On 30-07-18 05:42, Guillem Jover wrote: It would be great if dpkg-source would also parse debian/tests/control.autodep8 in the same way as it does debian/tests/control, except I guess it should not set the Testsuite field (as dpkg-source doesn't know which type of autopkgtest this is). >>> >>> Hmm, why did we get this new file at all? >> >> It's more than two years old. It was added in response to bug 823931. > > Would you be open to revisit that decision? Sure, if we find a good alternative. > The current interface > feels a bit like a wart to me. It didn't look like there were many > packages involved, when I checked the other day. I didn't check. https://codesearch.debian.net/search?q=Test+path%3Adebian%2Ftests%2Fcontrol.autodep8 gives 49 packages. >>> Why can't we use >>> debian/tests/control instead? The Testsuite field would be used to >>> specify that we still want autodep8 to be run, no? >> >> Looking at the bug, I can only assume it just wasn't considered to >> change autopkgtest, e.g. IIAC the maintainer of autodep8 wasn't a >> developer of autopkgtest at the time. > > Well, autodep8 seems to be called from autopkgtest, so I'm not sure I > understand that reason? :) Well, it would need changes in autopkgtest (on top of changes to autodep8). So there was the "out of my control" argument I guess. But as said, I am guessing. Anyways, I don't think it is relevant for the discussion. >> Also, the current implementation works without the spec needing to >> mention the debian/control file. > > Isn't it implicitly mentioned in the “Source package header” section? Right, missed that (I was doing it from memory, not looking up the file). But the ironic thing is that this paragraph is actually irrelevant for autopkgtest itself. autopkgtest doesn't care at all. So only autodep8 is using the Testsuite field, and then only from the debian/control file (not from the .dsc file). The d/contol file only needs to mention it when maintainers opt-in for autodep8 >> autopkgtest (the binary that is _an_ >> implementation of the spec) only calls autodep8 when it doesn't find the >> control file as a fall back. autopkgtest doesn't know why it is called, >> it doesn't know about the Testsuite field, nor does anything in the CI >> framework except for the one that requests the test. > > Right, but it seems autodep8 does check, and uses the Testsuite field > to decide whether some of the precooked checks should be used. Yes, but it may be absent: autodep8 also works on packages that haven't set any Testsuite field in debian/control. It tries to figure out if the binary packages are of some type and cooks up the control stanza for those. E.g. if a source or binary package starts with python-, it gives us the python2.7 version, if you run autopkgtest (with auto_control enabled) on them. This has been very useful on the ci.d.n infrastructure to have large classes of packages tested. This isn't used for influencing migration though. > Wouldn't simply changing the logic to something like the following > simplified stuff, do the trick? > > * change autopkgtest to always call autodep8 if auto_control is > enabled. > * change autodep8 to always cat debian/tests/control if present. > * then autodep8 would cat the autogenerated tests like now based on > the Testsuite field, and for backwards compat also the > control.autodep8 file if present. What is currently missing is a way for the package maintainer to say: DON'T add autodep8 stanza to my package (as it fails). Currently that is implemented by having a file called debian/tests/control. So if we want to go this route, we either need time to have all packages that need autodep8 off to be able to migrate to a still to be defined way of saying so (e.g. by explicitly adding "Testsuite: autopkgtest" to d/control, which may break a lot of packages when autodep8 adds support for another class) or we can have autopgktest behave differently for different ways of calling the test, e.g. when run on .dsc files compared to when run from an unbuilt source-tree. I don't like to go the route of different results. >> If we would want to add support for this adding autodep8 on top of an >> existing debian/tests/control file, apart from adapting autopkgtest with >> a new option, also multiple interface would need to be adapted to pass >> on this info: >> britney <-> debci <-> autopkgtest > > Why is that, what information do these require? But rethinking this now it doesn't make sense anymore. The same argument applies: autopkgtest would behave different for different ways of
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Hi! [ Please excuse some of the probably obvious questions, I probably just do not have a clear picture of how the pieces fit together. ] On Mon, 2018-07-30 at 20:03:07 +0200, Paul Gevers wrote: > On 30-07-18 05:42, Guillem Jover wrote: > >> It would be great if dpkg-source would also parse > >> debian/tests/control.autodep8 > >> in the same way as it does debian/tests/control, except I guess it should > >> not > >> set the Testsuite field (as dpkg-source doesn't know which type of > >> autopkgtest > >> this is). > > > > Hmm, why did we get this new file at all? > > It's more than two years old. It was added in response to bug 823931. Would you be open to revisit that decision? The current interface feels a bit like a wart to me. It didn't look like there were many packages involved, when I checked the other day. > > Why can't we use > > debian/tests/control instead? The Testsuite field would be used to > > specify that we still want autodep8 to be run, no? > > Looking at the bug, I can only assume it just wasn't considered to > change autopkgtest, e.g. IIAC the maintainer of autodep8 wasn't a > developer of autopkgtest at the time. Well, autodep8 seems to be called from autopkgtest, so I'm not sure I understand that reason? :) > Also, the current implementation works without the spec needing to > mention the debian/control file. Isn't it implicitly mentioned in the “Source package header” section? > autopkgtest (the binary that is _an_ > implementation of the spec) only calls autodep8 when it doesn't find the > control file as a fall back. autopkgtest doesn't know why it is called, > it doesn't know about the Testsuite field, nor does anything in the CI > framework except for the one that requests the test. Right, but it seems autodep8 does check, and uses the Testsuite field to decide whether some of the precooked checks should be used. Wouldn't simply changing the logic to something like the following simplified stuff, do the trick? * change autopkgtest to always call autodep8 if auto_control is enabled. * change autodep8 to always cat debian/tests/control if present. * then autodep8 would cat the autogenerated tests like now based on the Testsuite field, and for backwards compat also the control.autodep8 file if present. Maybe this new mode of operation would be selected via a new command-line option, dunno. > If we would want to add support for this adding autodep8 on top of an > existing debian/tests/control file, apart from adapting autopkgtest with > a new option, also multiple interface would need to be adapted to pass > on this info: > britney <-> debci <-> autopkgtest Why is that, what information do these require? Thanks, Guillem
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Dear Guillem, On 30-07-18 05:42, Guillem Jover wrote: >> It would be great if dpkg-source would also parse >> debian/tests/control.autodep8 >> in the same way as it does debian/tests/control, except I guess it should not >> set the Testsuite field (as dpkg-source doesn't know which type of >> autopkgtest >> this is). > > Hmm, why did we get this new file at all? It's more than two years old. It was added in response to bug 823931. > Why can't we use > debian/tests/control instead? The Testsuite field would be used to > specify that we still want autodep8 to be run, no? Looking at the bug, I can only assume it just wasn't considered to change autopkgtest, e.g. IIAC the maintainer of autodep8 wasn't a developer of autopkgtest at the time. Also, the current implementation works without the spec needing to mention the debian/control file. autopkgtest (the binary that is _an_ implementation of the spec) only calls autodep8 when it doesn't find the control file as a fall back. autopkgtest doesn't know why it is called, it doesn't know about the Testsuite field, nor does anything in the CI framework except for the one that requests the test. If we would want to add support for this adding autodep8 on top of an existing debian/tests/control file, apart from adapting autopkgtest with a new option, also multiple interface would need to be adapted to pass on this info: britney <-> debci <-> autopkgtest Paul signature.asc Description: OpenPGP digital signature
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Control: tags -1 moreinfo Hi! On Fri, 2018-07-27 at 22:04:24 +0200, Paul Gevers wrote: > Package: dpkg-dev > Version: 1.19.0.5 > Severity: normal > File: /usr/bin/dpkg-source > X-Debbugs-CC: debian...@lists.debian.org, python-co...@packages.debian.org > User: debian...@lists.debian.org > Usertags: issue > Control: affects -1 src:python-cobra > In commit 90324cf support was added to dpkg-source to read > debian/tests/control > files and convert all test dependencies into a Testsuite-Triggers field. This > works great and is used by britney to do unstable-to-testing migration > tests. However, we recently spotted a regression (in python-cobra) that was > missed, because it uses autodep8 with additional test cases. This is done in > line with the autodep8(1) man page¹: > ''' > COMBINING AUTO-GENERATED TESTS WITH MANUALLY SPECIFIED ONES >If `debian/tests/control.autodep8` exists, autodep8 will prepend the >contents of that file to its own output. In that case, autodep8 will >exit with a status of 0 even if no known package type is detected. > ''' > > It would be great if dpkg-source would also parse > debian/tests/control.autodep8 > in the same way as it does debian/tests/control, except I guess it should not > set the Testsuite field (as dpkg-source doesn't know which type of autopkgtest > this is). Hmm, why did we get this new file at all? Why can't we use debian/tests/control instead? The Testsuite field would be used to specify that we still want autodep8 to be run, no? Thanks, Guillem
Bug#904776: /usr/bin/dpkg-source: please parse d/t/control.autodep8 like d/t/control
Package: dpkg-dev Version: 1.19.0.5 Severity: normal File: /usr/bin/dpkg-source X-Debbugs-CC: debian...@lists.debian.org, python-co...@packages.debian.org User: debian...@lists.debian.org Usertags: issue Control: affects -1 src:python-cobra Dear dpkg maintainers, In commit 90324cf support was added to dpkg-source to read debian/tests/control files and convert all test dependencies into a Testsuite-Triggers field. This works great and is used by britney to do unstable-to-testing migration tests. However, we recently spotted a regression (in python-cobra) that was missed, because it uses autodep8 with additional test cases. This is done in line with the autodep8(1) man page¹: ''' COMBINING AUTO-GENERATED TESTS WITH MANUALLY SPECIFIED ONES If `debian/tests/control.autodep8` exists, autodep8 will prepend the contents of that file to its own output. In that case, autodep8 will exit with a status of 0 even if no known package type is detected. ''' It would be great if dpkg-source would also parse debian/tests/control.autodep8 in the same way as it does debian/tests/control, except I guess it should not set the Testsuite field (as dpkg-source doesn't know which type of autopkgtest this is). I cloned the dpkg source, but as it is written in Perl, I felt uncomfortable providing a patch. I may reconsider if this is found valuable nevertheless. Thanks for all your good work. Paul ¹ https://manpages.debian.org/stretch/autodep8/autodep8.1.en.html -- System Information: Debian Release: buster/sid APT prefers testing-debug APT policy: (500, 'testing-debug'), (500, 'testing'), (200, 'testing'), (50, 'testing') Architecture: amd64 (x86_64) Kernel: Linux 4.17.0-1-amd64 (SMP w/2 CPU cores) Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE=en_US:en (charmap=UTF-8) Shell: /bin/sh linked to /bin/dash Init: systemd (via /run/systemd/system) LSM: AppArmor: enabled Versions of packages dpkg-dev depends on: ii binutils 2.31.1-1 ii bzip2 1.0.6-8.1 ii libdpkg-perl 1.19.0.5 ii make 4.2.1-1.1 ii patch 2.7.6-2 ii perl 5.26.2-6 ii tar 1.30+dfsg-2 ii xz-utils 5.2.2-1.3 Versions of packages dpkg-dev recommends: ii build-essential 12.5 ii fakeroot 1.23-1 ii gcc 4:8.1.0-1 ii gcc-5 [c-compiler] 5.4.1-4 ii gcc-6 [c-compiler] 6.4.0-18 ii gcc-7 [c-compiler] 7.3.0-27 ii gcc-8 [c-compiler] 8.1.0-12 ii gnupg2.2.9-1 ii gnupg2 2.2.9-1 ii gpgv 2.2.9-1 ii libalgorithm-merge-perl 0.08-3 Versions of packages dpkg-dev suggests: ii debian-keyring 2018.06.24 -- no debconf information signature.asc Description: OpenPGP digital signature
