Ability to specify environment variables to be used with the run function #259

Closed
fgimian opened this Issue Jul 10, 2015 · 20 comments

Projects

None yet

3 participants

@fgimian
fgimian commented Jul 10, 2015

Hey there, love invoke.

In fabric, we have with shell_env(...) so that a particular set of environment variables will be used in a particular scope, which apply to commands being run.

Is there any way to currently accomplish this with invoke?

I would really love to switch back to it, but I've reluctantly switched to paver until I can figure this out.

Thanks
Fotis

@bitprophet
Member

There's no explicit support for this yet, but since Invoke is primary concerned with local-to-system shell commands, you should be able to modify os.environ yourself & the subprocess ought to inherit the current state of that data structure when you call run.

So this sort of thing ought to Just Work (tm):

import os
from invoke import run

my_vars = {'FOO': 'bar'}
os.environ.update(my_vars)
run("echo $FOO") # => should result in stdout of "bar"

That's not to say we won't add a 'helper' for this (i.e. a contextmanager that modifies os.environ only in a block) or take stronger ownership of the env presented to our subprocesses (e.g. making them default to an empty env, then offering something like run("command", env={'a': 'dict'})) so I'll leave this open as a placeholder for those concerns. Thanks!

@fgimian
fgimian commented Jul 11, 2015

Thanks so much for your reply @bitprophet, this will do for now. I'm all in favour of allowing us to pass in the env kwarg. The only con with the approach above (that modifies os.environ) is that it modifies os.environ for the entire script, one would need to save the original and restore it afterwards if they wanted to be a little neater ๐Ÿ˜„

A simple context manager would probably also do the trick, something like this (not tested yet):

def environ(env):
    original_environ = os.environ.copy()
    os.environ.update(env)
    yield
    os.environ = original_environ

Anyways, thanks a lot for all your help and have a lovely weekend!

Cheers
Fotis

@fgimian
fgimian commented Jul 11, 2015

OK, so I thought I had a nice neat little solution here:

from contextlib import contextmanager
import os

@contextmanager
def environ(env):
    original_environ = os.environ.copy()
    os.environ.update(env)
    yield
    os.environ = original_environ

print os.environ.get('name')
with environ({'name': 'Invoke Me :o'}):
    print os.environ.get('name')
print os.environ.get('name')

Output:

โž” python testing.py 
None
Invoke Me :o
None

But ti seems that invoke is doing something which doesn't like this solution ๐Ÿ˜ข

from invoke import task, run

@task
def runme():
    run('echo 1: $name')
    with environ({'name': 'Invoke Me :o'}):
        run('echo 2: $name')
    run('echo 3: $name')

Output:

โž” invoke runme
1:
2: Invoke Me :o
3: Invoke Me :o

Cheers
Fotis

@bitprophet
Member

Invoke isn't doing anything to os.environ right now IIRC, so something else must be afoot, though I can't see what offhand. My instinct was to be all "maybe os.environ doesn't allow itself to be outright replaced" but nope, I can do that no problem in a shell.

When I test, it looks like my initial assertion is actually false: os.environ mutations do not get picked up by the subprocess after all! To wit:

>>> os.environ['WHAT'] = 'THE HECK?'
>>> os.environ.get('WHAT')
'THE HECK?'
>>> run("env | grep LANG")
LANG=en_US.UTF-8
<invoke.runners.Result object at 0x1044510d0>
>>> run("env | grep WHAT")
<kaboom because grep exited 1 due to no matches>

So that's egg on my face :) turns out we do need a specific feature for this after all!

@bitprophet
Member

Hrm, I'm confused by the stdlib right now, here's the skinny:

@bitprophet
Member

Running some quick tests, neither execv nor subprocess are honoring changes to os.environ on my system (OS X Yosemite, system Python 2.6), either in the Python shell or via python <file.py>. So that's interesting. One of us should dig around in google and the Python tracker to see what the heck - I'm out of time for this today, was just intending to triage briefly :) thanks!

@fgimian
fgimian commented Jul 15, 2015

Firstly, thanks for all your efforts on this, really appreciate it! ๐Ÿ˜„

Interestingly, things work differently on my Red Hat box here:

Python 2.6.6 (r266:84292, Nov 21 2013, 10:50:32)
[GCC 4.4.7 20120313 (Red Hat 4.4.7-4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> from invoke import run
>>> os.environ['WHAT'] = 'THE HECK?'
>>> os.environ.get('WHAT')
'THE HECK?'
>>> run("env | grep LANG")
LANG=en_US.UTF-8
<invoke.runners.Result object at 0x1a10310>
>>> run("env | grep WHAT")
WHAT=THE HECK?
<invoke.runners.Result object at 0x1a19f10>
>>>

In my testing, both subprocess and os.execve seemed to honour the environment variables set in os.environ.

Edit: Hmm, tested on OS X with Python 2.7 and 2.6 and both got the environment variable.

Cheers
Fotis

@bitprophet
Member

Which OS X, and are those the system Python interpreters or e.g. installed via brew? I was on Yosemite (10.10.4) and using its system python 2.6(.9).

Also I think I never checked which Invoke version you're running on, though this still doesn't feel like anything our own code would be affecting, given my subprocess+execv test.

@fgimian
fgimian commented Jul 23, 2015

@bitprophet sorry for the late reply on this, this is OS X 10.10 with Homebrew versions of Python, but also tested with the Python that ships and it's fine too ๐Ÿ˜„

@frol
frol commented Aug 21, 2015

There is another concern here. Changing os.environ affects all threads. Basically, It is the same issue as Invoke already has with "current directory": #230

@fgimian
fgimian commented Aug 23, 2015

@frol Very good point. This is a good argument for passing env directly to subprocess instead of manipulating os.environ. @bitprophet what are your thoughts mate? ๐Ÿ˜„

@bitprophet
Member

See the last bullet point here - #259 (comment) - I noted that it's an option and yea, right now I can't see a great argument for anything but the "clean env" (obviously, with option for basing that dict on the current value of os.environ) solution. So we'll do that.

@fgimian
fgimian commented Sep 3, 2015

Thanks @bitprophet ๐Ÿ˜„

@bitprophet bitprophet removed the Needs patch label Sep 30, 2015
@bitprophet
Member

Poking this now (personal itch-scratch...:D). The timeline is hazy now but I'm not sure why I was implying we were no longer using subprocess above; at present it depends on whether one is using a PTY or not:

  • pty=True uses pty.fork + exec*. Currently execv but maybe wanting execvp (or execve/execvpe as per this ticket).
  • pty=False uses subprocess.Popen.
    • As of later comments in #152 I had mild thoughts about eschewing subprocess in favor of direct os.fork + exec*. That didn't turn out to be strictly necessary.
    • For now I think we can simply use the env argument to subprocess.Popen, which doesn't seem to be a thing we were discussing earlier in this ticket. As of Python 2.6 it sounds pretty equivalent to what one can do with execve re: supplying a completely clean environment map of one's choosing.

So I'll move forward with using both of those approaches as necessary and see how well it works. (Meaning, yes - as before, only using os.environ as a potential source for the new env, and not manipulating it at all.)

@bitprophet
Member
  • #280 lays an ok groundwork but is a bit out of date (my fault!)
  • Erich's note in there about the default env being the parent env, is accurate IMO
  • Furthermore, it should probably be possible to either replace or augment the new child env.
    • Granted, augmenting (dict.update) is likely the most useful/common case...
    • But I can see folks wanting to completely control the new env instead of needing to manually override everything their parent env happens to define, when they need full controllability/auditability.
    • Question is how best to do that. A primitive-type kwarg doesn't cut it, so either 2 kwargs (value and type/behavior, e.g. run(xxx, env={'update': 'plz'}, replace_env=False)) or a higher-level object (run(xxx, env=Env({'update': 'plz'}, replace=False))).
      • I normally go for the 2-kwargs approach unless there's more in play than just value+behavior-toggle.
      • That said, enough of that decision leads to a VERY overburdened function signature =/
      • I don't see any obvious benefits to an Env class at present unfortunately.
@bitprophet
Member

The 'behavior' kwarg (which would default to 'env is updated onto a copy of os.environ, no matter what) could have any number of names...brainstorm:

  • clean_env (default False)
  • replace_env (ditto)
  • fresh_env (ditto)
  • overwrite_env (yup)
  • update_env (inverse - default True)
  • probably any of those but phrased as env_<word>, eg env_overwrite?

My inclination, which isn't strong, is that replace_env is the least unintuitive. I'll go with it for now but definitely open to feedback, we can always change it before 1.0 (not super far off, but not next week or anything either).

@bitprophet
Member

Hm, well, or we could do this - two dict kwargs:

  • env: a full replacement value
  • env_overrides: overrides only

But then we'd need to handle the case when both are given, probably by exploding as per the Zen of Python

Will think about this for a bit. (Isn't this stream of consciousness so exciting?)

@bitprophet
Member

Eh, I think an updating env kwarg is still what most users are going to expect, so will move ahead with env + replace_env.

@bitprophet bitprophet added a commit that referenced this issue Mar 22, 2016
@bitprophet bitprophet DDD, TDD re: #259 7e6c02d
@bitprophet bitprophet added a commit that referenced this issue Mar 22, 2016
@bitprophet bitprophet Changelog re #259, #280 95498e7
@bitprophet
Member

All done. At least, works well enough for me to use it in a crummy personal script that needed to run shell commands with a tweaked env.

@bitprophet bitprophet closed this Mar 22, 2016
@fgimian
fgimian commented Mar 22, 2016

Huge thanks @bitprophet!! Really appreciate the efforts on this. I'll be sure to try it out soon and report back ๐Ÿ˜„

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