Hi Reinhard, On Sat, Nov 19, 2011 at 11:07:34AM +0100, Reinhard Tartler wrote: > Hi, > > Sorry for being late, but I am currently increadibly busy with my > day-job and have a numer of other open source projects that compete with > my available time. I've now spend over 30 minutes reviewing your > package, which is much longer than I intended to invest. Unfortunately, > I also have some comments. Well... This is your participation as an Debian maintainer. Am I right? BTW, I do not ask you to do something RIGHT NOW. Your reaction is not clear to me. > First of all, I noticed that you've pushed the tag for this upload to > the repository. Please don't, leave that to the uploader. In this > particular case, I think we need to do some additional changes so that > the tag will not represent what will eventually go into the archive. Ok.
> On Fr, Nov 11, 2011 at 17:31:00 (CET), jet-gu...@users.alioth.debian.org > wrote: > > > The following commit has been merged in the master branch: > > commit 1ba6610128bcf372e3295496d9f933470a3bed89 > > Author: Andriy Beregovenko <j...@jet.kiev.ua> > > Date: Fri Nov 11 18:23:54 2011 +0200 > > > > Correcting release > > This commit message does not really match the contents. I notice this > applies to a number your commits, which makes doesn't make reviewing any > easier. Ok, I will comment my next commits more descriptive and more accurately. > > diff --git a/debian/changelog b/debian/changelog > > index 12d651d..7162b0d 100644 > > --- a/debian/changelog > > +++ b/debian/changelog > > @@ -1,3 +1,29 @@ > > +crtmpserver (0.0~dfsg+svn611.1-1) UNRELEASED; urgency=low > > please merge the "UNRELEASED" entry of '0.0~dfsg+svn611-1' with this one Done. > > + * Makes main configuration script more clean > > I notice a number of whitespace issues in the scripts, making them 'unclean' Definitely!!! Btw that commit is not about whitespaces :) > > + * Add rc script > > I think we discussed this earlier this year. In commit > http://anonscm.debian.org/gitweb/?p=pkg-multimedia/crtmpserver.git;a=commitdiff;h=e4f2bce15ab888aabcb97c035a09373bbf44c5af, > I have removed it, and in > http://anonscm.debian.org/gitweb/?p=pkg-multimedia/crtmpserver.git;a=commitdiff;h=c06ba11fbb9741c8de6253a64e04e9d978b8346a > you are adding it back with 'Add rc script'. > > I'm still not convinced that this is the way to go, but if you insist > on this, you should better have used 'git revert', so that this fact > becomes clear. It would have saved my a lot of time, BTW. I can't cause that commit removes MANY files, but I need only one(two) file(s). > Moreover, I'm not happy with the script, because in order to use it, > users *have* to edit it. The previous solution with > /etc/default/$servername is much more common in debian packages. Still, > defaulting it to 'no' is something I personally feel very uncomfortable > with. That was error, already fixed. > I'm uncomfortable with it because IMO, installing a service should > instantly make it usable. For instance, installing the apache package > starts a webserver that serves a basic 'welcome' webpage. Can't we do > something similar for crtmpserver? Yes. It is ready to use out-of-box. But there is error in rc script, already fixed. > > + * Make some cosmetic fixes [ ... ] > In general, the changes in debian/changelog should state the differences > to the previous version that was uploaded to the archive. Already remade it. > Having said this, I fear the package still requires work :-( > > -- > Gruesse/greetings, > Reinhard Tartler, KeyID 945348A4 -- Best regards, Andriy 0xBDDBDAE3
signature.asc
Description: Digital signature
_______________________________________________ pkg-multimedia-maintainers mailing list pkg-multimedia-maintainers@lists.alioth.debian.org http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-multimedia-maintainers
