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?
Cheers
--
perl -E'$_=q;$/= @{[@_]};and s;\S+;<inidehG ordnasselA>;eg;say~~reverse'
--
To UNSUBSCRIBE, email to debian-mentors-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/20110309215050.ga29...@pc-ale.rete
