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

path() context manager no longer works with local() #775

Closed
mars-f opened this Issue Nov 15, 2012 · 7 comments

Comments

Projects
None yet
4 participants
@mars-f

mars-f commented Nov 15, 2012

The path() context manager stopped working for the local() operation in Fabric 1.5. It was working under 1.4.3.

We use the path context manager to add our local virtualenv's bin directory to our fabfile. Assuming that the virtualenv is in './env' relative to the fabfile, here is a task that shows the bug:

@task
def test_path():
    with path('env/bin', behavior='prepend'):
        local('echo $PATH')
        local('which pip')

Everything works under Fabric 1.4.3:

$ fab test_path
[localhost] local: echo $PATH
/usr/lib/lightdm/lightdm:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
[localhost] local: which pip
env/bin/pip

Done.

It breaks under Fabric 1.5.0:

$ fab test_path
[localhost] local: echo $PATH
"env/bin":$PATH
[localhost] local: which pip
/bin/sh: 1: which: not found

Fatal error: local() encountered an error (return code 127) while executing 'which pip'

Aborting.
@poswald

This comment has been minimized.

poswald commented Dec 5, 2012

This seems related to #178

@flashingpumpkin

This comment has been minimized.

flashingpumpkin commented Dec 13, 2012

This also appears to happen without the context manager. Here is some code that triggers the same error:

$ fab dev provision -u vagrant -f vagrant.py 
[127.0.0.1:2222] Executing task 'provision'
[127.0.0.1:2222] sudo: mkdir -p /var/chef
[localhost] local: echo $PATH
$PATH:"/home/pindrop/pindrop"
[localhost] local: mkdir /tmp/tmpsnf_qP/cookbooks
/bin/sh: 1: mkdir: not found

Fatal error: local() encountered an error (return code 127) while executing 'mkdir /tmp/tmpsnf_qP/cookbooks'

Aborting.

Fatal error: One or more hosts failed while executing task 'provision'

Aborting.

Note: I've squeezed a local("echo $PATH") in just before local('mkdir %s/cookbooks' % tmpfolder) in the linked code above.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 13, 2012

I have a test case reproducing it and believe I know the cause: the string "$PATH" used for the default "append to existing PATH value" behavior is now getting escaped, causing the dollar sign to become literal. A literal shell path of "$PATH:whatever_you_appended", where $PATH is not expanded by the shell, is going to be pretty broken.

Still determining exactly what causes this (the spots I've checked so far that would be related, appear to be unchanged since 1.4) but I do recall changes to the shell escaping so those may be at fault.

If that is correct, I will either revert those changes with a note to the committer, or (ideally) try moving this specific part of the env var updating to occur after the escaping. (This does mean that folks wanting literal dollar signs/etc in their PATH values, will get hosed, but given this change probably went out recently, I am not worried about impact.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 13, 2012

OK, the code I ran across that changed this was part of #675 / #263. I'll poke a bit to see whether the use of _shell_escape here is actually required -- offhand I think it may be going too far. Otherwise, will add conditional logic to treat PATH differently.

@flashingpumpkin

This comment has been minimized.

flashingpumpkin commented Dec 13, 2012

I just ran a git bisect between 1.4.4 and 1.5.0. It appears that this was the first bad commit:

1010359 is the first bad commit

commit 1010359e4b8504f411da417985100df45f5f63c3
Author: Kamil Kisiel <kamil@kamilkisiel.net>
Date:   Tue Jul 3 13:34:54 2012 -0700

    Added shell_env context manager

:040000 040000 2884f34046d00cb14d0385532f86a700010cf7f1 a5828e7fb9cd2b005e377a065120f42bc87f903b M  fabric
:040000 040000 9c4750a6088a062f8b511f866db9269d69ad4ffa 9176b631b016b4183ad605292f21c10f504148bd M  tests

bitprophet added a commit that referenced this issue Dec 13, 2012

@flashingpumpkin

This comment has been minimized.

flashingpumpkin commented Dec 14, 2012

Yes, I can confirm that it's fixed. Thanks @bitprophet

@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 16, 2012

Great! Glad to hear it. I will be publishing 1.5.2 within a week or so, hopefully less. Thanks again.

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