[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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

On Thu, Mar 03, 2011 at 04:10:03PM +0100, Alessandro Ghedini wrote:
> 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?

Any news about this?


perl -E'$_=q;$/= @{[@_]};and s;\S+;<inidehG ordnasselA>;eg;say~~reverse'

Reply to: