Bug#370610: NMU for apt-proxy
Hi Chris, 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. 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 - dget http://mentors.debian.net/debian/pool/main/a/apt-proxy/apt-proxy_1.9.36.3+nmu2.dsc Here is the changelog: apt-proxy (1.9.36.3+nmu2) unstable; urgency=low * Non-maintainer upload. * set Apt::Cache-Limit to a high value (Closes: #370610) * Correct handling of empty configuration file options (Closes: #285770) * Path for bunzip2 corrected (Closes: #391869) * doc/apt-proxy-import.8: explain what import means (Closes: #355523) * doc/apt-proxy.conf.5: - document file: backends (Closes: #386562) - document debug option (Closes: #272155) * French program translation update (Closes: #485078) * debian/default: Setting a nice value (Closes: #275658) * debian/init.d: Prevent apt-proxy to fail silently during startup (Closes: #432221) * Fix some easy to correct lintian warnings: - debian/control: Build-Depends on python (=2.3.5) instead of 2.3.5-1 - debian/rules: dh_python statement removed as useless - debian/postinst: deprecated chown usage fixed - debian/copyright: clear copyright notice added - debian/preinst, prerm, postrm: 'set -e' added * -- Xavier Luthi [EMAIL PROTECTED] Fri, 14 Nov 2008 11:46:12 +0100 Cheers, Xavierdiff -Nru apt-proxy-1.9.36.3+nmu1/apt_proxy/apt_proxy_conf.py apt-proxy-1.9.36.3+nmu2/apt_proxy/apt_proxy_conf.py --- apt-proxy-1.9.36.3+nmu1/apt_proxy/apt_proxy_conf.py 2007-06-04 11:54:39.0 +0200 +++ apt-proxy-1.9.36.3+nmu2/apt_proxy/apt_proxy_conf.py 2008-11-13 16:58:48.0 +0100 @@ -45,12 +45,17 @@ def getint(self, section, option): value = self.get(section, option) +# see bug #285770 +if len(value) == 0: +return None return int(value) def gettime(self, section, option): mult = 1 value = self.get(section, option) if len(value) == 0: -raise ConfigError(Configuration parse error: [%s] %s % (section, option)) +# see bug #285770 +#raise ConfigError(Configuration parse error: [%s] %s % (section, option)) +return None suffix = value[-1].lower() if suffix in self.time_multipliers.keys(): mult = self.time_multipliers[suffix] @@ -196,6 +201,10 @@ # read default values for name,default,getmethod in self.CONFIG_ITEMS: value = self.parseConfigValue(config, DEFAULTSECT, name, default, getmethod) +# see bug #285770 +if value == None: +value = default +# end of bug #285770 setattr(self, name, value) if value != default and name != telnet_pass: log.debug(config value %s=%s%(name, value), 'config') diff -Nru apt-proxy-1.9.36.3+nmu1/apt_proxy/cache.py apt-proxy-1.9.36.3+nmu2/apt_proxy/cache.py --- apt-proxy-1.9.36.3+nmu1/apt_proxy/cache.py 2008-06-28 20:31:24.0 +0200 +++ apt-proxy-1.9.36.3+nmu2/apt_proxy/cache.py 2008-11-14 10:13:27.0 +0100 @@ -597,7 +597,7 @@ elif re.search(r\.gz$, self.path): self.worker = FileVerifierProcess(self, '/bin/gunzip', '-t', '-v', self.path) elif re.search(r\.bz2$, self.path): -self.worker = FileVerifierProcess(self, '/usr/bin/bunzip2', '--test', self.path) +self.worker = FileVerifierProcess(self, '/bin/bunzip2', '--test', self.path) else: # Unknown file, just check it is not 0 size try: diff -Nru apt-proxy-1.9.36.3+nmu1/apt_proxy/packages.py apt-proxy-1.9.36.3+nmu2/apt_proxy/packages.py --- apt-proxy-1.9.36.3+nmu1/apt_proxy/packages.py 2007-08-14 23:02:44.0 +0200 +++ apt-proxy-1.9.36.3+nmu2/apt_proxy/packages.py 2008-11-13 16:55:50.0 +0100 @@ -105,7 +105,8 @@ #'APT' : '', 'APT::Architecture' : 'i386', # TODO: Fix this, see bug #436011 and #285360 #'APT::Default-Release' : 'unstable', - +'APT::Cache-Limit' : '1', + 'Dir':'.', # / 'Dir::State' : 'apt/', # var/lib/apt/ 'Dir::State::Lists': 'lists/', # lists/ diff -Nru apt-proxy-1.9.36.3+nmu1/debian/changelog apt-proxy-1.9.36.3+nmu2/debian/changelog --- apt-proxy-1.9.36.3+nmu1/debian/changelog2008-06-28 20:33:47.0 +0200 +++ apt-proxy-1.9.36.3+nmu2/debian/changelog2008-11-14 11:46:42.0 +0100 @@ -1,3 +1,27 @@ +apt-proxy (1.9.36.3+nmu2) unstable; urgency=low + + * Non-maintainer upload. + * set Apt::Cache-Limit to a high value (Closes: #370610) + * Correct handling of empty configuration file options (Closes: #285770) + * Path for bunzip2 corrected (Closes: #391869) + *
Bug#370610: NMU for apt-proxy
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'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. 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? -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. +## 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. +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? I am very happy with all of your other changes - thanks! Chris -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of unsubscribe. Trouble? Contact [EMAIL PROTECTED]
Bug#370610: NMU for apt-proxy
Hi Chris, On Fri, Nov 14, 2008 at 12:23:34PM +, 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]
