Code review comment for lp:~gary/zc.buildout/python-support-8-support-subprocess

> === modified file 'bootstrap/bootstrap.py'
> +# In order to be more robust in the face of system Pythons, we want to run
> +# with site-packages loaded. This is somewhat tricky, in particular because

With or without? It seems that you want to run with it, but remove any all
namespace from it? Anyway the comment could be clearer given the
non-obviousness of the code.

> +# Python 2.6's distutils imports site, so starting with the -S flag is not
> +# sufficient.
> +if 'site' in sys.modules:
> + # We will restart with python -S.
> + args = sys.argv[:]
> + args[0:0] = [sys.executable, '-S']
> + args = map(quote, args)
> + os.execv(sys.executable, args)
> +clean_path = sys.path[:]
> +import site
> +sys.path[:] = clean_path
> +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'))):
> + # This is a namespace package. Remove it.
> + sys.modules.pop(k)
> +

> === modified file 'dev.py'
> +# In order to be more robust in the face of system Pythons, we want to run
> +# with site-packages loaded. This is somewhat tricky, in particular because
> +# Python 2.6's distutils imports site, so starting with the -S flag is not
> +# sufficient.

Since this is copy and paste from the other location, my other comment applies
here also.

> +if 'site' in sys.modules:
> + # We will restart with python -S.
> + args = sys.argv[:]
> + args[0:0] = [sys.executable, '-S']
> + args = map(quote, args)
> + os.execv(sys.executable, args)
> +clean_path = sys.path[:]
> +import site
> +sys.path[:] = clean_path
> +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'))):
> + # This is a namespace package. Remove it.
> + sys.modules.pop(k)
> +
> is_jython = sys.platform.startswith('java')

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

> - args.insert(0, zc.buildout.easy_install._safe_arg (sys.executable))
> -
> + args.insert(0, zc.buildout.easy_install._safe_arg(sys.executable))
> + env = os.environ.copy()
> + env['PYTHONPATH'] = partsdir
> if is_jython:
> - sys.exit(subprocess.Popen([sys.executable] + list(args)).wait())
> + sys.exit(
> + subprocess.Popen(
> + [sys.executable] + list(args), env=env).wait())
> else:
> - sys.exit(os.spawnv(os.P_WAIT, sys.executable, args))
> + sys.exit(os.spawnve(os.P_WAIT, sys.executable, args, env))
>

The intent here is to run the script with only partsdir in the path?

> === modified file 'src/zc/buildout/easy_install.py'
> --- src/zc/buildout/easy_install.py 2010-03-19 16:04:20 +0000
> +++ src/zc/buildout/easy_install.py 2010-03-19 16:04:20 +0000
> @@ -99,6 +99,7 @@
> "print repr([os.path.normpath(p) for p in sys.path if p])"])
> # Windows needs some (as yet to be determined) part of the real env.
> env = os.environ.copy()
> + env.pop('PYTHONPATH', None)

Care to explain why you are removing PYTHONPATH?

> env.update(kwargs)
> _proc = subprocess.Popen(
> cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env)

> + env = os.environ.copy()
> + env.pop('PYTHONPATH', None)
> _proc = subprocess.Popen(
> - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> + cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env)
> stdout, stderr = _proc.communicate();
> if _proc.returncode:
> logger.info(

Again, I'm not sure why I understand PYTHONPATH is removed.

>
> === modified file 'src/zc/buildout/tests.py'

>
> +def subprocesses_have_same_environment_by_default():
> + """
> +The scripts generated by sitepackage_safe_scripts set the PYTHONPATH so that,
> +if the environment is maintained (the default behavior), subprocesses get
> +the same Python packages.
> +

Do you need a test that shows that any PYTHONPATH is appended to the existing
set?