On Aug 18, 2009, at 5:57 PM, Francis J. Lacoste wrote:

> Review: Approve
> 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!

Thanks!

Responses below.  I deleted the sections that I simply changed as you  
suggested.

...

>> === 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.

I *think* this must be a conflict resolution mistake of mine.  I  
removed the line again, and ec2test will tell me whether it is OK.

>
>> === 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.

The reason why was that there were many tests that were still able to  
run.  All of the ones that do not rely on the retry stuff still are  
registered.  Is that not worthwhile for some reason?

>> === modified file 'lib/canonical/testing/tests/test_mockdb.py'
>
> Same kind of comments here.

Likewise. :-)

>> === 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?

We have it here because of testbrowser, but we import it directly.

Here and in the other usage lists below I show you the uses I found  
quickly today, reviewing your questions.  There may be more.

lib/lp/code/browser/tests/test_product.py:11:from mechanize import  
LinkNotFoundError
./lib/canonical/launchpad/pagetests/standalone/xx-beta-testers- 
redirection.txt:137:    >>> from mechanize._clientcookie import Cookie
./lib/lp/translations/stories/standalone/xx-sourcepackage-export.txt: 
31:    >>> from mechanize import LinkNotFoundError
./lib/lp/soyuz/stories/ppa/xx-ppa-files.txt:127:    >>> from mechanize  
import LinkNotFoundError

>
>>>         '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!?!

Yeah, see that dav-configure.zcml thing.  That's why the comment is  
there.  Do you want me to try removing that file and then removing the  
dependency?

Unless you are sure we can get rid of that zcml file, I think I'd  
prefer to make a bug for this, asking to investigate, and just leave  
things as they are.

>
>> +        '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?

Yeah, I tried to be pretty careful with these.

lib/canonical/launchpad/scripts/sftracker.py:45:from zope.contenttype  
import guess_content_type
lib/canonical/lazr/folder.py:17:from zope.contenttype import  
guess_content_type
lib/canonical/widgets/image.py:10:from zope.contenttype import  
guess_content_type

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

lib/canonical/launchpad/webapp/launchbag.py:16:from zope import thread

>> +        '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?

Like I said, unless there's a comment about broken dependencies, we're  
using it directly AFAIK.  I didn't review the list again, but I was  
careful at the time.

>> === 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?

'Cause I didn't know if what I had done was a good idea. :-) I removed  
the comments now.

Here's the incremental diff so far.  Please let me know what you think  
about my replies to the other stuff, particularly the test_mockdb bits.

Thank you!

Gary



=== modified file 'buildout-templates/_pythonpath.py.in'
--- buildout-templates/_pythonpath.py.in	2009-08-07 13:16:30 +0000
+++ buildout-templates/_pythonpath.py.in	2009-08-21 17:55:58 +0000
@@ -12,9 +12,9 @@
  # Enable Storm's C extensions
  os.environ['STORM_CEXTENSIONS'] = '1'

-# we don't want to bother tests or logs with these.
+# We don't want to bother tests or logs with these.
  import warnings
  warnings.filterwarnings(
      'ignore',
      'Module .+ was already imported from .+, but .+ is being added.*',
-    UserWarning)
\ No newline at end of file
+    UserWarning)

=== modified file 'buildout.cfg'
--- buildout.cfg	2009-08-05 19:05:57 +0000
+++ buildout.cfg	2009-08-21 18:20:33 +0000
@@ -56,7 +56,8 @@
  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.
+                 # XXX 2009-08-21 gary bug 417077
+                 # This can hopefully be removed when Twisted is used  
as an egg.
                   import warnings
                   warnings.filterwarnings(
                   'ignore',

=== modified file 'lib/canonical/config/__init__.py'
--- lib/canonical/config/__init__.py	2009-08-05 18:52:52 +0000
+++ lib/canonical/config/__init__.py	2009-08-21 18:22:07 +0000
@@ -195,8 +195,6 @@

      def _setZConfig(self):
          """Modify the config, adding automatically generated  
settings"""
-        self.root = TREE_ROOT
-
          schemafile = pkg_resources.resource_filename(
              'zope.app.server', 'schema.xml')
          schema = ZConfig.loadSchema(schemafile)

=== modified file 'versions.cfg'
--- versions.cfg	2009-08-21 17:55:25 +0000
+++ versions.cfg	2009-08-21 18:42:03 +0000
@@ -16,10 +12,6 @@
  docutils = 0.5
  elementtree = 1.2.6-20050316
  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
  functest = 0.8.7
  funkload = 1.10.0
  httplib2 = 0.4.0