Code review comment for lp://staging/~gary/zc.buildout/support-system-python

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>