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.
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. 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. > 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 > + > + [ Reinhard Tartler ] > + * Refresh patches. > + > + [ Andriy Beregovenko ] > + * Add basic Lua configuration scripts > + * Returns back package "crtmpserver" I fail to understand this. Remember, that debian/changelog is exposed to the users of your package, for example by apt-listchanges and others. > + * Add more application conf scripts Which ones? > + * Makes main configuration script more clean I notice a number of whitespace issues in the scripts, making them 'unclean' > + * 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. 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. 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? > + * Make some cosmetic fixes Unclar. Clarify or drop it. > + * Update installed list of files for package crtmpserver > + * Add installing config scripts for all applications > + * Rename application scripts to make future updates more easy > + * Add portinstall script for crtmpserver-apps package typo > + * Rename scripts back :) Uh? > + * Install application cofngis as example, and while postinstall stage > "configure" copy it to etc if not exists already. typo (I know that some of them have been fixed in later commits) > + * Move enabled_applications.conf to -apps package > + * Move enabled_applications.conf to -apps package > + * Add to packages > + * Update gbp.conf to ignore .svn while import sources > + * Imported Upstream version 0.0~dfsg+svn611.1 > + > + -- Andriy Beregovenko <j...@jet.kiev.ua> Fri, 11 Nov 2011 18:23:04 +0200 > + In general, the changes in debian/changelog should state the differences to the previous version that was uploaded to the archive. Having said this, I fear the package still requires work :-( -- Gruesse/greetings, Reinhard Tartler, KeyID 945348A4 _______________________________________________ pkg-multimedia-maintainers mailing list pkg-multimedia-maintainers@lists.alioth.debian.org http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pkg-multimedia-maintainers
