Hi Chris, On Fri, Nov 14, 2008 at 12:23:34PM +0000, Chris Halls wrote: > Hi Xavier > > On Friday 14 Nov 2008, Xavier Luthi wrote: > > I've prepared a new package version for apt-proxy. It's currently an > > NMU but of course, if you've time, you can upload it as a new non-NMU > > revision of the package. > > Thanks a lot for making your changes. Do you have an Alioth account? If so > I'll add you to the project so you can check changes into SVN.
I've an Alioth account xluthi-guest. > > I'm too busy to make an upload before the end of next week, so please go > ahead > with an upload if you want to do it before then. > I'm not in a hurry at all for uploading and as I've no uploading rights for apt-proxy (I'm already a Debian Maintainer for others packages), it makes no sense to harrass another DD to sponsor my upload. So please, take your time to do the upload once you have more time. > > You'll find attached the corresponding debdiff, the package itself > > can be downloaded here: > > - URL: http://mentors.debian.net/debian/pool/main/a/apt-proxy > > I have some comments on your changes. > > > + # see bug #285770 > > + if len(value) == 0: > > + return None > > > - raise ConfigError("Configuration parse error: [%s] %s" % > > (section, option)) + # see bug #285770 > > + #raise ConfigError("Configuration parse error: [%s] %s" % > > (section, option)) + return None > > > value = self.parseConfigValue(config, DEFAULTSECT, name, > > default, getmethod) + # see bug #285770 > > + if value == None: > > + value = default > > + # end of bug #285770 > > These changes feel ugly to me. An empty value, such as 'timeout =' is an > error > in the configuration and I think you should only use the default value if the > line is not there in the configuration at all. > > What about handling the ConfigError exception and giving a proper error > message instead? > With this simple patch, my point was to not block the starting process of apt-proxy because some options are empty in the configuration file. However, I understand your point. For me, choosing between the two possibilities is more a personal preference. The current behavior of apt-proxy is to raise a ConfigError exception with a not so explicit message. This exception is never catched meaning the end user is seeing an ugly traceback before the error message, which is not user friendly at all. So for me , the ideal solution is to raise an exception AND to catch it anywhere in order to: 1. log the proper message in the log file 2. print an error message on the console 3. stop the application in a clean way OR continue it with the default value. > > - self.worker = FileVerifierProcess(self, '/usr/bin/bunzip2', > > '--test', self.path) > > + self.worker = FileVerifierProcess(self, > > '/bin/bunzip2', '--test', self.path) else: > > Are we sure that bunzip2 will always be found here? It might be better to use > the system path instead of a hardcoded absolute path. > I cannot be sure it will always be there, of course ;-) The best solution is to use the system path for bunzip2, but also for dpkg and gunzip (a few lines above my patch in cache.py). > > +## change process priority as requested in http://bugs.debian.org/275658 > > +# and do that silent > > +renice 5 $$ >/dev/null > > I think this should be configurable in /etc/default/apt-proxy so that the > system administrator can turn this off if they need to. > Except if I've made some mistake in my patch, this patch is applied on debian/default which is installed as /etc/default/apt-proxy... > > +process=$(basename $application) > > > + sleep 1 # for some startup time and then check > > + ps -eo cmd | grep -q ^$process > > Can you use ps -u $user instead of ps -e, to only check processes with the > aptproxy user? Yes we can. Do you think it's better? > > I am very happy with all of your other changes - thanks! > Thank you! Before uploading anything, I think it should be good to clean the debian maintainers scripts of this package to remove all stuff concerning the transition between v1 and v2 of apt-proxy. The reasons are: 1. AFAIK, this transition is not happening anymore 2. It can produce unecessary work for l10n teams (see #378095) 3. preinst script is buggy because of that (try running it with set -e...) 4. Associated templates produce a lot of lintian warnings. Cheers, Xavier -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
