[MERGE] run_bzr_subprocess(env_changes={})

[Martin Pool]

On 28 Aug 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:

> > That sounds good.  In general setting them while the process is
> > running is not a good idea.  (For example some systems scan $TZ at
> > libc startup and some every time it's needed.)  So
> > 
> >   run_bzr_subprocess(...,
> >       env_changes = {"REMOVE_ME": None, "LANG": "de_DE"})
> > 
> 
> I went ahead and merged the locale changes, since they had been
> approved, but I liked the idea of this, so I went ahead and updated
> run_bzr_subprocess, the locale tests, and added a direct test for this
> that the environment changes are used, but don't stick in the parent.

Great

> === modified file bzrlib/tests/__init__.py
> --- bzrlib/tests/__init__.py
> +++ bzrlib/tests/__init__.py
> @@ -843,12 +843,28 @@
>          profiled or debugged so easily.
>  
>          :param retcode: The status code that is expected.  Defaults to 0.  If
> -        None is supplied, the status code is not checked.
> +            None is supplied, the status code is not checked.
> +        :param env_changes: A dictionary which lists changes to environment
> +            variables. A value of None will unset the env variable.
> +            The values must be strings. The change will only occur in the
> +            child, so you don't need to fix the environment after running.
>          """
> +        env_changes = kwargs.get('env_changes', None)
> +        preexec_func = None
> +        if env_changes:
> +            def cleanup_environment():
> +                for env_var, value in env_changes.iteritems():
> +                    if value is None:
> +                        del os.environ[env_var]
> +                    else:
> +                        os.environ[env_var] = value
> +            preexec_func = cleanup_environment
> +
>          bzr_path = os.path.dirname(os.path.dirname(bzrlib.__file__))+'/bzr'
>          args = list(args)
> -        process = Popen([sys.executable, bzr_path]+args, stdout=PIPE, 
> -                         stderr=PIPE)
> +        process = Popen([sys.executable, bzr_path]+args,
> +                         stdout=PIPE, stderr=PIPE,
> +                         preexec_fn=preexec_func)
>          out = process.stdout.read()
>          err = process.stderr.read()
>          retcode = process.wait()

Wouldn't it be simpler to just run the preexec function every time?
It'll do nothing if there are no changes and that will avoid several
lines.

I recall that in some situations deleting from os.environ does not work.
Perhaps that was on old Pythons.  The manual says

## If the platform supports the unsetenv() function, you can delete items
## in this mapping to unset environment variables. unsetenv() will be
## called automatically when an item is deleted from os.environ.

Let's put it in and see if it fails anywhere.

>      def test_log_C(self):
> -        os.environ['LANG'] = 'C'
>          out, err = self.run_bzr_subprocess('--no-aliases', '--no-plugins',
>                                             '-q', 'log', '--log-format=long',
> -                                           'tree')
> +                                           'tree',
> +                                           env_changes={'LANG':'C',
> +                                                  'BZR_PROGRESS_BAR':'none'}
> +                                          )

I think when things start getting squished over like this it's better to
change indenting modes, like

    out, err = self.run_bzr_subprocess('--no-aliases', '--no-plugins',
        '-q', 'log', '--log-format=long', 'tree',
        env_changes={'LANG':'C', 'BZR_PROGRESS_BAR':'none'})

>  
> +    def test_run_bzr_subprocess_env(self):
> +        """run_bzr_subprocess can set environment variables in the child only.
> +
> +        These changes should not change the running process, only the child.
> +        """
> +        # The test suite should unset this variable
> +        self.assertEqual(None, os.environ.get('BZR_EMAIL'))
> +        out, err = self.run_bzr_subprocess('whoami', env_changes={
> +                                            'BZR_EMAIL':'Joe Foo <joe at foo.com>'
> +                                          })

Nice way to test it.

> +        self.assertEqual('', err)
> +        self.assertEqual('Joe Foo <joe at foo.com>\n', out)
> +        # And it should not be modified
> +        self.assertEqual(None, os.environ.get('BZR_EMAIL'))
> +
> +        # Do it again with a different address, just to make sure
> +        # it is actually changing
> +        out, err = self.run_bzr_subprocess('whoami', env_changes={
> +                                            'BZR_EMAIL':'Barry <bar at foo.com>'
> +                                          })
> +        self.assertEqual('', err)
> +        self.assertEqual('Barry <bar at foo.com>\n', out)
> +        self.assertEqual(None, os.environ.get('BZR_EMAIL'))

This doesn't test asking that a variable be deleted.  Perhaps you could
set it in the parent process, then run it in a subprocess set it to
None, then in a finally block remove it.

+1 with those changes.

-- 
Martin