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

Package:
debhelper
Source:
debhelper
Submitter:
Max-Julian Pogner
Date:
2022-07-03 09:21:12 UTC
Severity:
normal
Tags:
#1010594#5
Date:
2022-05-05 07:34:58 UTC
From:
To:
Dear Maintainer,

   * What led up to the situation?

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

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

#1010594#10
Date:
2022-05-08 06:25:45 UTC
From:
To:
Max-Julian Pogner:


Hi,

Thanks for reporting this bug.


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

#1010594#15
Date:
2022-05-14 20:00:59 UTC
From:
To:
tags +patch

thanks

(can i also send control commands here, or only to
control@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

#1010594#20
Date:
2022-05-15 06:45:08 UTC
From:
To:
Max-Julian Pogner:

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! :)

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:

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.

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

#1010594#27
Date:
2022-05-15 09:01:51 UTC
From:
To:
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.

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


cya,

Max

#1010594#34
Date:
2022-05-15 18:41:38 UTC
From:
To:
Max-Julian Pogner:

Ok, I merged your second patch too. :)

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

Thanks,
~Niels

#1010594#39
Date:
2022-07-03 09:18:54 UTC
From:
To:
We believe that the bug you reported is fixed in the latest version of
debhelper, 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 1010594@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Niels Thykier <niels@thykier.net> (supplier of updated debhelper 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)
Format: 1.8
Date: Sun, 03 Jul 2022 09:04:23 +0000
Source: debhelper
Architecture: source
Version: 13.8
Distribution: unstable
Urgency: medium
Maintainer: Debhelper Maintainers <debhelper@packages.debian.org>
Changed-By: Niels Thykier <niels@thykier.net>
Closes: 997961 1006805 1010591 1010594 1011099 1011256 1011404 1012777 1013969
Changes:
 debhelper (13.8) unstable; urgency=medium
 .
   [ Niels Thykier ]
   * dh_perl: Fix incorrect binary versioning caused by a regression
     in 13.3.  Thanks to Niko Tyni for reporting the issue.
     (Closes: #997961)
   * d/control: Bump Standards-Version to 4.6.1 - no changes required.
   * root_sequence.pm: Run `dh_installsysusers` before
     `dh_installtmpfiles` (only affects compat 14 or later).  Thanks
     to Nicholas Brown for the suggestion.  (Closes: #1011099)
   * autoscripts/postinst-init-tmpfiles: Remove runtime check on the
     system having systemd as init system.
   * dh_installtmpfiles: Replace runtime check on the system running
     under systemd in generated `postinst` script with a dependency
     on `systemd | systemd-tmpfiles` added to `${misc:Depends}`.
     Thanks to Guilhem Moulin for the suggestion.  (Closes: #1013969)
   * dh_installsystemd: Ditto.
   * dh_installinit: Ditto.
   * debhelper-compat-upgrade-checklist.pod: Correct description for
     the addition of dh_installsysusers in compat 14.
   * meson.pm: Use `meson install` instead of `ninja install` for
     dh_auto_install in compat 14.  Thanks to Andrea Pappacoda for
     the suggestion.  (Closes: #1006805)
 .
   [ Max-Julian Pogner ]
   * Dh_Lib.pm: Remove double semi-colon.  (Closes: #1010591)
   * Dh_Lib.pm: _strip_spaces now explicitly returns undef on undef
     argument.
     (Closes: #1010594)
 .
   [ Andrea Pappacoda ]
   * cmake.pm: Set FETCHCONTENT_FULLY_DISCONNECTED to true.  This
     ensures that the `FetchContent` module does not attempt to
     use internet connection.
 .
   [ Vagrant Cascadian ]
   * debhelper-compat-upgrade-checklist.pod: Update documentation
     for compat 14 change.  (Closes: #1011404)
 .
   [ Ferenc Wágner ]
   * debhelper.pod: Correct link to the new upgrade checklist.
     (Closes: #1012777)
 .
   [ Luca Boccassi ]
   * dh_installman: Avoid false positive language detection on
     shared libs.
 .
   [ Translations ]
   * Update Portuguese translation (Américo Monteiro)
     (Closes: #1011256)
Checksums-Sha1:
 ab83b353a662b126d7da2026ac0ad86baa4817c2 1781 debhelper_13.8.dsc
 490b5e4c26597e05ea9b023ba245467aeb173d82 571980 debhelper_13.8.tar.xz
 f8ec73d3d3c49891a82040fc2ec2f26284a6ff0b 5172 debhelper_13.8_source.buildinfo
Checksums-Sha256:
 0deea560ceff3f0e0bc310e8e69b1d868cae74099e4ca3ddaa9bf3a5078f6959 1781 debhelper_13.8.dsc
 3d22db5662a54e3bbe67de699d1e7fd88f920fb1fc1db58a1c45c164f4fb96ea 571980 debhelper_13.8.tar.xz
 3dea3d634f304086e0da639c0a16efd0fb591e5cd600db3bdfc08fab83efcfd5 5172 debhelper_13.8_source.buildinfo
Files:
 856c26b5f503ec889ceb4f3613826997 1781 devel optional debhelper_13.8.dsc
 504e6ddd34f8163c091622e510eb4254 571980 devel optional debhelper_13.8.tar.xz
 02353b46898a8c129a3d6dea14331157 5172 devel optional debhelper_13.8_source.buildinfo
-----BEGIN PGP SIGNATURE-----

iQJGBAEBCgAwFiEE8f9dDX4ALfD+VfsMplt42+Z8eqwFAmLBXJkSHG5pZWxzQHRo
eWtpZXIubmV0AAoJEKZbeNvmfHqsB1AP/jEqU/ia8TMTRW+bEDWVGq+WWqzYs3R+
7OJW6Vx4ZRIGLu97r5S0TG/U6xlVn3evLSWclz617+CLYLqThzpX7gv0Jf1E2lTw
LGbvjieMmGFCR/rX6i6tFHMs6dPqbzDCUHR2ES15cHqSmCt+U+m7vLLt0H83f94p
Fnfbjly8AXLmPv6UikLpViyT3RNQTMkwpHBknaH/8LCwu1MWEev8HRQI3WZoZ7dJ
tysZDTk6YN74RCrIy82uA9qzpNzUW4UxkuzEZ2y28bVtsXGZV0KOMB/8S5uX1sXy
FboQfuePazknAtZrqQVoe5uqog/mYhi4ogLOnmiy2xwS5sNu729wJGDvsa1Lg3WT
HuToCc1bHUIe+aJHVz0vRx0lU0oBk+16mZE6YX/tGuvJc9faIPFOoYTyCSWNos0v
knRYe8wbNFsOynF0uXgWCU3yyCVbWITDtzPV55AziADyakkgqmKUHRpz8F45Mb84
SM7QN35GcKQhnoBic/dMCedHlFhsSYZTO3SmB1XROf3STxR46q5/U7+VgNH6ZfO7
zUkAY5B2/BtQk3lc83ld/JEStW+KiZi0p6CTaj6fMxT/X1gqyyBKQIx+Q1KiANKo
V61mrR968Io3s9uveAURCLLPcZICqn2zXAdlGy7xC4CjKkdy+5CvUgzEBaZoH/Zq
doBEMIudGdZK
=SvjG
-----END PGP SIGNATURE-----