New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

specify environment variables in run() #280

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@philtay
Contributor

philtay commented Sep 29, 2015

Ref: #259

Tests and docs after your approval and first round of comments.

@@ -466,13 +474,14 @@ def start(self, command):
# bash for now because that's what we have been testing
# against.
# TODO: also see if subprocess is using equivalent of execvp...
os.execv('/bin/bash', ['/bin/bash', '-c', command])
os.execve('/bin/bash', ['/bin/bash', '-c', command], env=self.env)

This comment has been minimized.

@betterdiff

betterdiff Sep 29, 2015

  • E501 line too long (82 > 79 characters)
philtay
@philtay

This comment has been minimized.

Contributor

philtay commented Sep 29, 2015

Don't know exactly why the tests fail. On my PC tox doesn't report any problem. Anyway, please find below a simple task that shows the feature.

from invoke import run, task

@task
def foo():
    run("env")
    print "************************************************************"
    run("env", env={'FOO':'BAR'})
    print "************************************************************"
    run("env", env={})
if env:
self.env.update(env)
else:
self.env = {}

This comment has been minimized.

@sophacles

sophacles Sep 29, 2015

This is a pretty big breaking change to current (and arguably expected) behaviour. If the user doesn't pass an environment, the child process is executed with no environment (empty dict). The general expected behaviour is that a child is spawned with the environment of it's parent unless otherwise specified.

This would probably be best as:

 # Environment variables
self.env = os.environ.copy()
if env:
    self.env.update(env)
@philtay

This comment has been minimized.

Contributor

philtay commented Sep 29, 2015

@sophacles thanks for your feedback. if the user doesn't pass an environment the child process is executed WITH the parent's environment. there is no breaking change. only if the user passes an empty dict the child process is executed with no environment.

my implementation is:

  • non empty dict -> add these vars to the current env and execute the process
  • empty dict (or something else that evaluates to False in boolean context) -> execute the process with no env
  • if the user doesn't pass an env (or None) -> execute the process with the parent's env (i.e. current behavior)
@sophacles

This comment has been minimized.

sophacles commented Sep 29, 2015

@philtay Oh got it, I was a bit confused on that chunk of code. I understand the logic now - thanks for the clarification

Thinking further about it - I think perhaps the best thing to do would be mirror the subprocess environment logic, that is:

                           # from subprocess.py line 1281 in python 2.7.10
                            if env is None:
                                os.execvp(executable, args)
                            else:
                                os.execvpe(executable, args, env)

Using values from os.environ should be left to the user to set up: there are too many edge cases otherwise:

  • user wants to remove some variables from os.environ
  • user wants to only have a specific environment (note #67 will also need to be completed before this case is fully available - setting shell to something like /bin/env -i /bin/bash) or alternately a runner with shell=False available.
  • etc
@sophacles

This comment has been minimized.

sophacles commented Sep 29, 2015

Tangentially related to my comment above: the invoke code has:

                # TODO: also see if subprocess is using equivalent of execvp...

@bitprophet - the answer to this todo is: yes, subprocess uses execvp and execvpe when not on windows.

@philtay

This comment has been minimized.

Contributor

philtay commented Sep 29, 2015

@sophacles i agree with you. i'll wait for @bitprophet input before making changes.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Mar 22, 2016

Went ahead with my own version of this today (necessitated by interim changes); credited in changelog though. Thanks!

@bitprophet bitprophet closed this Mar 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment