Transport: add option to not use a login shell for all commands (#4271

)

Both the `LocalTransport` as well as the `SshTransport` were using a
bash login shell, i.e., using the `-l` flag in the bash commands, in
order to properly load the user environment which may contain crucial
environment variables to be set or modules to be loaded.

However, for certain machines, the login shell will produce spurious
output that prevents AiiDA from properly parsing the output from the
commands that are executed. The recommended approach is to remove the
code that is producing the output, but this is not always within the
control of the user. That is why a `use_login_shell` option is added to
the `Transport` class that switches the use of a login shell.

The new option is added to the `verdi computer configure` command and as
such is stored in the `AuthInfo`. The new logic affects all bash commands
that are executed, including the `gotocomputer` command that follows a
slightly different code path.

.github/config/localhost-config.yaml

@@ -1,2 +1,3 @@
 ---
+use_login_shell: true
 safe_interval: 0

aiida/transports/plugins/local.py

@@ -42,7 +42,6 @@ class LocalTransport(Transport):
     ``unset PYTHONPATH`` if you plan on running calculations that use Python.
     """
 
-    # There are no valid parameters for the local transport
     _valid_auth_options = []
 
     # There is no real limit on how fast you can safely connect to a localhost, unlike often the case with SSH transport
@@ -744,7 +743,9 @@ def _exec_command_internal(self, command, **kwargs):  # pylint: disable=unused-a
 
         # Note: The outer shell will eat one level of escaping, while
         # 'bash -l -c ...' will eat another. Thus, we need to escape again.
-        command = 'bash -l -c ' + escape_for_bash(command)
+        bash_commmand = self._bash_command_str + '-c '
+
+        command = bash_commmand + escape_for_bash(command)
 
         proc = subprocess.Popen(
             command,
@@ -803,11 +804,8 @@ def gotocomputer_command(self, remotedir):
 
         :param str remotedir: the full path of the remote directory
         """
-        script = ' ; '.join([
-            'if [ -d {escaped_remotedir} ]', 'then cd {escaped_remotedir}', 'bash', "else echo ' ** The directory'",
-            "echo ' ** {remotedir}'", "echo ' ** seems to have been deleted, I logout...'", 'fi'
-        ]).format(escaped_remotedir="'{}'".format(remotedir), remotedir=remotedir)
-        cmd = 'bash -c "{}"'.format(script)
+        connect_string = self._gotocomputer_string(remotedir)
+        cmd = 'bash -c {}'.format(connect_string)
         return cmd
 
     def rename(self, oldpath, newpath):

aiida/transports/plugins/ssh.py

@@ -1232,7 +1232,9 @@ def _exec_command_internal(self, command, combine_stderr=False, bufsize=-1):  #
 
         # Note: The default shell will eat one level of escaping, while
         # 'bash -l -c ...' will eat another. Thus, we need to escape again.
-        channel.exec_command('bash -l -c ' + escape_for_bash(command_to_execute))
+        bash_commmand = self._bash_command_str + '-c '
+
+        channel.exec_command(bash_commmand + escape_for_bash(command_to_execute))
 
         stdin = channel.makefile('wb', bufsize)
         stdout = channel.makefile('rb', bufsize)
@@ -1298,21 +1300,14 @@ def gotocomputer_command(self, remotedir):
             further_params.append('-i {}'.format(escape_for_bash(self._connect_args['key_filename'])))
 
         further_params_str = ' '.join(further_params)
-        # I use triple strings because I both have single and double quotes, but I still want everything in
-        # a single line
-        connect_string = (
-            """ssh -t {machine} {further_params} "if [ -d {escaped_remotedir} ] ;"""
-            """ then cd {escaped_remotedir} ; bash -l ; else echo '  ** The directory' ; """
-            """echo '  ** {remotedir}' ; echo '  ** seems to have been deleted, I logout...' ; fi" """.format(
-                further_params=further_params_str,
-                machine=self._machine,
-                escaped_remotedir="'{}'".format(remotedir),
-                remotedir=remotedir
-            )
-        )
 
-        # print connect_string
-        return connect_string
+        connect_string = self._gotocomputer_string(remotedir)
+        cmd = 'ssh -t {machine} {further_params} {connect_string}'.format(
+            further_params=further_params_str,
+            machine=self._machine,
+            connect_string=connect_string,
+        )
+        return cmd
 
     def symlink(self, remotesource, remotedestination):
         """

aiida/transports/transport.py

@@ -47,21 +47,50 @@ class Transport(abc.ABC):
     _valid_auth_params = None
     _MAGIC_CHECK = re.compile('[*?[]')
     _valid_auth_options = []
-    _common_auth_options = [(
-        'safe_interval', {
-            'type': float,
-            'prompt': 'Connection cooldown time (s)',
-            'help': 'Minimum time interval in seconds between opening new connections.',
-            'callback': validate_positive_number
-        }
-    )]
+    _common_auth_options = [
+        (
+            'use_login_shell', {
+                'default':
+                True,
+                'switch':
+                True,
+                'prompt':
+                'Use login shell when executing command',
+                'help':
+                ' Not using a login shell can help suppress potential'
+                ' spurious text output that can prevent AiiDA from parsing the output of commands,'
+                ' but may result in some startup files (.profile) not being sourced.',
+                'non_interactive_default':
+                True
+            }
+        ),
+        (
+            'safe_interval', {
+                'type': float,
+                'prompt': 'Connection cooldown time (s)',
+                'help': 'Minimum time interval in seconds between opening new connections.',
+                'callback': validate_positive_number
+            }
+        ),
+    ]
 
     def __init__(self, *args, **kwargs):  # pylint: disable=unused-argument
         """
         __init__ method of the Transport base class.
+
+        :param safe_interval: (optional, default self._DEFAULT_SAFE_OPEN_INTERVAL)
+           Minimum time interval in seconds between opening new connections.
+        :param use_login_shell: (optional, default True)
+           if False, do not use a login shell when executing command
         """
         from aiida.common import AIIDA_LOGGER
         self._safe_open_interval = kwargs.pop('safe_interval', self._DEFAULT_SAFE_OPEN_INTERVAL)
+        self._use_login_shell = kwargs.pop('use_login_shell', True)
+        if self._use_login_shell:
+            self._bash_command_str = 'bash -l '
+        else:
+            self._bash_command_str = 'bash '
+
         self._logger = AIIDA_LOGGER.getChild('transport').getChild(self.__class__.__name__)
         self._logger_extra = None
         self._is_open = False
@@ -193,6 +222,13 @@ def _get_safe_interval_suggestion_string(cls, computer):  # pylint: disable=unus
         """
         return cls._DEFAULT_SAFE_OPEN_INTERVAL
 
+    @classmethod
+    def _get_use_login_shell_suggestion_string(cls, computer):  # pylint: disable=unused-argument
+        """
+        Return a suggestion for the specific field.
+        """
+        return 'True'
+
     @property
     def logger(self):
         """
@@ -756,6 +792,18 @@ def glob0(self, dirname, basename):
     def has_magic(self, string):
         return self._MAGIC_CHECK.search(string) is not None
 
+    def _gotocomputer_string(self, remotedir):
+        """command executed when goto computer."""
+        connect_string = (
+            """ "if [ -d {escaped_remotedir} ] ;"""
+            """ then cd {escaped_remotedir} ; {bash_command} ; else echo '  ** The directory' ; """
+            """echo '  ** {remotedir}' ; echo '  ** seems to have been deleted, I logout...' ; fi" """.format(
+                bash_command=self._bash_command_str, escaped_remotedir="'{}'".format(remotedir), remotedir=remotedir
+            )
+        )
+
+        return connect_string
+
 
 class TransportInternalError(InternalError):
     """

docs/source/intro/troubleshooting.rst

@@ -269,6 +269,13 @@ To test if a the computer does not produce spurious output, run (after configuri
 
 which checks and, in case of problems, suggests how to solve the problem.
 
+.. note::
+
+    If the methods explained above do not work, you can configure AiiDA to not use a login shell when connecting to your computer, which may prevent the spurious output from being printed:
+    During ``verdi computer configure``, set ``-no-use-login-shell`` or when asked to use a login shell, set it to ``False``.
+    Note, however, that this may result in a slightly different environment, since `certain startup files are only sourced for login shells <https://unix.stackexchange.com/a/46856/155909>`_.
+
+
 .. _StackExchange thread: https://apple.stackexchange.com/questions/51036/what-is-the-difference-between-bash-profile-and-bashrc
 
 

tests/cmdline/commands/test_computer.py

@@ -370,9 +370,16 @@ def test_local_interactive(self):
         comp = self.comp_builder.new()
         comp.store()
 
-        result = self.cli_runner.invoke(computer_configure, ['local', comp.label], input='\n', catch_exceptions=False)
+        command_input = ('{use_login_shell}\n{safe_interval}\n').format(use_login_shell='False', safe_interval='1.0')
+        result = self.cli_runner.invoke(
+            computer_configure, ['local', comp.label], input=command_input, catch_exceptions=False
+        )
         self.assertTrue(comp.is_user_configured(self.user), msg=result.output)
 
+        new_auth_params = comp.get_authinfo(self.user).get_auth_params()
+        self.assertEqual(new_auth_params['use_login_shell'], False)
+        self.assertEqual(new_auth_params['safe_interval'], 1.0)
+
     def test_ssh_interactive(self):
         """
         Check that the interactive prompt is accepting the correct values.
@@ -411,6 +418,7 @@ def test_ssh_interactive(self):
         self.assertEqual(new_auth_params['port'], port)
         self.assertEqual(new_auth_params['look_for_keys'], look_for_keys)
         self.assertEqual(new_auth_params['key_filename'], key_filename)
+        self.assertEqual(new_auth_params['use_login_shell'], True)
 
     def test_local_from_config(self):
         """Test configuring a computer from a config file"""

tests/transports/test_local.py

@@ -48,3 +48,16 @@ def test_basic():
         """Test constructor."""
         with LocalTransport():
             pass
+
+
+def test_gotocomputer():
+    """Test gotocomputer"""
+    with LocalTransport() as transport:
+        cmd_str = transport.gotocomputer_command('/remote_dir/')
+
+        expected_str = (
+            """bash -c  "if [ -d '/remote_dir/' ] ;"""
+            """ then cd '/remote_dir/' ; bash -l  ; else echo '  ** The directory' ; """
+            """echo '  ** /remote_dir/' ; echo '  ** seems to have been deleted, I logout...' ; fi" """
+        )
+        assert cmd_str == expected_str

tests/transports/test_ssh.py

@@ -55,3 +55,16 @@ def test_no_host_key(self):
 
         # Reset logging level
         logging.disable(logging.NOTSET)
+
+
+def test_gotocomputer():
+    """Test gotocomputer"""
+    with SshTransport(machine='localhost', timeout=30, use_login_shell=False, key_policy='AutoAddPolicy') as transport:
+        cmd_str = transport.gotocomputer_command('/remote_dir/')
+
+        expected_str = (
+            """ssh -t localhost   "if [ -d '/remote_dir/' ] ;"""
+            """ then cd '/remote_dir/' ; bash  ; else echo '  ** The directory' ; """
+            """echo '  ** /remote_dir/' ; echo '  ** seems to have been deleted, I logout...' ; fi" """
+        )
+        assert cmd_str == expected_str