Bug#1010594: debhelper: Dh_Lib.pm assumes source "Section:" field is required, but it's documented as optional

[Max-Julian Pogner]

Package: debhelper
Version: 13.7.1
Severity: normal
X-Debbugs-Cc: max-j...@pogner.at

Dear Maintainer,

* What led up to the situation?

When using ``dh_testdir``, the following error messages appeared to me:

> Use of uninitialized value $v in substitution (s///) at /usr/share/perl5/Debian/Debhelper/Dh_Lib.pm line 1767, <$fd> line 9.
> Use of uninitialized value $v in substitution (s///) at /usr/share/perl5/Debian/Debhelper/Dh_Lib.pm line 1768, <$fd> line 9.

However, dh_testdir to the best of my knowledge continued normally after that.


* What exactly did you do (or not do) that was effective (or
ineffective)?

My investigation suggests, that Dh_Lib.pm assumes the source field "Section:" to be present always in the file ``debian/control``, although https://manpages.debian.org/sid/dpkg-dev/deb-src-control.5.en.html documents the "Section" source field to be optional (in that other source fields are explicitly marked "required" or "recommended", but this source field is neither).

I therefore think, that either

a) the documentation deb-src-control is incomplete and the source field "Section:" should be marked as required there, or
b) the Dh_Lib.pm should assign some default value to variable $source_section after the loop ``while (<$fd>) {`` in lines 1786 to 1835 has finished, or
c) Dh_Lib.pm line 1994, which reads

> $package_sections{$package} = _strip_spaces($field_values{'section'} // $source_section);;

should be changed to cope with the possibility that variable $source_section has value ``undef``, or
d) something else?


Please advise what would be the next step, maybe i can provide a patch then.


best regards,

Max



-- System Information:
Debian Release: bookworm/sid
APT prefers testing
APT policy: (500, 'testing')
Architecture: amd64 (x86_64)

Kernel: Linux 5.17.0-1-amd64 (SMP w/12 CPU threads; PREEMPT)
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8), LANGUAGE=en_GB:en
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages debhelper depends on:
ii autotools-dev 20220109.1
ii dh-autoreconf 20
ii dh-strip-nondeterminism 1.13.0-1
ii dpkg 1.21.7
ii dpkg-dev 1.21.7
ii dwz 0.14-1
ii file 1:5.41-4
ii libdebhelper-perl 13.7.1
ii libdpkg-perl 1.21.7
ii man-db 2.10.2-1
ii perl 5.34.0-4
ii po-debconf 1.0.21+nmu1

debhelper recommends no packages.

Versions of packages debhelper suggests:
pn dh-make <none>

-- no debconf information

[Niels Thykier]

Max-Julian Pogner:

> Package: debhelper
> Version: 13.7.1
> Severity: normal
> X-Debbugs-Cc: max-j...@pogner.at
>
> Dear Maintainer,
>
> * What led up to the situation?
>
> When using ``dh_testdir``, the following error messages appeared to me:
>
>> Use of uninitialized value $v in substitution (s///) at /usr/share/perl5/Debian/Debhelper/Dh_Lib.pm line 1767, <$fd> line 9.
>> Use of uninitialized value $v in substitution (s///) at /usr/share/perl5/Debian/Debhelper/Dh_Lib.pm line 1768, <$fd> line 9.
>

Hi,

Thanks for reporting this bug.


> [...]

>
> I therefore think, that either
>
> a) the documentation deb-src-control is incomplete and the source field "Section:" should be marked as required there, or
> b) the Dh_Lib.pm should assign some default value to variable $source_section after the loop ``while (<$fd>) {`` in lines 1786 to 1835 has finished, or
> c) Dh_Lib.pm line 1994, which reads
>
> > $package_sections{$package} = _strip_spaces($field_values{'section'} // $source_section);;
>
> should be changed to cope with the possibility that variable $source_section has value ``undef``, or
> d) something else?
>
>
> Please advise what would be the next step, maybe i can provide a patch then.
>
>
> best regards,
>
> Max
>

> [...]


I am inclined to go with option C by having `_strip_spaces` cope with
its input being undefined and just immediately returning (or skipping
the stripping part).

Thanks for considering to provide a patch, it is very appreciated. :) I
have already applied your patch from #1010591. If you prefer, you are
also very welcome to use salsa.debian.org to provide a merge request at
https://salsa.debian.org/debian/debhelper/-/merge_requests

Thanks,
~Niels

[Max-Julian Pogner]

tags +patch

thanks

(can i also send control commands here, or only to
con...@bugs.debian.org? i will know after sending this email)


Hi Niels,

> I am inclined to go with option C by having `_strip_spaces` cope with
> its input being undefined and just immediately returning (or skipping
> the stripping part).
>

I have created a patch and attached it to this mail.

Using command `debbuild` the package builds and i installed the
resulting deb package and it seems to work; plus one of the many
messages says:

> All tests successful.

So maybe all is fine? However my perl foo is not good enough to know how
to test whether a perl warning is issued.

> Thanks for considering to provide a patch, it is very appreciated. :)
> I have already applied your patch from #1010591. If you prefer, you
> are also very welcome to use salsa.debian.org to provide a merge
> request at
> https://salsa.debian.org/debian/debhelper/-/merge_requests

I have registered for an account now, but am of-course awaiting approval.


cya,

Max

[Niels Thykier]

Control: tags -1 pending

Max-Julian Pogner:

> tags +patch
>
> thanks
>
> (can i also send control commands here, or only to
> con...@bugs.debian.org? i will know after sending this email)
>
>
> Hi Niels,
>

Hi,

Thanks for the follow up. :)

You can do control messages when following up on the bug, but you have
to prefix them with "Control: " (in addition to them being in the top).
The bug number "-1" is in this case pre-sent to the bug you sent to.

I have included an example above, which also serves to mark it as
pending because I have merged your patch. Thanks for your contribution! :)

>> I am inclined to go with option C by having `_strip_spaces` cope with
>> its input being undefined and just immediately returning (or skipping
>> the stripping part).
>>
>
> I have created a patch and attached it to this mail.
>

Thanks, as mentioned I have merged it already.

For future reference, I do have one Perl nit that I do not know if you
are aware of for the following line:

> + return undef if not defined($v);

Perl has this weirdness where `return` and `return undef` behaves
differently when the sub is called in `list` context. In the concrete
case, it does not matter (as the sub is always called in `scalar`
context). My nit is that I prefer to use `return` (without `undef`)
for consistency that I am aiming towards (the existing code is not
following 100% either as I recall).

As mentioned (for this case), it is just a minor style thing and I have
not bothered with it. But in case you are doing more debhelper patches
(and I would welcome it if you did!), I wanted you to be aware of it.

> Using command `debbuild` the package builds and i installed the
> resulting deb package and it seems to work; plus one of the many
> messages says:
>
>> All tests successful.
>
> So maybe all is fine? However my perl foo is not good enough to know how
> to test whether a perl warning is issued.
>

> [...]
>
>
> cya,
>
> Max

Code-wise, this strongly resembles the fix I would have done and if it
removes the warning for the cause that triggered this bug report, I am
inclined to say it works.

In theory, we could also do a test for it under t/Dh_Lib. However, it
would involve a lot of work plus (I think) the Test::Warnings as a new
test dependency. To be honest, I am not sure it is worth the hassle.
In particular because Build-Dependencies have to be kept to a minimum -
which we can work around with a bit of more effort, but I still think it
is a lot of effort for something that is unlikely to regress.

On a final note - thanks for taking your time to report the bug and
providing the patch, which is now merged. I really appreciated that. :)

Thanks,
~Niels

[Max-Julian Pogner]

Control: tags -1 +patch


Hi,

> You can do control messages when following up on the bug, but you have
> to prefix them with "Control: " (in addition to them being in the
> top). The bug number "-1" is in this case pre-sent to the bug you sent
> to.
>
> I have included an example above, which also serves to mark it as
> pending because I have merged your patch. Thanks for your
> contribution! :)

I will remember for next time, thanks! :-)

> Perl has this weirdness where `return` and `return undef` behaves
> differently when the sub is called in `list` context.

I see, it feels weirdly logical that it is like that. And no, i was
definitely not aware of this.
Then i would rather do it correctly. Attached is a patch on top of the
first patch.

> and if it removes the warning for the cause that triggered this bug report

Yes, no warnings anymore for all my use-cases.


cya,

Max

[Niels Thykier]

Max-Julian Pogner:

> Control: tags -1 +patch
>
>
> Hi,
>

> [...]

>> Perl has this weirdness where `return` and `return undef` behaves
>> differently when the sub is called in `list` context.
>
> I see, it feels weirdly logical that it is like that. And no, i was
> definitely not aware of this.
> Then i would rather do it correctly. Attached is a patch on top of the
> first patch.
>

Ok, I merged your second patch too. :)

>
>> and if it removes the warning for the cause that triggered this bug
>> report
>
> Yes, no warnings anymore for all my use-cases.
>
>
> cya,
>
> Max

Excellent, then it will be fixed in the next version of debhelper.

Thanks,
~Niels