On Sat, May 05, 2018 at 04:10:18PM +0300, Peter Pentchev wrote:
> 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.
On second thoughts, maybe you meant "just accept a code ref and pass
the compat levels to it one by one"; I can do that later today, tomorrow
at the latest. Thanks for the idea, and, yes, it is indeed consistent
with the other Test::DH routines.
> > > [...]
> > > +}
> > > +
> > > 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
