Merge lp://staging/~gary/zc.buildout/support-system-python into lp://staging/~gary/zc.buildout/oldtrunk

I have made a number of changes to zc.buildout in the gary-support-system-python branch in zope.org's svn repository. They all support Launchpad or Landscape's use of Buildout with a system Python, directly or indirectly.

The following is a list of changes taken from my CHANGES.txt additions:

- Support and bugfixes for using a system Python with buildout.

  In all of these descriptions, "site-packages" is an imprecise term for a
  precise definition: the packages that are added by the Python's site.py.
  Practically, this is the difference of the set of paths normally used by
  Python minus those used when Python is started with the -S flag.

  * A new boolean option, 'include-site-packages', includes or excludes site
    packages from finding requirements, and from generated scripts.
    zc.buildout's own buildout.cfg dogfoods this option.

  * Script generation pushes dependency paths that are in site-packages to
    the end of the dependency paths in sys.path (but, as before, these are
    still before extra paths, the standard library, and the rest of the
    site-package paths).

  * Fix an error 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.

- The generated interpreter scripts now have a few more similarities to a
  real Python interpreter.

  * __file__ is set correctly for executed files.

  * -c incorporates all subsequent arguments as part of the command.

  * -V returns the version.

  * -S causes the script to not modify the standard path (for tests)

- Improve bootstrap.

  * New options let you specify where to find ez_setup.py and where to find
    a download cache. These options can keep bootstrap from going over the
    network.

  * Another new option lets you specify where to put generated eggs.

  * The buildout script generated by bootstrap honors more of the settings
    in the designated configuration file (e.g., buildout.cfg).

I failed to implement one feature I had wanted: a way to cherrypick parts of a system Python's site-packages. I proposed an approach, but it ugly, fragile, and risky; and Jim proposed an approach, but it does not work for a modern Ubuntu.

Review requests for small changes to zc.recipe.testrunner and z3c.recipe.filetemplate, based on some of these changes, are also forthcoming.

I effectively reverted a change I had made to bootstrap.py that tried to get rid of the is_jython code. I had thought we could rely on subprocess generally, if it exists, but I'm told (by Sidnei, thanks to him) that my version of bootstrap failed on Windows, but changing it back to use spawnle worked. Therefore, I've changed the branch back to use spawnle unless is_jython. I still suspect that this could be done more elegantly, but solving that is not a goal I want for this branch.

The following is the incremental diff for this change.

Thanks.

Gary

=== modified file 'bootstrap/bootstrap.py'
--- bootstrap/bootstrap.py 2009-07-09 23:55:19 +0000
+++ bootstrap/bootstrap.py 2009-07-13 14:03:35 +0000
@@ -139,16 +139,17 @@
     os.environ,
     PYTHONPATH=ws.find(pkg_resources.Requirement.parse('setuptools')).location)

-try:
+is_jython = sys.platform.startswith('java')
+if is_jython:
     import subprocess
-except ImportError:
+ exitcode = subprocess.Popen(cmd, env=env).wait()
+else: # Windows needs this, apparently; otherwise we would prefer subprocess
     exitcode = os.spawnle(*([os.P_WAIT, sys.executable] + cmd + [env]))
-else:
- # Jython can use subprocess but not spawn. We prefer it generally.
- exitcode = subprocess.Popen(cmd, env=env).wait()
 if exitcode != 0:
- # we shouldn't need an error message because a failure
- # should have generated a visible traceback in the subprocess.
+ sys.flush()
+ print ("An error occured when trying to install zc.buildout. "
+ "Look above this message for any errors that "
+ "were output by easy_install.")
     sys.exit(exitcode)

 ws.add_entry(configuration['--eggs'])

=== modified file 'src/zc/buildout/easy_install.py'
--- src/zc/buildout/easy_install.py 2009-07-11 15:49:15 +0000
+++ src/zc/buildout/easy_install.py 2009-07-13 14:03:35 +0000
@@ -419,8 +419,8 @@

             if exit_code:
                 logger.error(
- "An error occured when trying to install %s."
- "Look above this message for any errors that"
+ "An error occured when trying to install %s. "
+ "Look above this message for any errors that "
                     "were output by easy_install.",
                     dist)

On July 11, 2009, Gary Poster wrote:
> Gary Poster has proposed merging lp:~gary/zc.buildout/support-system-python
> into lp:~gary/zc.buildout/trunk.
>
> Requested reviews:
> Francis J. Lacoste (flacoste)
>
> I have made a number of changes to zc.buildout in the
> gary-support-system-python branch in zope.org's svn repository. They all
> support Launchpad or Landscape's use of Buildout with a system Python,
> directly or indirectly.
>

Wow, this was a lot of changes!

From my cursory understanding, these changes looks good. I have a bunch of
questions / suggestions. So from the Launchpad point of view, it seems to
satisfy our use case.

Thanks a lot, this will make zc.buildout a lot more robust for our use-case.

> +# We have to manually parse our options rather than using one of the stdlib
> +# tools because we want to pass the ones we don't recognize along to
> +# zc.buildout.buildout.main.

Wouldn't it have been possible to use a '--' marker to delimit the bootstrap
options from the buildout ones:

  ./bootstrap.py --eggs eggs -- ZC.buildout options follows.

That's a common idiom I think.

> +match_equals = re.compile(r'(%s)=(\S*)' % ('|'.join(configuration),)).match

By using this regex, you preclude any paths with spaces in them. I know many
people avoid them because a lot of UNIX programs break on them, but I'm sure
you'll encounter a lot of Mac OS X users that have spaces in some of their
paths :-)

> +# defaults
> +tmpeggs = None

The above comment doesn't make sense to me.

> +if (configuration['--download-base'] and
> + not configuration['--download-base'].endswith('/')):
> + # download base needs a trailing slash to make the world happy
> + configuration['--download-base'] += '/'
> +

I know this is something specific to our coding conventions, but comments are
sentences needing a capital and a period ;-)

> +if configuration['--version']:
> + configuration['--version'] = '==' + configuration['--version']

I don't understand the reason for that code? I'm guessing it's because you are
going to pass it over to setuptools? Maybe add a comment?

> === modified file 'src/zc/buildout/buildout.py'
> - prefer_final = options.get('prefer-final', 'false')
> + prefer_final = options.setdefault('prefer-final', 'false')

Is there any reason for the switch to setdefault? If it's because you are
reusing options in other places, then I guess that's fine. Otherwise, I know
some people find using setdefault() with it's return value a little confusing
to read.

> + options = self['buildout']
> +
> + # Get a base working set for our distributions that corresponds to
the
> + # stated desires in the configuration.
> + distributions = ['setuptools', 'zc.buildout']

You have trailing white space on that previous line.

> === modified file 'src/zc/buildout/buildout.txt'

I like the merging of the sections into the general option list. It might make
them harder to find from the table of contents though.

> === modified file 'src/zc/buildout/easy_install.py'
> +
> +def _get_system_packages(executable):
> + """return a pair of the standard lib and site packages for the
executable.
...

On Jul 13, 2009, at 5:06 PM, Francis J. Lacoste wrote:

> On July 11, 2009, Gary Poster wrote:
>> Gary Poster has proposed merging lp:~gary/zc.buildout/support-
>> system-python
>> into lp:~gary/zc.buildout/trunk.
>>
>> Requested reviews:
>> Francis J. Lacoste (flacoste)
>>
>> I have made a number of changes to zc.buildout in the
>> gary-support-system-python branch in zope.org's svn repository.
>> They all
>> support Launchpad or Landscape's use of Buildout with a system
>> Python,
>> directly or indirectly.
>>
>
> Wow, this was a lot of changes!

Yeah, sorry.

>> From my cursory understanding, these changes looks good. I have a
>> bunch of
> questions / suggestions. So from the Launchpad point of view, it
> seems to
> satisfy our use case.
>
> Thanks a lot, this will make zc.buildout a lot more robust for our
> use-case.
>
>> +# We have to manually parse our options rather than using one of
>> the stdlib
>> +# tools because we want to pass the ones we don't recognize along to
>> +# zc.buildout.buildout.main.
>
> Wouldn't it have been possible to use a '--' marker to delimit the
> bootstrap
> options from the buildout ones:
>
> ./bootstrap.py --eggs eggs -- ZC.buildout options follows.
>
> That's a common idiom I think.

It is, but it would break backwards compatibility of the usage of this
script. This was a design decision from before I came a long. If Jim
were OK with breaking backwards compatibility I'd be happy to go this
way.

>> +match_equals = re.compile(r'(%s)=(\S*)' %
>> ('|'.join(configuration),)).match
>
> By using this regex, you preclude any paths with spaces in them. I
> know many
> people avoid them because a lot of UNIX programs break on them, but
> I'm sure
> you'll encounter a lot of Mac OS X users that have spaces in some of
> their
> paths :-)

Yes, that was silly, thanks. (.*) should be sufficient. See
incremental diff.

>
>> +# defaults
>> +tmpeggs = None
>
> The above comment doesn't make sense to me.

Agreed. It was a bit of a legacy comment--at one point in development
I had set a lot of default values there. Now there is only one. I
removed "default" and added a more pertinent comment. See incremental
diff.

>
>> +if (configuration['--download-base'] and
>> + not configuration['--download-base'].endswith('/')):
>> + # download base needs a trailing slash to make the world happy
>> + configuration['--download-base'] += '/'
>> +
>
> I know this is something specific to our coding conventions, but
> comments are
> sentences needing a capital and a period ;-)

Ack, see diff.

>
>> +if configuration['--version']:
>> + configuration['--version'] = '==' + configuration['--version']
>
> I don't understand the reason for that code? I'm guessing it's
> because you are
> going to pass it over to setuptools? Maybe add a comment?

This is legacy code to some degree, but yes. I changed this approach
entirely to try to clarify, and added a comment.

> === modified file 'src/zc/buildout/buildout.py'
>> - prefer_final = options.get('prefer-final', 'false')
>> + prefer_final = options.setdefault('prefer-final', 'false')
>
> Is there any reason for th...

On July 13, 2009, Gary Poster wrote:
> On Jul 13, 2009, at 5:06 PM, Francis J. Lacoste wrote:
> > On July 11, 2009, Gary Poster wrote:
> >> Gary Poster has proposed merging lp:~gary/zc.buildout/support-
> >> system-python
> >> into lp:~gary/zc.buildout/trunk.
> >>
> >> Requested reviews:
> >> Francis J. Lacoste (flacoste)
> >>
> >> I have made a number of changes to zc.buildout in the
> >> gary-support-system-python branch in zope.org's svn repository.
> >> They all
> >> support Launchpad or Landscape's use of Buildout with a system
> >> Python,
> >> directly or indirectly.
> >
> > Wow, this was a lot of changes!
>
> Yeah, sorry.
>
> >> From my cursory understanding, these changes looks good. I have a
> >> bunch of
> >
> > questions / suggestions. So from the Launchpad point of view, it
> > seems to
> > satisfy our use case.
> >
> > Thanks a lot, this will make zc.buildout a lot more robust for our
> > use-case.
> >
> >> +# We have to manually parse our options rather than using one of
> >> the stdlib
> >> +# tools because we want to pass the ones we don't recognize along to
> >> +# zc.buildout.buildout.main.
> >
> > Wouldn't it have been possible to use a '--' marker to delimit the
> > bootstrap
> > options from the buildout ones:
> >
> > ./bootstrap.py --eggs eggs -- ZC.buildout options follows.
> >
> > That's a common idiom I think.
>
> It is, but it would break backwards compatibility of the usage of this
> script. This was a design decision from before I came a long. If Jim
> were OK with breaking backwards compatibility I'd be happy to go this
> way.
>

I see, it's fine like it is then.

> > === modified file 'src/zc/buildout/buildout.py'
> >
> >> - prefer_final = options.get('prefer-final', 'false')
> >> + prefer_final = options.setdefault('prefer-final', 'false')
> >
> > Is there any reason for the switch to setdefault? If it's because
> > you are
> > reusing options in other places, then I guess that's fine.
> > Otherwise, I know
> > some people find using setdefault() with it's return value a little
> > confusing
> > to read.
>
> It is because when buildout uses -vv, it is supposed to show you all
> configuration values, as shown in buildout.txt in the "Predefined
> buildout options" section. To appear in this output, they need to be
> in the options dict. Therefore, when appropriate, I switched
> from .get to .setdefault so that the options would get the default.
> It seemed like a reasonable, concise, and idiomatic way of
> accomplishing this. Otherwise, we'd need something like this.
>
> prefer_final = options.get('prefer-final', 'false')
> options['prefer-final'] = prefer_final
>
> I prefer the setdefault approach, but I'm happy to change.

No, setdefault is fine then.

> >
> >> + # one minus the set of the ones in ``python -S`` is the set of
> >> packages
> >> + # that are effectively site-packages.
> >> + def get_sys_path(clean=False):
> >> + cmd = [executable, "-c",
> >> + "import sys, os;"
> >> + "print repr([os.path.normpath(p) for p in
> >> sys.path])"]
> >> + if clean:
> >> + cmd.insert(1, '-S')
> >> + _proc = subprocess.Popen(
> >...

So, I have a, um, 1500+ line additional diff. This further breaks our review policies, which might mean we should think further about how to do these upstream-project reviews in the future. But meanwhile...

https://pastebin.canonical.com/20796/

It was developed to prepare some previous bugfixes for upstream review by adding tests, and to address and give diagnostic tools for bug 407408 ( https://bugs.launchpad.net/launchpad-foundations/+bug/407408 ).

This diff does the following:

- Add new option and tests: ``allowed-eggs-from-site-packages`` allows you
  to specify a glob-aware whitelist of project names that may come from
  site-packages. This defaults to '*', meaning there is no filtering.
- Add feature and test to let you see what eggs have been selected from
  site-packages: "Egg from site-packages: ..." in the output of
  ``bin/buildout -v``.
- Add new option and tests: ``include-site-packages-for-buildout`` allows
  you to control if site-packages are used for the bin/buildout script.
  This defaults to "false," which is a change in behavior! (Rationale is in
  docs.)
- Standardize on one of the several competing approaches for default options
  in the [buildout] section. This removes my previous use of setdefault,
  discussed with flacoste.
- Make generated script paths prettier by eliminating duplicate paths.
- Add comment about recipes using buildout options, not buildout attributes.
  Practice what I preach in zc.recipe.egg.
- Standardize on one approach for handling bool options in zc.recipe.egg. Not
  sure I like it but it is preexisting, and now it is consistent, and it
  behaves well in terms of letting buildout be configured before it looks too
  hard at it.
- Normalize tests.py function docstrings to begin at col 0
- Correct some typos

On August 5, 2009, Gary Poster wrote:
> So, I have a, um, 1500+ line additional diff. This further breaks our
> review policies, which might mean we should think further about how to do
> these upstream-project reviews in the future. But meanwhile...
>
> https://pastebin.canonical.com/20796/

Hi Gary,

Thanks for fixing this. Hopefully, this is the last changes we need to support
our use case properly. A good way to split a branch like this is to start by
doing all the clean-ups you want to do first (option changes, reformmating,
etc) and then do your actual changes. Keep that in mind for another time.

I don't have much comments, so feel free to land this.

> === modified file 'CHANGES.txt'
> --- CHANGES.txt 2009-07-11 17:15:31 +0000
> +++ CHANGES.txt 2009-08-04 23:57:12 +0000
> @@ -13,13 +13,28 @@
>
> * A new boolean option, 'include-site-packages', includes or excludes
site
> packages from finding requirements, and from generated scripts.
> - zc.buildout's own buildout.cfg dogfoods this option.
> + zc.buildout's own buildout.cfg dogfoods this option. This defaults
> + to 'true', which is very similar to buildout's previous behavior.
> +
> + * A new boolean option, 'include-site-packages-for-buildout', does the
> + same thing but only for the bin/buildout script. This can be important
> + for getting recipes and their dependencies without conflicts. This
> + defaults to 'false', which is different from buildout's previous
behavior.
> +
> + * Another new option, 'allowed-eggs-from-site-packages', lets you specify
> + a whitelist of project names of eggs that are allowed to come from
> + your Python's site-packages. This lets you more tightly control your
use
> + of site-packages.
>

You should explain how this option interacts with include-site-packages. If
include-site-packages is False, does it overrides it? Or, is it only used if
include-site-packages is True?

I see that you explained this in the documentation, although you didn't
explain how it interacted with the buildout variant.

> === modified file 'src/zc/buildout/tests.py'
> +def allowed_eggs_from_site_packages_option():
> + """
> +As introduced in the previous test, the allowed-eggs-from-site-packages
option
> +allows you to specify a whitelist of project names that may be included
from
> +site-packages.
> +
> +This test shows the option being used in a buildout.
> +
> +The buildout defaults to a whitelist of ('*',), or any project name.
> +

Is it really necessary here to duplicate all the options you already tested
directly using the API?

--
Francis J. Lacoste
<email address hidden>