Bug#932438: RFS: irqbalance/1.6.0-1 [ITA] -- Daemon to balance interrupts for SMP systems
Hi Dmitry, thanks for your review.
Dmitry Bogatov wrote on 20/07/2019:
> [2019-07-19 12:57] Paride Legovini <pl@ninthfloor.org>
>> Package: sponsorship-requests
>> Severity: normal
>>
>> Please review my packaging branch for irqbalance/1.6.0-1 at:
>>
>> https://salsa.debian.org/paride-guest/irqbalance
>> [...]
>
> On your changes:
>
> 1. In [942ed5e] you added this line:
>
> export DEB_LDFLAGS_MAINT_APPEND = -Wl,--as-needed
>
> Why is it needed? Maybe, these flags should be provided by
> dpkg-buildflags(1)?
dpkg-buildflags does not currently provide the --as-needed flag to the
linker, see:
https://wiki.debian.org/HardeningWalkthrough
or just try. The flags are not needed but I think using them when
possible is good practice. For what --as-needed does see ld(1).
> 2. I think manpage in wrong direction should be patched, not overrided.
>
> On package in general (does not block upload). These issues were present
> before your changes, but it would be nice to solve them.
I was expecting this comment :-)
I don't really oppose patching the manpage section, and I'm willing to
do so should you suggest it again in the next review round, but I
personally don't like the idea very much.
It's nice to have the manpage in the "right" section, but I also value
consistency across distributions, operating systems, and with upstream,
when it makes sense. Irqbalance has its manpage in section 1 in every
supported operating system; is fixing this small annoyance worth
deviating? What if some README file, code comment, or the manpage itself
references to the manpage as irqbalance(1)? We should patch all the
references too, and double-check every future upstream release. Is the
maintenance burden justified? In 5 years from now will we end up with a
Debian package sometimes referencing to section 1, sometimes to section
8, with no apparent reason?
Now, irqbalance is a relatively simple package, it would be easy to keep
the patch up to date, but you see my general point.
I *did* want to get it fixed, and even submitted a PR for it:
https://github.com/Irqbalance/irqbalance/pull/110
but I think upstream's objection is valid.
> 3. What is "irqbalance-ui"? It has no manpage, not referenced in
> irqbalance(1) and even after source diving I can't understand how to
> use it.
I'll probably add a stub manpage for it.
> 4. Situation with "oneshot" option is quite... inconvenient.
>
> * If you have ONESHOT option set, `/etc/init.d/irqbalance status`
> will report failure. It can be fixed by checking for ONESHOT
> variable in status) clause;
Looks reasonable.
> * current runscript does not respect ONESHOT option. It can be fixed
> with something like
>
> #!/bin/sh
> . /etc/default/irqbalance
> if [ -n "${ONESHOT:-}" ] ; then
> irqbalance --oneshot
> sv down irqbalance
> else
> exec irqbalance --foreground
> fi
>
> but it would be quite unnatural. I think proper solution would be
> separation of /etc/init.d/irqbalance and /etc/init.d/irqbalance-oneshot.
I agree calling `sv down` from the runscript is ugly, on the other side
I don't really like the idea of having two sets of init scripts for
three init systems. Moreover while dh_installsystemd and dh_installinit
have a --no-enable option, dh_unit does not, requiring some manual
(hacky) handling.
I found an answer of stackoverflow suggesting to run "sv once" in the
runscript to set the expected mode. What do you think of this solution?
[0]
https://stackoverflow.com/questions/17035341/use-runit-to-only-boot-up-the-service-and-not-supervise
> 5. Do we really need debconf to configure oneshot feature? Debconf
> question block installation process, so they are not to be used
> lightly, imho. Even ssh server does not use debconf to make me
> review its config, which is of much more importance.
I am more than happy with dropping it entirely.
I'll ping you again when the updated branch is ready to be reviewed
again. Thanks again.
Paride
Reply to: