Thank you, Francis!

Francis and I talked about the review.  On the basis of his comments and the discussion, I did the revisions listed above as well as two further branches: https://code.edge.launchpad.net/~gary/zc.buildout/python-support-5-initial-egg-control and https://code.edge.launchpad.net/~gary/zc.buildout/python-support-6-egg-control .

I'll respond to his comments point by point now.

> === modified file 'CHANGES.txt'
> --- CHANGES.txt	2010-02-17 22:17:12 +0000
> +++ CHANGES.txt	2010-02-17 22:17:12 +0000
> @@ -6,6 +6,25 @@
> 
> New Features:
> 
> +- Buildout can be safely used with a system Python, as long as you use the
> +  new z3c.recipe.scripts recipe to generate scripts and interpreters, rather
> +  than zc.recipe.egg (which is still a fully supported, and simpler, way of
> +  generating scripts and interpreters if you are using a "clean" Python).
> 
> Really, I tohugh zc.recipe.egg would also do the right thing. Can you explain
> the differences to me?

We discussed this.  He meant that he wanted the CHANGES document to more clearly describe the differences.  I tried this:

- Buildout can be safely used with a system Python (or any Python with code
  in site-packages), as long as you use the new z3c.recipe.scripts
  recipe to generate scripts and interpreters, rather than zc.recipe.egg.

  zc.recipe.egg is still a fully supported, and simpler, way of
  generating scripts and interpreters if you are using a "clean" Python,
  without code installed in site-packages. It keeps its previous behavior in
  order to provide backwards compatibility.

> 
> +
> +  A hopefully slight limitation: in no cases are distributions in your
> +  site-packages used to satisfy buildout dependencies.  The
> +  site-packages can be used in addition to the dependencies specified in
> +  your buildout, and buildout dependencies can override code in your
> +  site-packages, but even if your Python's site-packages has the same
> +  exact version as specified in your buildout configuration, buildout
> +  will still use its own copy.
> 
> I assume dependencies related to zc.buildout itself like distribute,
> and setuptools. Not general dependencies managed by buildout, right?

My comment was wrong--and the actual situation turned out to be a big problem, leading to the two subsequent branches I mentioned above.

The actual situation was that dependencies would be obtained from site-packages--even if you said you didn't want to use site-packages.  This is completely broken for the use case of wanting to use a system Python without site-packages.

I fixed this in the later branches.

The later branches mean that one can choose to get general dependencies from site-packages, or not.

> +- Installing a namespace package using a Python that already has a package
> +  in the same namespace (e.g., in the Python's site-packages) failed in
> +  some cases.
> +
> 
> I assume you mean this is fixed. (IOW, that is in a fixed bugs section?)

Right.

> +- Another variation of this error showed itself when at least two
> +  dependencies were in a shared location like site-packages, and the
> +  first one met the "versions" setting.  The first dependency would be
> +  added, but subsequent dependencies from the same location (e.g.,
> +  site-packages) would use the version of the package found in the
> +  shared location, ignoring the version setting.
> +
> 
> I assume the same thing here.

Right.

> === modified file 'README.txt'
> --- README.txt	2010-02-17 22:17:12 +0000
> +++ README.txt	2010-02-17 22:17:12 +0000
> @@ -37,6 +37,11 @@
>    dependencies.  It installs their console-script entry points with
>    the needed eggs included in their paths.
> 
> +`z3c.recipe.scripts <http://pypi.python.org/pypi/z3c.recipe.scripts>`_
> +  This scripts recipe builds interpreter scripts and entry point scripts
> +  based on eggs.  These scripts have more features and flexibility than the
> +  ones offered by zc.recipe.egg.
> +
> 
> Again, might be worthwhile to go into more details of when one should be used
> instead of the other.

I tried to clarify:

`zc.recipe.egg <http://pypi.python.org/pypi/zc.recipe.egg>`_
   The egg recipe installes one or more eggs, with their
   dependencies.  It installs their console-script entry points with
   the needed eggs included in their paths.  It is suitable for use with
   a "clean" Python: one without packages installed in site-packages.

`z3c.recipe.scripts <http://pypi.python.org/pypi/z3c.recipe.scripts>`_
  Like zc.recipe.egg, this recipe builds interpreter scripts and entry
  point scripts based on eggs.  It can be used with a Python that has
  packages installed in site-packages, such as a system Python.  The
  interpreter also has more features than the one offered by
  zc.recipe.egg.

> === modified file 'src/zc/buildout/easy_install.py'
> --- src/zc/buildout/easy_install.py	2010-02-17 22:17:12 +0000
> +++ src/zc/buildout/easy_install.py	2010-02-17 22:17:12 +0000
> @@ -137,9 +137,50 @@
> else:
>     _safe_arg = str
> 
> -_easy_install_cmd = _safe_arg(
> -    'from setuptools.command.easy_install import main; main()'
> -    )
> +# The following string is used to run easy_install in
> +# Installer._call_easy_install.  It is started with python -S (that is,
> +# don't import site at start).  That flag, and all of the code in this
> +# snippet above the last two lines, exist to work around a relatively rare
> +# problem.  If
> +#
> +# - your buildout configuration is trying to install a package that is within
> +#   a namespace package, and
> +#
> +# - you use a Python that has a different version of this package
> +#   installed in in its site-packages using
> +#   --single-version-externally-managed (that is, using the mechanism
> +#   sometimes used by system packagers:
> +#   http://peak.telecommunity.com/DevCenter/setuptools#install-command ), and
> +#
> +# - the new package tries to do sys.path tricks in the setup.py to get a
> +#   __version__,
> +#
> +# then the older package will be loaded first, making the setup version
> +# the wrong number. While very arguably packages simply shouldn't do
> +# the sys.path tricks, some do, and we don't want buildout to fall over
> +# when they do.
> +#
> +# The namespace packages installed in site-packages with
> +# --single-version-externally-managed use a mechanism that cause them to
> +# be processed when site.py is imported.  Simply starting Python with -S
> +# addresses the problem in Python 2.4 and 2.5, but Python 2.6's distutils
> +# imports a value from the site module, so we unfortunately have to do more
> +# drastic surgery in the _easy_install_cmd code below.  The changes to
> +# sys.modules specifically try to only remove namespace modules installed by
> +# the --single-version-externally-managed code.
> +
> +_easy_install_cmd = _safe_arg('''\
> +import sys; \
> +p = sys.path[:]; \
> +m = sys.modules.keys(); \
> +import site; \
> +sys.path[:] = p; \
> +m_attrs = set(('__builtins__', '__file__', '__package__', '__path__')); \
> +match = set(('__path__',)); \
> +[sys.modules.pop(k) for k, v in sys.modules.items()\
> + if k not in m and v and m_attrs.intersection(dir(v)) == match]; \
> +from setuptools.command.easy_install import main; \
> +main()''')
> 
> Ouch, that's nasty :-)

:-) Yes, it is.

> The conditional expression in the list comprehension is pretty arcane to me.
> Could you either rewrite it to make it more intelligible or explain what it
> does in a comment?

I did so.  In the course of preparing it, I found an approach that seemed to make more sense.  The code now reads as follows.

# The following string is used to run easy_install in
# Installer._call_easy_install.  It is started with python -S (that is,
# don't import site at start).  That flag, and all of the code in this
# snippet above the last two lines, exist to work around a relatively rare
# problem.  If
#
# - your buildout configuration is trying to install a package that is within
#   a namespace package, and
#
# - you use a Python that has a different version of this package
#   installed in in its site-packages using
#   --single-version-externally-managed (that is, using the mechanism
#   sometimes used by system packagers:
#   http://peak.telecommunity.com/DevCenter/setuptools#install-command ), and
#
# - the new package tries to do sys.path tricks in the setup.py to get a
#   __version__,
#
# then the older package will be loaded first, making the setup version
# the wrong number. While very arguably packages simply shouldn't do
# the sys.path tricks, some do, and we don't want buildout to fall over
# when they do.
#
# The namespace packages installed in site-packages with
# --single-version-externally-managed use a mechanism that cause them to
# be processed when site.py is imported  (see
# http://mail.python.org/pipermail/distutils-sig/2009-May/011730.html
# for another description of the problem).  Simply starting Python with
# -S addresses the problem in Python 2.4 and 2.5, but Python 2.6's
# distutils imports a value from the site module, so we unfortunately
# have to do more drastic surgery in the _easy_install_cmd code below.
#
# Here's an example of the .pth files created by setuptools when using that
# flag:
#
# import sys,new,os;
# p = os.path.join(sys._getframe(1).f_locals['sitedir'], *('<NAMESPACE>',));
# ie = os.path.exists(os.path.join(p,'__init__.py'));
# m = not ie and sys.modules.setdefault('<NAMESPACE>',new.module('<NAMESPACE>'));
# mp = (m or []) and m.__dict__.setdefault('__path__',[]);
# (p not in mp) and mp.append(p)
#
# The code, below, then, runs under -S, indicating that site.py should
# not be loaded initially.  It gets the initial sys.path under these
# circumstances, and then imports site (because Python 2.6's distutils
# will want it, as mentioned above). It then reinstates the old sys.path
# value. Then it removes namespace packages (created by the setuptools
# code above) from sys.modules.  It identifies namespace packages by
# iterating over every loaded module.  It first looks if there is a
# __path__, so it is a package; and then it sees if that __path__ does
# not have an __init__.py.  (Note that PEP 382,
# http://www.python.org/dev/peps/pep-0382, makes it possible to have a
# namespace package that has an __init__.py, but also should make it
# unnecessary for site.py to preprocess these packages, so it should be
# fine, as far as can be guessed as of this writing.)  Finally, it
# imports easy_install and runs it.

_easy_install_cmd = _safe_arg('''\
import sys,os;\
p = sys.path[:];\
import site;\
sys.path[:] = p;\
[sys.modules.pop(k) for k, v in sys.modules.items()\
 if hasattr(v, '__path__') and len(v.__path__)==1 and\
 not os.path.exists(os.path.join(v.__path__[0],'__init__.py'))];\
from setuptools.command.easy_install import main;\
main()''')

> def scripts(reqs, working_set, executable, dest,
>             scripts=None,
>             extra_paths=(),
> @@ -912,20 +956,85 @@
>             initialization='',
>             relative_paths=False,
>             ):
> -
> +    """Generate scripts and/or an interpreter.
> +
> 
> +def generate_scripts(
> +    dest, working_set, executable, site_py_dest,
> +    reqs=(), scripts=None, interpreter=None, extra_paths=(),
> +    initialization='', add_site_packages=False, exec_sitecustomize=False,
> +    relative_paths=False, script_arguments='', script_initialization=''):
> +    """Generate scripts and/or an interpreter.
> 
> 
> 
> I find the fact that there is scripts and generate_scripts that share exactly
> the same first line of docstring confusing.
> 
> How about scripts() and sitepackage_safe_scripts()? (If you can't rename
> scripts() for API compatibility)

API compatibility was in fact what I was trying to maintain.

I understood your point, and your specific suggestion grew on me until I adopted it: it is now named sitepackage_safe_scripts.  The doc strings now begin differently as well:

def scripts(...):
    """Generate scripts and/or an interpreter.

    See sitepackage_safe_scripts for a version that can be used with a Python
    that has code installed in site-packages. It has more options and a
    different approach.
    """

...

def sitepackage_safe_scripts(...):
    """Generate scripts and/or an interpreter from a system Python.

    This accomplishes the same job as the ``scripts`` function, above,
    but it does so in an alternative way that allows safely including
    Python site packages, if desired, and  choosing to execute the Python's
    sitecustomize.
    """

...

> def _relative_depth(common, path):
> +    """Return number of dirs separating ``path`` from ancestor, ``common``.
> +
> +    For instance, if path is /foo/bar/baz/bing, and common is /foo, this will
> +    return 2--in UNIX, the number of ".." to get from bing's directory
> +    to foo.
> +
> +    This is a helper for _relative_path_and_setup.
> +    """
>     n = 0
>     while 1:
>         dirname = os.path.dirname(path)
> @@ -993,6 +1112,11 @@
>     return n
> 
> def _relative_path(common, path):
> +    """Return the relative path from ``common`` to ``path``.
> +
> +    This is a helper for _relativitize, which is a helper to
> +    _relative_path_and_setup.
> +    """
>     r = []
>     while 1:
>         dirname, basename = os.path.split(path)
> 
> Thanks for addint the docstrings btw!

:-) They helped me, too.

> 
> 
> +# These are used only by the newer ``generate_scripts`` function.
> +
> +def _get_system_paths(executable):
> +    """return lists of standard lib and site paths for executable.
> +    """
> 
> s/return/Return/

Changed.

> +
> +def _generate_site(dest, working_set, executable, extra_paths=(),
> +                   add_site_packages=False, relative_paths=False):
> +    """Write a site.py file with eggs from working_set.
> +
> +    extra_paths will be added to the path.  If add_site_packages is True,
> +    paths from the underlying Python will be added.
> +    """
> 
> +    site = open(site_path, 'w')
> +    try:
> +        for line in real_site.readlines():
> +            if line.startswith(enableusersite_marker):
> +                site.write(enableusersite_marker)
> +                site.write('False # buildout does not support user sites.\n')
> +            elif line.startswith(addsitepackages_marker):
> +                site.write(addsitepackages_script % (
> +                    preamble, egg_path_string, original_path_setup))
> +                site.write(line[len(addsitepackages_marker):])
> +                successful_rewrite = True
> +            else:
> +                site.write(line)
> 
> Is it just me or this looks like fragile? Copying site.py and modifying based
> on two markers? Not that I have a better solution though :-)

Right.  We discussed the alternatives, and agreed that this was a reasonable choice from a bad lot.  On the bright side, it does work on Py 2.4, 2.5 and 2.6 (*NIX), and on Windows (2.5 tested).

> === modified file 'src/zc/buildout/easy_install.txt'
> 
> +
> +The ``generate_scripts`` function: using site-packages
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The ``generate_scripts`` function supports including site packages.  This has
> +some advantages and some serious dangers.
> +
> +A typical reason to include site-packages is that it is easier to
> +install one or more dependencies in your Python than it is with
> +buildbot.  Some packages, such as lxml or Python PostgreSQL integration,
> 
> buildout :-)

Heh.  There were actually several of these typos.  I found and squished them.

> === added file 'z3c.recipe.scripts_/src/z3c/recipe/scripts/README.txt'
> 
> +In addition to these, the recipe offers these new options.  They are
> +introduced here, and described more in depth below.
> +
> +add-site-packages
> +    You can choose to have the site-packages of the underlying Python
> +    available to your script or interpreter, in addition to the packages
> +    from your eggs.  See the section on this option for motivations and
> +    warnings.
> +
> +extends
> +    You can extend another section using this value.  It is intended to be
> +    used by extending a section that uses this package's scripts recipe.
> +    In this manner, you can avoid repeating yourself.
> +
> +exec-sitecustomize
> +    Normally the Python's real sitecustomize module is not processed.
> +    If you want it to be processed, set this value to 'true'.  This will
> +    be honored irrespective of the setting for include-site-paths.
> 
> I don't find include-site-paths? Do you mean add-site-packages?

I did mean that.

In this branch, I normalized on add-site-packages.

In the next branch, I changed this to include-site-packages, because I thought it was clearer in some uses.

> +Including site-packages and sitecustomize
> +-----------------------------------------
> +
> +As introduced above, this recipe supports including site packages.  This has
> +some advantages and some serious dangers.
> +
> +A typical reason to include site-packages is that it is easier to
> +install one or more dependencies in your Python than it is with
> +buildbot.  Some packages, such as lxml or Python PostgreSQL integration,
> +have dependencies that can be much easier to build and/or install using
> +other mechanisms, such as your operating system's package manager.  By
> +installing some core packages into your Python's site-packages, this can
> +significantly simplify some application installations.
> +
> +However, doing this has a significant danger.  One of the primary goals
> +of buildout is to provide repeatability.  Some packages (one of the
> +better known Python openid packages, for instance) change their behavior
> +depending on what packages are available.  If Python curl bindings are
> +available, these may be preferred by the library.  If a certain XML
> +package is installed, it may be preferred by the library.  These hidden
> +choices may cause small or large behavior differences.  The fact that
> +they can be rarely encountered can actually make it worse: you forget
> +that this might be a problem, and debugging the differences can be
> +difficult.  If you allow site-packages to be included in your buildout,
> +and the Python you use is not managed precisely by your application (for
> +instance, it is a system Python), you open yourself up to these
> +possibilities.  Don't be unaware of the dangers.
> 
> Copy and paste alert! That's identical to some of the other doctest content.
> Not sure if you care :-)

:-) If you don't, I don't. :-) It was intentional.  They are for two different packages.  In one case, I'm warning developers about a zc.buildout API.  In the other, I'm warning recipe users about the brhavior of the z3c.recipe.scripts recipe.  Since the API underlies the recipe, the content is the same, but I felt the different contexts made the repetition defensible and even desirable.

> +
> +The other options all identical to the zc.recipe.egg script.  Here are some
> +quick demos and discussions.
> 
> No, there aren't!

Heh, good call.  I removed the last sentence.

> 
> === added file 'z3c.recipe.scripts_/src/z3c/recipe/scripts/tests.py'
> --- z3c.recipe.scripts_/src/z3c/recipe/scripts/tests.py	1970-01-01 00:00:00 +0000
> +++ z3c.recipe.scripts_/src/z3c/recipe/scripts/tests.py	2010-02-17 22:17:12 +0000
> @@ -0,0 +1,293 @@
> +##############################################################################
> +#
> +# Copyright (c) 2006 Zope Corporation and Contributors.
> +# All Rights Reserved.
> +#
> +# This software is subject to the provisions of the Zope Public License,
> +# Version 2.1 (ZPL).  A copy of the ZPL should accompany this distribution.
> +# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED
> +# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS
> +# FOR A PARTICULAR PURPOSE.
> +#
> +##############################################################################
> +
> +import os, re, shutil, sys
> +import zc.buildout.tests
> +import zc.buildout.testselectingpython
> +import zc.buildout.testing
> +
> +import unittest
> +from zope.testing import doctest, renormalizing
> +
> +# We do not explicitly test the recipe support for the ``eggs``,
> +# ``find-links``, and ``index`` options because they are used for most or
> +# all of the examples.  The README tests ``extends``,
> +# ``include-site-customization`` and ``name``.  That leaves ``python``,
> +# ``extra-paths``, ``initialization``, ``relative-paths``, and
> +# ``include-site-packages``.
> 
> add-site-packages

Changed, though changed back to include-site-packages in a later branch, as I mentioned above.

> === modified file 'zc.recipe.egg_/src/zc/recipe/egg/README.txt'
> 
> In terms of improving splitting of work, the options refactoring in 
> zc.recipe.egg could have gone with the previous branch. Would have make this
> one a little lighter :-)

Good point.  Jim says it is not necessary to divide things up further for his review, though, so I won't (though I'll remember it for the future).

Thank you again!

Gary