Code review comment for lp:~gary/launchpad/zbuildout

Hi Gary,

OK, I managed to go through the changes. I have a bunch of trival comment,
nothing that should prevent you from merging this.

Finally! Good work!

  status approved
  review approve

> === modified file 'buildout-templates/_pythonpath.py.in'
> --- buildout-templates/_pythonpath.py.in 2009-06-24 20:22:29 +0000
> +++ buildout-templates/_pythonpath.py.in 2009-08-18 21:42:18 +0000
> @@ -11,3 +11,10 @@
> sys.path[0:0] = [${string-paths}]
> # Enable Storm's C extensions
> os.environ['STORM_CEXTENSIONS'] = '1'
> +
> +# we don't want to bother tests or logs with these.

Capitalization.

> +import warnings
> +warnings.filterwarnings(
> + 'ignore',
> + 'Module .+ was already imported from .+, but .+ is being added.*',
> + UserWarning)
> \ No newline at end of file

Missing newline.

> === modified file 'buildout.cfg'
> --- buildout.cfg 2009-08-05 03:30:57 +0000
> +++ buildout.cfg 2009-08-18 21:42:18 +0000
> @@ -56,6 +56,12 @@
> initialization = import os
> os.environ['STORM_CEXTENSIONS'] = '1'
> os.environ.setdefault('LPCONFIG',
'${configuration:instance_name}')
> + # this can hopefully be removed when Twisted is used as an
egg.

This should be an XXX pointing to a bug about using twisted as an egg. (And
please fix the capitalization of the comment.)

> === modified file 'lib/canonical/config/__init__.py'
> @@ -194,8 +195,10 @@
>
> def _setZConfig(self):
> """Modify the config, adding automatically generated settings"""
> - schemafile = os.path.join(
> - self.root, 'lib/zope/app/server/schema.xml')
> + self.root = TREE_ROOT

Why did you need to add that line back? I mean self.root should already be
defined properly.

> === modified file 'lib/canonical/testing/ftests/test_mockdb.py'

Instead of commenting everything here, why didn't you simply comment out the
line adding the MockDBTestCase to the suite in suite() ? I think this would
basically have the same effect. (With a comment explaining why this is
disabled.

> === modified file 'lib/canonical/testing/tests/test_mockdb.py'

Same kind of comments here.

> === modified file 'setup.py'
> --- setup.py 2009-07-24 11:14:47 +0000
> +++ setup.py 2009-08-18 21:42:18 +0000
> @@ -20,26 +20,78 @@
> maintainer='Launchpad Developers',
> description=('A unique collaboration and Bazaar code hosting platform '
> 'for software projects.'),
> - license='LGPL v3',
> + license='Affero GPL v3',
> + # this list should only contain direct dependencies--things imported or
> + # used in zcml.
> install_requires=[
> 'bzr',
> + 'chameleon.core',
> + 'chameleon.zpt',
> 'feedvalidator',
> 'funkload',
> 'launchpadlib',
> 'lazr.smtptest',
> 'lazr.uri',
> + 'mechanize',

Do we require mechanize? Isn't this an indirect dependency of
zope.testbrowser?

> > 'mocker',
> > 'oauth',
> > 'python-openid',
> 'pytz',
> + # This appears to be a broken indirect dependency from
zope.security:
> + 'RestrictedPython',
> 'setuptools',
> 'sourcecodegen',
> 'storm',
> - 'chameleon.core',
> - 'chameleon.zpt',
> + 'transaction',
> + 'wadllib',
> 'z3c.pt',
> 'z3c.ptcompat',
> - 'wadllib',
> + 'zc.zservertracelog',
> + 'zope.app.appsetup',
> + 'zope.app.component',
> + 'zope.app.dav', # ./package-includes/dav-configure.zcml

Really!?!

> + 'zope.app.error',
> + 'zope.app.exception',
> + 'zope.app.file',
> + 'zope.app.form',
> + 'zope.app.pagetemplate',
> + 'zope.app.pluggableauth',
> + 'zope.app.publication',
> + 'zope.app.publisher',
> + 'zope.app.security',
> + 'zope.app.securitypolicy',
> + 'zope.app.server',
> + 'zope.app.session',
> + 'zope.app.testing',
> + 'zope.app.wsgi',
> + 'zope.app.zapi',
> + 'zope.contenttype',

We use that directly?

> + 'zope.component[zcml]',
> + 'zope.datetime',
> + 'zope.thread',

And this?

> + 'zope.error',
> + 'zope.event',
> + 'zope.exceptions',
> + 'zope.formlib',
> + 'zope.i18n',
> + 'zope.interface',
> + 'zope.hookable', # indirect, via zope.app.component

> + 'zope.lifecycleevent',
> + 'zope.location',
> + 'zope.pagetemplate',
> + 'zope.publisher',
> + 'zope.proxy',
> + 'zope.schema',
> + 'zope.security',
> + 'zope.sendmail',
> + 'zope.server',
> + 'zope.session',
> + 'zope.tal',
> + 'zope.tales',
> + 'zope.testbrowser',
> + 'zope.testing',
> + 'zope.traversing',
> + 'zope.viewlet', # only fixing a broken dependency

Isn't possible to trim that list a little, to contain only what we actually
requires and let the rest be pulled in automatically?

> === modified file 'versions.cfg'
> --- versions.cfg 2009-08-12 12:35:51 +0000
> +++ versions.cfg 2009-08-18 21:42:18 +0000
> @@ -2,17 +2,20 @@
> versions = versions
>
> [versions]
> -# Alphabetical, please! :-)
> +# Alphabetical, case-insensitive, please! :-)
> bzr = 1.17
> chameleon.core = 1.0b35
> chameleon.zpt = 1.0b17
> +ClientForm = 0.2.10
> # Required by Windmill to run on 2.4
> ctypes = 1.0.2
> docutils = 0.5
> elementtree = 1.2.6-20050316
> -# We use a version of feedvalidator that has been changed to not
> -# change the default socket timeout on import.
> -feedvalidator = 0.0.0DEV-r1049-hacked
> +feedvalidator = 0.0.0DEV-r1049
> +# We could use a locally hacked version of feedvalidator that has been
changed
> +# to not change the default socket timeout on import. We are currently
handling
> +# that problem elsewhere.
> +# feedvalidator = 0.0.0DEV-r1049-hacked

Why not simply remove the comments and line altogether?