Re: Please review my patch for sysklogd
Am Sonntag, den 17.09.2006, 15:17 -0300 schrieb Otavio Salvador:
> Patrick Winnertz <patrick.winnertz@skolelinux.org> writes:
>
> > Hello!
> >
> > I tried this weekend on the BSP in Jülich, to remove some of the
> > blocking bugs for our RC Bug.
> >
> > So If you please can review my patch for sysklogd (and maybe improve it)
> > I added a debconf question, which only appears with a debconf prio of
> > "low": With this debconf question it is possible to preseed sysklogd to
> > log to a remote server. (The debconf question will
> > edit /etc/default/syslogd to specify some options.
>
> I'll do some comments myself that I think would make it easy to
> merge..., read bellow:
>
> [...]
> > diff -Nru /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/config /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/config
> > --- /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/config 1970-01-01 00:00:00.000000000 +0000
> > +++ /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/config 2006-09-17 16:15:44.000000000 +0000
> > @@ -0,0 +1,9 @@
> > +#!/bin/sh
> > +
> > +. /usr/share/debconf/confmodule
> > +
> > +##Fix for debian-edu-config:
>
> I think you can justify it, on bug report, saying that your motivation
> was to solve a Debian-EDU bug but I think the solution shouldn't cite
> it on code.
>
> > +#make sysklogd options preseedable
>
> # ask about default sysklogd options
>
> This question isn't only suitable for preseeding but also for advanced
> users and LTSP users too.#
Okay, this is easy to solve ;-)
>
> [...]
> > diff -Nru /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/control /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/control
> > --- /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/control 2006-09-17 16:15:44.000000000 +0000
> > +++ /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/control 2006-09-17 16:15:44.000000000 +0000
> > @@ -7,7 +7,7 @@
> > Package: sysklogd
> > Architecture: any
> > Section: admin
> > -Depends: ${shlibs:Depends}, klogd | linux-kernel-log-daemon
> > +Depends: ${shlibs:Depends}, klogd | linux-kernel-log-daemon, debconf ( >=0.5 ) | debconf-2.0
> > Conflicts: syslogd
> > Provides: syslogd, system-log-daemon
> > Replaces: syslogd
>
> This isn't the best way of doing that. Would be better to use
> dh_installdebconf capability to provide the misc:Depends variable for
> dpkg-gencontrol and that would also makes the new migrations for
> debconf far easier to deal then with hardcoded defaults.
>
> +Depends: ${shlibs:Depends}, ${misc:Depends}, klogd | linux-kernel-log-daemon
>
> Should work.
Yes; I know it, but as you see, the maintainer doesn't even use any dh_*
things, and he wrote a NMU-Disclaimer, where he said, that he doesn't
want to see dh_* in his rules file (so ${misc:Depends} is hard to
use :S )
>
> > diff -Nru /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/postinst /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/postinst
> > --- /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/postinst 2006-09-17 16:15:44.000000000 +0000
> > +++ /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/postinst 2006-09-17 16:15:44.000000000 +0000
> > @@ -2,6 +2,8 @@
> >
> > set -e
> >
> > +. /usr/share/debconf/confmodule
> > +
> > if [ ! -d /var/log/news ] \
> > && grep -q /var/log/news/ /etc/syslog.conf \
> > && grep -q ^news: /etc/passwd \
> > @@ -74,6 +76,19 @@
> > fi
> > set -e
> > fi
> > + ##Fix for debian-edu-config:
> > + #Make sysklogd options preseedable.
>
> Same comments then config file, see above.
>
> [...]
> > diff -Nru /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/rules /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/rules
> > --- /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/rules 2006-09-17 16:15:44.000000000 +0000
> > +++ /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/rules 2006-09-17 16:15:44.000000000 +0000
> > @@ -74,8 +74,8 @@
> > chown -R root:root debian/tmp.sysklogd
> > chmod -R g-ws debian/tmp.sysklogd
> > $(installbin) -d debian/tmp.sysklogd/usr/share/doc/$(package)
> > - $(installbin) debian/{preinst,postinst,postrm,prerm} debian/tmp.sysklogd/DEBIAN/
> > - $(installdoc) debian/conffiles debian/tmp.sysklogd/DEBIAN/
> > + $(installbin) debian/{preinst,postinst,postrm,prerm,config} debian/tmp.sysklogd/DEBIAN/
> > + $(installdoc) debian/{conffiles,templates} debian/tmp.sysklogd/DEBIAN/
>
> For patching propose this is OK but I would suggest to use
> dh_installdebconf and the rest of debhelper script bug it's outside of
> your patch scope. The general packaging refactoring would be another
> bug report. Keep in mind that current rules, used by the package, has
> bashism and would break with dash and others shells.
As I wrote above the maintainer doesn't want to use dh_* scripts :S
I also dislike this way to package a deb.
(But I doesn't want to fully replace the rules file, with a cdbs or
debhelper one.
>
> [...]
> > diff -Nru /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/templates /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/templates
> > --- /tmp/VrD1HUUixW/sysklogd-1.4.1/debian/templates 1970-01-01 00:00:00.000000000 +0000
> > +++ /tmp/PdZaIpjxOf/sysklogd-1.4.1/debian/templates 2006-09-17 16:15:44.000000000 +0000
> > @@ -0,0 +1,8 @@
> > +Template: sysklogd/sysklogd-default-options
> > +Type: string
> > +Description: Enter the options you want sysklogd to use
> > + Enter only the options like "-r" or "-m 0" without anything else before
> > + or behind.
> > +Description-de.UTF-8: Geben Sie die Optionen an, die sysklogd benutzen soll
> > + Geben Sie lediglich die Optionen an, ohne zum Beispiel den Programmnamen davor
> > + oder dahinter. Ähnlich wie dieses Beispiel: -r
>
> Use po-debconf for translation, please. Add it also in depends line
> ;-)
>
> Hopefully you'll appreciate the commends but, as I said, this are my
> personal POV and you might disagree.
In my own packages I use po-debconf and cdbs | debhelper so I fully
aggree with you. :)
We should talk to the maintainer if he doesn't want to rewrite his rules
file. (But as far as I can see, the maintainer doesn't want to have any
build-depends).
I will keep this way of packaging this package and rewrite the parts of
the patch which are easy to change. (If I have enough time I will
rewrite the whole rules file)
Greetings
Patrick
>
> TIA,
>
Reply to: