Repository: nickstenning/honcho

Issue:
Exported upstart configuration silently fails with quoted variables
At or near d8be8f4a8 (version 0.5.0), quoted variables in exported upstart scripts became invalid and fail silently. Previously, quoted `.env` variables with spaces generated upstart configurations with correctly-nested double and single quotes. This is an example generated by 84a1f7d (also version 0.5.0):

```
VAR="foo bar"
```

```
start on starting things-stuff
stop on stopping things-stuff
respawn

exec su - migurski -s /bin/sh -c 'cd /home/migurski/things; export PORT=5000; export VAR="foo bar"; python stuff.py >> /var/log/things/stuff-1.log 2>&1'
```

Starting at d8be8f4a8 and still in 0.6.6, the exported configuration from the configuration above began producing this invalid and failing upstart configuration, due to the single quotes:

```
start on starting things-stuff
stop on stopping things-stuff
respawn

exec su - migurski -s /bin/sh -c 'cd /home/migurski/things;export VAR='foo bar';export PORT=5000;python stuff.py >> /var/log/things/stuff-1.log 2>&1'
```

Here are my Procfile and python script for testing:

```
stuff: python stuff.py
```

```python
from sys import stderr
from os import environ
from time import sleep

while True:
	print >> stderr, repr(environ['VAR'])
	sleep(5)
```

Discussion:
nickstenning: @migurski Could you check this out and let me know if it correctly addresses the issue for you?

migurski: Thanks Nick! I checked again with Python 2.7.6 on Ubuntu 14.04. At 84a1f7d it generated a correct script, at d8be8f4 and it generated a bad one, and with your recent efd6292 and it generated this improved working script:

```
start on starting honcho-stuff
stop on stopping honcho-stuff
respawn

env VAR='foo bar'
env PORT=5000

exec su - migurski -m -s /bin/sh -c 'cd /home/migurski/honcho; exec python /home/migurski/honcho/stuff.py >> /var/log/honcho/stuff-1.log 2>&1'
```
migurski: I’ve updated https://github.com/nickstenning/honcho/pull/154, though with the file `honcho/test/integration/test_export.py` gone I’m not sure it will have an effect.

Patch:
diff --git a/honcho/export/templates/upstart/process.conf b/honcho/export/templates/upstart/process.conf
index 8a05378..fe6b451 100644
--- a/honcho/export/templates/upstart/process.conf
+++ b/honcho/export/templates/upstart/process.conf
@@ -2,8 +2,7 @@ start on starting {{ group_name }}
 stop on stopping {{ group_name }}
 respawn
 
-exec su - {{ user }} -s {{ shell }} -c 'cd {{ app_root }};
-{%- for k, v in process.env.items() -%}
-  export {{ k }}={{ v | shellquote }};
-{%- endfor -%}
-exec {{ process.cmd }} >> {{ log }}/{{ process.name|dashrepl }}.log 2>&1'
+{% for k, v in process.env.items() -%}
+env {{ k }}={{ v | shellquote }}
+{% endfor %}
+exec su - {{ user }} -m -s {{ shell }} -c 'cd {{ app_root }}; exec {{ process.cmd }} >> {{ log }}/{{ process.name|dashrepl }}.log 2>&1'
diff --git a/honcho/manager.py b/honcho/manager.py
index 218f2b4..ff31a8e 100644
--- a/honcho/manager.py
+++ b/honcho/manager.py
@@ -53,7 +53,7 @@ class Manager(object):
 
         self._terminating = False
 
-    def add_process(self, name, cmd, quiet=False, env=None):
+    def add_process(self, name, cmd, quiet=False, env=None, cwd=None):
         """
         Add a process to this manager instance. The process will not be started
         until #loop() is called.
@@ -63,7 +63,8 @@ class Manager(object):
                                   name=name,
                                   quiet=quiet,
                                   colour=next(self._colours),
-                                  env=env)
+                                  env=env,
+                                  cwd=cwd)
         self._processes[name] = {}
         self._processes[name]['obj'] = proc
 
diff --git a/honcho/process.py b/honcho/process.py
index 669c8ef..c211af1 100644
--- a/honcho/process.py
+++ b/honcho/process.py
@@ -21,12 +21,14 @@ class Process(object):
                  name=None,
                  colour=None,
                  quiet=False,
-                 env=None):
+                 env=None,
+                 cwd=None):
         self.cmd = cmd
         self.colour = colour
         self.quiet = quiet
         self.name = name
         self.env = os.environ.copy() if env is None else env
+        self.cwd = cwd
 
         # This is a honcho.environ.Env object, to allow for stubbing of
         # external calls, not the operating system environment.
@@ -36,7 +38,7 @@ class Process(object):
 
     def run(self, events=None, ignore_signals=False):
         self._events = events
-        self._child = self._child_ctor(self.cmd, env=self.env)
+        self._child = self._child_ctor(self.cmd, env=self.env, cwd=self.cwd)
         self._send_message({'pid': self._child.pid}, type='start')
 
         # Don't pay attention to SIGINT/SIGTERM. The process itself is


Test patch:
diff --git a/tests/integration/test_export.py b/tests/integration/test_export.py
index bbb2397..09543b7 100644
--- a/tests/integration/test_export.py
+++ b/tests/integration/test_export.py
@@ -37,3 +37,37 @@ def test_export_upstart(testenv):
                      'trunk-web-1.conf'):
         expected = testenv.path('elephant', filename)
         assert os.path.exists(expected)
+
+
+@pytest.mark.parametrize('testenv', [{
+    'Procfile': "web: python web.py",
+    '.env': """
+NORMAL=ok
+SQ_SPACES='sqspace sqspace'
+DQ_SPACES="dqspace dqspace"
+SQ="it's got single quotes"
+DQ='it has "double" quotes'
+EXCL='an exclamation mark!'
+SQ_DOLLAR='costs $UNINTERPOLATED amount'
+DQ_DOLLAR="costs $UNINTERPOLATED amount"
+"""
+}], indirect=True)
+def test_export_upstart_environment(testenv):
+    ret, out, err = testenv.run_honcho([
+        'export',
+        'upstart',
+        testenv.path('test'),
+        '-a', 'envvars',
+    ])
+
+    assert ret == 0
+
+    lines = open(testenv.path('test', 'envvars-web-1.conf')).readlines()
+    assert 'env NORMAL=ok\n' in lines
+    assert "env SQ_SPACES='sqspace sqspace'\n" in lines
+    assert "env DQ_SPACES='dqspace dqspace'\n" in lines
+    assert "env SQ='it'\"'\"'s got single quotes'\n" in lines
+    assert "env DQ='it has \"double\" quotes'\n" in lines
+    assert "env EXCL='an exclamation mark!'\n" in lines
+    assert "env SQ_DOLLAR='costs $UNINTERPOLATED amount'\n" in lines
+    assert "env DQ_DOLLAR='costs $UNINTERPOLATED amount'\n" in lines
diff --git a/tests/test_manager.py b/tests/test_manager.py
index abee53c..ae028c6 100644
--- a/tests/test_manager.py
+++ b/tests/test_manager.py
@@ -59,12 +59,13 @@ class FakeEnv(object):
 
 class FakeProcess(object):
 
-    def __init__(self, cmd, name=None, colour=None, quiet=None, env=None):
+    def __init__(self, cmd, name=None, colour=None, quiet=None, env=None, cwd=None):
         self.cmd = cmd
         self.name = name
         self.colour = colour
         self.quiet = quiet
         self.env = env
+        self.cwd = cwd
 
         self._events = None
         self._options = {}
@@ -229,6 +230,10 @@ class TestManager(object):
         with pytest.raises(AssertionError):
             self.m.add_process('foo', 'another command')
 
+    def test_add_process_sets_cwd(self):
+        proc = self.m.add_process('foo', 'ruby server.rb', cwd='foo-dir')
+        assert proc.cwd == 'foo-dir'
+
     def test_loop_with_empty_manager_returns_immediately(self):
         self.m.loop()
 
diff --git a/tests/test_process.py b/tests/test_process.py
index 59e1538..9002212 100644
--- a/tests/test_process.py
+++ b/tests/test_process.py
@@ -184,3 +184,9 @@ class TestProcess(object):
         proc.run(self.q)
         msg = self.q.find_message({'returncode': 42})
         assert msg.type == 'stop'
+
+    def test_cwd_passed_along(self):
+        proc = Process('echo 123', cwd='fake-dir')
+        proc._child_ctor = FakePopen
+        proc.run(self.q)
+        assert proc._child.kwargs['cwd'] == 'fake-dir'