On Sat, May 05, 2018 at 06:18:00AM +0000, Niels Thykier wrote: > Peter Pentchev: > > Package: debhelper > > Version: 11.2.1 > > Severity: minor > > Tags: patch > > > > Hi, > > > > Thanks for maintaining and extending debhelper! > > > > > Hi Peter, > > Thanks for testing and the feedback. :) > > > When trying to test the new B-D: debhelper-compat (= 11) mechanism on > > my mdk and mdk-doc packages, I stumbled upon a minor issue: if this > > dependency is the first one listed, and it's on a separate line (there > > are no package names on the "Build-Depends:" line itself), then it is > > not recognized because the regular expression used will not match > > the whitespace before the "debhelper-compat" token. > > > > Indeed, that sounds like an annoying papercut. > > > Attached is a trivial patch. Well, actually the trivial patch is in > > the second commit; the first one adds some debhelper-compat tests and > > the third one adds a test for the bug fixed by the second one. > > Of course, if you feel that adding test-only functions to the Dh_Lib > > module is not really proper, it's your call whether to accept just > > the fix itself without the tests. > > > > Thanks again for taking care of debhelper! > > > > Best regards, > > Peter > > > > [...] > I am happy to apply all the patches with/after some minor tweaks: > > > From 4680d18a5125ce6a46afa846e9b4866df0ce7a72 Mon Sep 17 00:00:00 2001 > > From: Peter Pentchev <r...@ringlet.net> > > Date: Fri, 4 May 2018 23:59:04 +0300 > > Subject: [PATCH 1/3] Lay the groundwork for testing debhelper-compat. > > [...] > > diff --git a/lib/Debian/Debhelper/Dh_Lib.pm b/lib/Debian/Debhelper/Dh_Lib.pm > > index 9c829f0f..9666b7ad 100644 > > --- a/lib/Debian/Debhelper/Dh_Lib.pm > > +++ b/lib/Debian/Debhelper/Dh_Lib.pm > > @@ -63,7 +63,7 @@ our (@EXPORT, %dh); > > &compute_doc_main_package &is_so_or_exec_elf_file &hostarch > > &assert_opt_is_known_package &dbgsym_tmpdir &find_hardlinks > > &should_use_root &gain_root_cmd DEFAULT_PACKAGE_TYPE > > - DBGSYM_PACKAGE_TYPE > > + DBGSYM_PACKAGE_TYPE &compat_levels &resetcompat &resetpackages > > No need to export these; no tool (except tests) should need them and > they can live with using the fully qualified name. > > Example usage: > """ > Debian::Debhelper::Dh_Lib::resetcompat(); > """
Right, I should have thought of that. Will do.
> Note the separate comment about compat_levels below.
>
> > [...]
> > @@ -2386,4 +2399,15 @@ sub dbgsym_tmpdir {
> > }
> > }
> >
> > +sub compat_levels {
>
> Let's move this to Test::DH (and use the same method the max as used in
> each_compat_from_and_above_subtest). Probably renamed to
> "non_deprecated_compat_levels".
Hm, if by "use the same method" you mean "accept a code ref, wrap
the creation of the temporary directory in it, and copy some files",
it wouldn't be exactly the same in this case: here we need to generate
the control file from a template substituting different B-D contents for
each test case. Of course, if you mean just "accept a code ref and
wrap the temp dir creation within it", that might work - the test case
itself will remember its original directory and generate the control
file each time. Hm, come to think of it, there wouldn't really be any
harm done if the function also copied the control file over, and then
the test case regenerated it anyway - install_file() is cheap.
However, what about the fact that I actually lifted this from
the debian/gen-provides tool? It doesn't feel very clean to me to have
gen-provides use Test::DH, and it certainly feels a bit weird to have
gen-provides create a temporary directory for each compat level and
then proceed to completely ignore it and just use the level number.
> > [...]
> > +}
> > +
> > 1
> > [...]
> > index 00000000..0f711f5f
> > --- /dev/null
> > +++ b/t/debhelper-compat/syntax.t
> > [...]
> > +
> > +sub test_build_depends {
> > + my ($level, $build_depends) = @_;
> > + my $dir = Test::DH::tempdir(CLEANUP => 1);
> > [...]
>
> That is actually "File::Temp::tempdir" and should be imported/use'd
> directly. E.g.
>
> """
> use File::Temp qw(tempdir);
>
> ...
>
> my $dir = tempdir(CLEANUP => 1);
> """
Sure, that's actually what I initially wrote; I don't even remember why
I decided to change it to use the one "already provided" by Test::DH.
Will do.
> > --
> > 2.17.0
> >
>
>
> > From 8cc0086067b77be6f3ac130a50e375c6c379e212 Mon Sep 17 00:00:00 2001
> > From: Peter Pentchev <r...@ringlet.net>
> > Date: Sat, 5 May 2018 00:09:08 +0300
> > Subject: [PATCH 2/3] Allow whitespace before the debhelper-compat B-D.
> >
> > This allows a newline before the debhelper-compat dependency, e.g.
> > when all the dependencies are listed on separate rows.
> > ---
> > lib/Debian/Debhelper/Dh_Lib.pm | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/Debian/Debhelper/Dh_Lib.pm b/lib/Debian/Debhelper/Dh_Lib.pm
> > index 9666b7ad..dffd2fad 100644
> > --- a/lib/Debian/Debhelper/Dh_Lib.pm
> > +++ b/lib/Debian/Debhelper/Dh_Lib.pm
> > @@ -1467,7 +1467,7 @@ sub getpackages {
> > my $value = join(' ', @{$bd_fields{$field}});
> > $value =~ s/\s*,\s*$//;
> > for my $dep (split(/\s*,\s*/, $value)) {
> > - if ($dep =~
> > m/^debhelper-compat\s*[(]\s*=\s*(${PKGVERSION_REGEX})\s*[)]$/) {
> > + if ($dep =~
> > m/^\s*debhelper-compat\s*[(]\s*=\s*(${PKGVERSION_REGEX})\s*[)]$/) {
>
> Let's add trailing whitespace while we are at it, so we do not get a
> repeat for that. I am not sure how to generate that in a real case but
> I hadn't seen how to generate leading whitespace either plus the test
> case can trivially do it.
Hm, and now you got me thinking - should we also try to microoptimize and
fix this in a different way, taking it out of this regular expression
altogether so that it's not done for each and every dependency, and moving
it before the loop?
$value =~ s/^\s*//;
$value =~ s/\s*(?:,\s*)?$//;
...or something like that?
Thanks for the quick reply!
G'luck,
Peter
--
Peter Pentchev r...@ringlet.net r...@freebsd.org p...@storpool.com
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13
signature.asc
Description: PGP signature
