#16 Always execute commands and prelude in subshell
Opened 4 years ago by sorlov. Modified 4 years ago
sorlov/python-pytest-multihost prelude-in-subshell  into  master

file modified
+4 -6
@@ -245,25 +245,23 @@ 

              quoted = shell_quote(encode(self.env_sh_path))

              command.stdin.write(b'. %s\n' % quoted)

  

-         if self.command_prelude:

-             command.stdin.write(encode(self.command_prelude))

- 

          if stdin_text:

              command.stdin.write(b"echo -en ")

              command.stdin.write(_echo_quote(encode(stdin_text)))

              command.stdin.write(b" | ")

  

+         command.stdin.write(b'(')

+         if self.command_prelude:

+             command.stdin.write(encode(self.command_prelude))

          if isinstance(argv, basestring):

              # Run a shell command given as a string

-             command.stdin.write(b'(')

              command.stdin.write(encode(argv))

-             command.stdin.write(b')')

          else:

              # Run a command given as a popen-style list (no shell expansion)

              for arg in argv:

                  command.stdin.write(shell_quote(encode(arg)))

                  command.stdin.write(b' ')

- 

+         command.stdin.write(b')')

          command.stdin.write(b'\nexit\n')

          command.stdin.flush()

          command.raiseonerr = raiseonerr

@@ -8,6 +8,8 @@ 

  import contextlib

  import sys

  import os

+ import tempfile

+ import shutil

  

  import pytest_multihost

  import pytest_multihost.transport
@@ -117,6 +119,16 @@ 

      mh.host.transport_class = transport_class

      return mh.install()

  

+ @pytest.yield_fixture(scope='class')

+ def multihost_with_failing_bash_logout(request, transport_class):

+     mh = multihost(request, transport_class)

+     temp_dir = tempfile.mkdtemp()

+     bash_logout = os.path.join(temp_dir, '.bash_logout')

+     with open(bash_logout, 'w') as f:

+         f.write('false\n')

+     mh.host.command_prelude += b'HOME=%s\n' % temp_dir.encode('utf-8')

+     yield mh

+     shutil.rmtree(temp_dir)

  

  @contextlib.contextmanager

  def _first_command(host):
@@ -330,6 +342,14 @@ 

              with pytest.raises(CalledProcessError):

                  false.wait()

  

+     @pytest.mark.parametrize('argv_as_list', [True, False])

+     def test_failing_bash_logout(self, multihost_with_failing_bash_logout,

+                                  argv_as_list):

+         argv = 'true'

+         if argv_as_list:

+             argv = [argv]

+         host = multihost_with_failing_bash_logout.host

+         host.run_command(argv)

  

  

  @pytest.mark.needs_ssh

Before the fix the prelude was executed in main shell and commands were
executed in nested shell only when they were passed as a string, not as a
list. There are two problems with this approach:
1. We can invoke run_command with argument ['false', ';', 'true'] which
is equivalent to 'false ; true' but the former will fail and later will
succeed which is confusing.
2. There are installations (particularly Cygwin) which have
/etc/bash.bash_logout containing invocation of "/usr/bin/clear".
This file is executed when user logs out. When run_command() send
"exit" command to remote shell, "clear" returns exit code 1 complaining
about TERM variable not being defined, which is normal for non-interactive
sessions. Without "set -e" option this is handled correctly and shell
returns result code of last executed user command. But with "set -e" shell
terminates prematurely and when user command succeeded we receive exit
status 1.

Executing "set -e" and user commands inside subshell solves both problems.

Can you add a test?

Note that running ['false', ';', 'true'] should not be equivalent to running 'false ; true'.
The former should run false with the arguments ';' and 'true' (which the false command will ignore); the ; should not be interpreted as a special character.
The latter should run the Bash command false ; true, where ; is interpreted as a command separator.

rebased onto 4a1158c

4 years ago

About issue 1 -- I was wrong, now there is no difference in executing "false; true" or ['false', ';', 'true'], they both fail.

I have reworded the commit message to mention only problem with bash_logout script.

About the test - I do not see how I can test the fix without modifying /etc/bash.bash_logout or ~/.bash_logout which I guess is a bad idea...

Side note:
I agree that ['false', ';', 'true'] is not a proper way of using the function, but the way it is written now, there is no difference with "false ; true" besides spawning a subshell.

About the test - I do not see how I can test the fix without modifying /etc/bash.bash_logout or ~/.bash_logout which I guess is a bad idea...

It's not: set HOME to a temporary directory, and then modify $HOME/.bash_logout. You can even set $HOME from within the shell.

I agree that ['false', ';', 'true'] is not a proper way of using the function, but the way it is written now, there is no difference with "false ; true" besides spawning a subshell.

I suspect that might be right; the current implementation isn't ideal

rebased onto 867a5db

4 years ago

rebased onto 5afbe57

4 years ago

Thanks for idea about setting HOME variable.
I have added the test, it is passing with fix applied and expectedly failing with current version. Surprisingly it fails only with paramiko and passing with openssh.