Re: RFS: dhcp-probe, another try to request with a lot of update
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Michael Tautschnig a écrit :
> Hi Laurent,
>
> [...]
>
> I've started another review of your package. The following things jumped at me
> immediately (just tried to build the package using dpkg-buildpackage):
>
> =================================== dpkg-buildpackage snippet ==================
> make: Circular distclean <- clean dependency dropped.
> dh_testdir
> dh_testroot
> test ! -f config.cache || rm -f config.cache
> test ! -f config.status || rm -f config.status
> test ! -f config.log || rm -f config.log
> =================================== dpkg-buildpackage snippet ==================
OK, OK, i think that is not the cleaner method to run a rm -rf on a file
which doesn't exist but if you want, and if it is the usage in debian
project, i will do
>
> - Your debian/rules has a circular dependency distclean/clean, please fix this.
> Actually, clean alone should be all you need, distclean is usually what is
> provided by the source's Makefiles
> - rm -f will do, no need to test first. Really.
Circular dependency between clean and distclean is dead ;)
Sorry for absentmindedness.
>
> Then, trying to build stuff again (that is, running dpkg-buildpackage twice in a
> row) building immediately fails because some cleanup is not done properly, which
> may be the result of the clean/distclean mess.
>
> A further review of your debian/ files yields:
> - Please cleanup your changelog. Run a spell checker, remove duplicate info (no
> need to have 4 lines to comment on the man-page fix), remove wrong info
> (buildconf), and
Done with changelog review
> - make sure you get consistent about the config directory -- names range from
> /etc/dhcp-probe to /etc/dhcp-probed to /etc/default/dhcp-probe (check also
> debian/dirs)
/etc/dhcp-probed not found after several corrections. May be it was in
the changelog ?
> - Your init.d file feels fairly hacked, please take a look at, e.g., the init.d
> file of the arpwatch package (which also runs multiple daemons for multiple
> interfaces). Just looking at the code you use to kill daemons makes me shiver.
> start-stop-daemon will do. Really. Please don't keep all the stuff you
> commented out, either remove it or implement it. force-reload, however, must
> get implemented (see Debian Policy Sec. 9.3.2).
your words "fairly hacked" is a joke ?
The main problem i found in my init.d file was the signal used to kill
the daemon (9) instead of the normal (15).
For each configuration files, the script will read information about
interface and its properties. Then script read daemon PID associated to
configuration file and kill it and delete the PID file (added in latest
update).
The force_stop function is here to handle, with a basic system view,
cases where daemons have to be stop.
Moreover, arpwatch package does not provide the status option if
'running*' function are sources of the reaction...
Could you explain me your comment ?
> - Your postinst should not do the configuration file auto-build in all cases,
> rather make use of the case statement to only do it in specific cases (like
> configure, but not on abort-*).
OK, configuration files are built only in configure step of package
installation.
> - Your postrm has redundant (and strange!) calls to rm: You don't need to remove
> each and every file if you run rm -rf afterwards!
OK, it code from an older version that was in...
> - debian/rules:
> * lines 11--18: These variables are never used, remove them
These variables are now used !
> * line 39: Remove that distclean stuff in here
This target now call the "make distclean" of the upstream code via the
dh_auto_clean from debhelper.
> * line 44: No, don't remove config.log manually, instead do
> test ! -f Makefile || make distclean
> which will actually also fix some more problems, I guess.
Now I use dh_auto_clean -- distclean instead of direct call of make.
> * lines 45, 46: What is that good for? dh_clean will do.
> * line 51: build depends on config.status already, no need to depends once
> again
> * line 56: Remove dead comment
Done
> * lines 59,61: Only depend on install, you can't do build and install in
> parallel and install already depends on build
> * lines 62,63: dh_testdir is ok, no additional files required
> * line 69: I think defaults should be fine, why do you add 80 15; that
> contradicts "defaults"
According to tour advice i put "defaults" level to start daemon but i
think that it is better to start the daemon before network start and
stop it after.
> You may have good reasons for all that stuff and have done that intentionally.
> If that is the case, please comment on the specific parts.
OK, understood !
As everybody, it is difficult to argue a thing that it seems logical but
i will try to do for future ;)
>
> Best,
> Michael
>
>
>
>
>
>
- --
Laurent Guignard, Registered as user #301590 with the Linux Counter
Site : http://www.famille-guignard.org
Blog : http://blog.famille-guignard.org
Projet : http://sicontact.sourceforge.net
GULL de Villefranche sur Saône : http://www.cagull.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFJSV0hjcKpXFc/7oYRAoijAKDEA+zPWIkr1Evjp2P0aRlrEeP40QCfUNhE
Q8P7f9rw39XXjbk23yvL9C4=
=wiSL
-----END PGP SIGNATURE-----
Reply to: