Merge into trunk : python-support-8-support-subprocess : Code : Buildout

Status:	Needs review
Proposed branch:	lp://staging/~gary/zc.buildout/python-support-8-support-subprocess
Merge into:	lp://staging/zc.buildout
Prerequisite:	lp://staging/~gary/zc.buildout/python-support-7
Diff against target:	644 lines (+324/-65) (has conflicts) 8 files modified bootstrap/bootstrap.py (+33/-10) buildout.cfg (+3/-3) dev.py (+43/-3) src/zc/buildout/buildout.py (+40/-19) src/zc/buildout/easy_install.py (+26/-8) src/zc/buildout/easy_install.txt (+11/-1) src/zc/buildout/tests.py (+149/-5) src/zc/buildout/update.txt (+19/-16) Text conflict in CHANGES.txt Text conflict in src/zc/buildout/buildout.py Text conflict in src/zc/buildout/buildout.txt Text conflict in src/zc/buildout/easy_install.py Text conflict in src/zc/buildout/tests.py
To merge this branch:	bzr merge lp://staging/~gary/zc.buildout/python-support-8-support-subprocess
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Francis J. Lacoste (community)		2010-03-19	Approve on 2010-03-19
Review via email: mp+21732@code.staging.launchpad.net

Description of the change

This branch does two things.

1) It makes it simple to start Python processes from scripts started with the new recipe: PYTHONPATH is set so that everything works by default.

2) It makes bootstrap.py (and dev.py) yet more robust in the face of system Pythons.

#1 affects #2.

Revision history for this message

Gary Poster (gary) wrote on 2010-03-19:

#

The conflicts in this branch are resolved in https://code.edge.launchpad.net/~gary/zc.buildout/python-support (see https://code.edge.launchpad.net/~gary/zc.buildout/python-support/+merge/21733 ).

Reply

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2010-03-19:

#

Download full text (4.1 KiB)

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

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

review: Needs Information

Reply

lp://staging/~gary/zc.buildout/python-support-8-support-subprocess updated on 2010-03-19

564. By Gary Poster on 2010-03-19: add explanatory comments; extend test to show that scripts honor explicit PYTHONPATH

Revision history for this message

Gary Poster (gary) wrote on 2010-03-19:

#

Good call on all comments. In the new revision, I answered your questions in the new code comments, and added a test as you described.

(Yes, we want to run without site-packages loaded.)

Reply

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2010-03-19:

#

All good!

review: Approve

Reply

Unmerged revisions

564. By Gary Poster on 2010-03-19: add explanatory comments; extend test to show that scripts honor explicit PYTHONPATH
563. By Gary Poster on 2010-03-19: fix intermittent test failure in update.txt
562. By Gary Poster on 2010-03-19: fixes for bootstrap and a system Python; changes based on learning what would be necessary to be able to develop buildout with a system Python (zc.recipe.testing would also need to use sitepackage_safe_scripts)
561. By Gary Poster on 2010-03-11: set up PYTHONPATH for scripts too, so subprocesses are good to go by default.
560. By Gary Poster on 2010-02-24: merge from gary-6 <- gary-5
559. By Gary Poster on 2010-02-24: with these changes, I can build zc.buildout and run its tests successfully with my system Python. To make it fully robust, zc.recipe.test probably would need to use sitepackage_safe_scripts, but this works for now.
558. By Gary Poster on 2010-02-24: add test for recent fix for buildout
557. By Gary Poster on 2010-02-24: make the buildout script itself safe for a Python with site packages.
556. By Gary Poster on 2010-02-23: merge from gary-5 <- gary-4
555. By Gary Poster on 2010-02-23: merge from gary-5 <- gary-4

Buildout

Merge lp://staging/~gary/zc.buildout/python-support-8-support-subprocess into lp://staging/zc.buildout

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers