Re: RFS: ulatencyd - Daemon to minimize latency on a linux system using cgroups (2nd try)

[Alessandro Ghedini <al3xbio@gmail.com>]

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'