Hello Didier, On Wed, Mar 02, 2011 at 05:35:04PM +0100, Didier 'OdyX' Raboud wrote: > here is my (promised) review, with some delay; please forgive me for that; > life took over…
No problem, and thanks for the review. I've just uploaded the new upstream version (0.4.9) of the package, released a couple of days ago. It fixes some issues (e.g. the wrong systemd service file path) and it also adds some new nice features. The most notable is the addition of a 'ulatency' python script, which is basically, a simple client for the daemon. Since it depends on python and some other python modules I decided to add a new binary package for it (called 'ulatency') so that if someone do not want the client, he is not force to install all its python dependencies. Hope it's not a problem for you to review the new version as well. You can find the new package on mentors.d.n: dget http://mentors.debian.net/debian/pool/main/u/ulatencyd/ulatencyd_0.4.9-1.dsc As well as on git: git clone git://git.debian.org/collab-maint/ulatencyd.git > Now some questions: > > * Why don't you ship the systemd service file? With systemd around the > corner, you will certainly end up adding it in the future. And why are you > stripping it away with a patch (where you could dh_auto_install to > debian/tmp and have a "ulatencyd.install" file to opt files _in_) ? I would > just correct the path in this install file and be done with it. Indeed. The wrong path issue has been fixed upstream, so that now the systemd service file gets installed properly. Regarding the patch, I simply found adding a patch easier, but your solution is more elegant. In the new version of the package there's another patch that makes a file to be installed. In this case I chose a patch to report more easily the problem upstream (I've already forwarded that patch, and it is being merged). > * Your debian/init.d isn't named correctly (IMHO). man dh_installinit tells > us that it should be named debian/ulatency.init (or debian/init, but I very > much prefer being explicit). As for the names, it's the same for logrotate, > manpages and docs (but don't worry, it's mostly a matter of taste). I've fixed the names for all the files. Since now two binary packages are built, it's not a matter of taste anymore (particularly for the *.manpages ones) :) > * Deactivation of the tests: why do you disable the tests ? Build tests > should be run and they should not fail (obviously…). You should either > comment your debian/rules explaining the reasons or (preferably) convince > upstream to patch (or patch yourself) the tests in order to be able to run > within the buildd environment. The tests need the ulatencyd daemon to be running, which needs its core library properly installed under /usr/lib/... AFAIK this is not possible at build time, unless build-depending circularly on the ulatencyd package itself. I have now documented this in the rules file though. > * debian/gbp.conf should not be in the source package; having a > debian/source/local-options to filter it out sounds nice. Fixed... I think. I renamed the debian/gbp.conf to .gbp.conf and added the extend-diff-ignore local-option to ignore it. Is this what you suggested or is there a better solution? Thank you again for the help. Cheers -- perl -E'$_=q;$/= @{[@_]};and s;\S+;<inidehG ordnasselA>;eg;say~~reverse' -- To UNSUBSCRIBE, email to [email protected] with a subject of "unsubscribe". Trouble? Contact [email protected] Archive: http://lists.debian.org/20110303151003.GA2640@PC-Ale
