Merge pull request #545 from tclose/rebased-shlex-split

Split command line using quote respecting shlex.split instead of ' '.split

Author: djarecka

pydra/conftest.py

@@ -1,5 +1,6 @@
 import shutil
 import os
+import pytest
 
 os.environ["NO_ET"] = "true"
 
@@ -26,3 +27,30 @@ def pytest_generate_tests(metafunc):
         else:
             Plugins = ["cf"]
         metafunc.parametrize("plugin", Plugins)
+
+
+# For debugging in IDE's don't catch raised exceptions and let the IDE
+# break at it
+if os.getenv("_PYTEST_RAISE", "0") != "0":
+
+    @pytest.hookimpl(tryfirst=True)
+    def pytest_exception_interact(call):
+        raise call.excinfo.value
+
+    @pytest.hookimpl(tryfirst=True)
+    def pytest_internalerror(excinfo):
+        raise excinfo.value
+
+
+# Example VSCode launch configuration for debugging unittests
+# {
+#     "name": "Test Config",
+#     "type": "python",
+#     "request": "launch",
+#     "purpose": ["debug-test"],
+#     "justMyCode": false,
+#     "console": "internalConsole",
+#     "env": {
+#         "_PYTEST_RAISE": "1"
+#     },
+# }

pydra/engine/core.py

@@ -489,6 +489,7 @@ def _run(self, rerun=False, **kwargs):
         lockfile = self.cache_dir / (checksum + ".lock")
         # Eagerly retrieve cached - see scenarios in __init__()
         self.hooks.pre_run(self)
+        logger.debug(f"'%s' is attempting to acquire lock on %s", self.name, lockfile)
         with SoftFileLock(lockfile):
             if not (rerun or self.task_rerun):
                 result = self.result()
@@ -1063,6 +1064,11 @@ async def _run(self, submitter=None, rerun=False, **kwargs):
         output_dir = self.output_dir
         lockfile = self.cache_dir / (checksum + ".lock")
         self.hooks.pre_run(self)
+        logger.debug(
+            f"'%s' is attempting to acquire lock on %s with Pydra lock",
+            self.name,
+            lockfile,
+        )
         async with PydraFileLock(lockfile):
             if not (rerun or self.task_rerun):
                 result = self.result()

pydra/engine/task.py

@@ -38,10 +38,13 @@
       <https://colab.research.google.com/drive/1RRV1gHbGJs49qQB1q1d5tQEycVRtuhw6>`__
 
 """
+import platform
+import re
 import attr
 import cloudpickle as cp
 import inspect
 import typing as ty
+import shlex
 from pathlib import Path
 import warnings
 
@@ -447,7 +450,7 @@ def _command_pos_args(self, field):
             cmd_el_str = field.metadata["formatter"](**call_args_val)
             cmd_el_str = cmd_el_str.strip().replace("  ", " ")
             if cmd_el_str != "":
-                cmd_add += cmd_el_str.split(" ")
+                cmd_add += split_cmd(cmd_el_str)
         elif field.type is bool:
             # if value is simply True the original argstr is used,
             # if False, nothing is added to the command
@@ -483,10 +486,8 @@ def _command_pos_args(self, field):
                         cmd_el_str = f"{argstr} {value}"
                     else:
                         cmd_el_str = ""
-            # removing double spacing
-            cmd_el_str = cmd_el_str.strip().replace("  ", " ")
             if cmd_el_str:
-                cmd_add += cmd_el_str.split(" ")
+                cmd_add += split_cmd(cmd_el_str)
         return pos, cmd_add
 
     @property
@@ -501,9 +502,19 @@ def cmdline(self):
         if self.state:
             raise NotImplementedError
         if isinstance(self, ContainerTask):
-            cmdline = " ".join(self.container_args + self.command_args)
+            command_args = self.container_args + self.command_args
         else:
-            cmdline = " ".join(self.command_args)
+            command_args = self.command_args
+        # Skip the executable, which can be a multi-part command, e.g. 'docker run'.
+        cmdline = command_args[0]
+        for arg in command_args[1:]:
+            # If there are spaces in the arg and it is not enclosed by matching
+            # quotes, add quotes to escape the space. Not sure if this should
+            # be expanded to include other special characters apart from spaces
+            if " " in arg:
+                cmdline += " '" + arg + "'"
+            else:
+                cmdline += " " + arg
         return cmdline
 
     def _run_task(self):
@@ -519,10 +530,12 @@ def _run_task(self):
             values = execute(args, strip=self.strip)
             self.output_ = dict(zip(keys, values))
             if self.output_["return_code"]:
+                msg = f"Error running '{self.name}' task with {args}:"
                 if self.output_["stderr"]:
-                    raise RuntimeError(self.output_["stderr"])
-                else:
-                    raise RuntimeError(self.output_["stdout"])
+                    msg += "\n\nstderr:\n" + self.output_["stderr"]
+                if self.output_["stdout"]:
+                    msg += "\n\nstdout:\n" + self.output_["stdout"]
+                raise RuntimeError(msg)
 
 
 class ContainerTask(ShellCommandTask):
@@ -825,3 +838,29 @@ def container_args(self):
         cargs.extend(["--pwd", str(self.output_cpath)])
         cargs.append(self.inputs.image)
         return cargs
+
+
+def split_cmd(cmd: str):
+    """Splits a shell command line into separate arguments respecting quotes
+
+    Parameters
+    ----------
+    cmd : str
+        Command line string or part thereof
+
+    Returns
+    -------
+    str
+        the command line string split into process args
+    """
+    # Check whether running on posix or windows system
+    on_posix = platform.system() != "Windows"
+    args = shlex.split(cmd, posix=on_posix)
+    cmd_args = []
+    for arg in args:
+        match = re.match("('|\")(.*)\\1$", arg)
+        if match:
+            cmd_args.append(match.group(2))
+        else:
+            cmd_args.append(arg)
+    return cmd_args

pydra/engine/tests/test_nipype1_convert.py

@@ -34,6 +34,23 @@ class Interf_2(ShellCommandTask):
     executable = "testing command"
 
 
+class Interf_3(ShellCommandTask):
+    """class with customized input and executables"""
+
+    input_spec = SpecInfo(
+        name="Input",
+        fields=[
+            (
+                "in_file",
+                str,
+                {"help_string": "in_file", "argstr": "'{in_file}'"},
+            )
+        ],
+        bases=(ShellSpec,),
+    )
+    executable = "testing command"
+
+
 class TouchInterf(ShellCommandTask):
     """class with customized input and executables"""
 
@@ -96,6 +113,13 @@ def test_interface_executable_2():
         assert task.cmdline == "i want a different command"
 
 
+def test_interface_cmdline_with_spaces():
+    task = Interf_3(in_file="/path/to/file/with spaces")
+    assert task.executable == "testing command"
+    assert task.inputs.executable == "testing command"
+    assert task.cmdline == "testing command '/path/to/file/with spaces'"
+
+
 def test_interface_run_1():
     """testing execution of a simple interf with customized input and executable"""
     task = TouchInterf(new_file="hello.txt")

pydra/engine/tests/test_shelltask.py

@@ -4,6 +4,7 @@
 import pytest
 from pathlib import Path
 import re
+import stat
 
 from ..task import ShellCommandTask
 from ..submitter import Submitter
@@ -18,7 +19,7 @@
     MultiOutputFile,
     MultiInputObj,
 )
-from .utils import result_no_submitter, result_submitter, use_validator
+from .utils import result_no_submitter, result_submitter, use_validator, no_win
 
 if sys.platform.startswith("win"):
     pytest.skip("SLURM not available in windows", allow_module_level=True)
@@ -307,7 +308,7 @@ def test_shell_cmd_inputspec_1(plugin, results_function, use_validator, tmpdir):
     )
     assert shelly.inputs.executable == cmd_exec
     assert shelly.inputs.args == cmd_args
-    assert shelly.cmdline == "echo -n hello from pydra"
+    assert shelly.cmdline == "echo -n 'hello from pydra'"
 
     res = results_function(shelly, plugin)
     assert res.output.stdout == "hello from pydra"
@@ -356,7 +357,7 @@ def test_shell_cmd_inputspec_2(plugin, results_function, use_validator, tmpdir):
     )
     assert shelly.inputs.executable == cmd_exec
     assert shelly.inputs.args == cmd_args
-    assert shelly.cmdline == "echo -n HELLO from pydra"
+    assert shelly.cmdline == "echo -n HELLO 'from pydra'"
     res = results_function(shelly, plugin)
     assert res.output.stdout == "HELLO from pydra"
 
@@ -4749,3 +4750,62 @@ def formatter_1(in1, in2):
     # com_results = ["executable -t [in11 in2]", "executable -t [in12 in2]"]
     # for i, cr in enumerate(com_results):
     #     assert results[i] == cr
+
+
+@no_win
+def test_shellcommand_error_msg(tmpdir):
+
+    script_path = Path(tmpdir) / "script.sh"
+
+    with open(script_path, "w") as f:
+        f.write(
+            """#!/bin/bash
+                echo "first line is ok, it prints '$1'"
+                /command-that-doesnt-exist"""
+        )
+
+    os.chmod(
+        script_path,
+        mode=(
+            stat.S_IRUSR
+            | stat.S_IWUSR
+            | stat.S_IXUSR
+            | stat.S_IRGRP
+            | stat.S_IWGRP
+            | stat.S_IROTH
+        ),
+    )
+
+    input_spec = SpecInfo(
+        name="Input",
+        fields=[
+            (
+                "in1",
+                str,
+                {"help_string": "a dummy string", "argstr": "", "mandatory": True},
+            ),
+        ],
+        bases=(ShellSpec,),
+    )
+
+    shelly = ShellCommandTask(
+        name="err_msg", executable=str(script_path), input_spec=input_spec, in1="hello"
+    )
+
+    with pytest.raises(RuntimeError) as excinfo:
+        shelly()
+
+    path_str = str(script_path)
+
+    assert (
+        str(excinfo.value)
+        == f"""Error running 'err_msg' task with ['{path_str}', 'hello']:
+
+stderr:
+{path_str}: line 3: /command-that-doesnt-exist: No such file or directory
+
+
+stdout:
+first line is ok, it prints 'hello'
+"""
+    )

pydra/engine/tests/test_task.py

@@ -1124,7 +1124,7 @@ def test_docker_cmd(tmpdir):
         docky.cmdline
         == f"docker run --rm -v {docky.output_dir}:/output_pydra:rw -w /output_pydra busybox pwd"
     )
-    docky.inputs.container_xargs = ["--rm -it"]
+    docky.inputs.container_xargs = ["--rm", "-it"]
     assert (
         docky.cmdline
         == f"docker run --rm -it -v {docky.output_dir}:/output_pydra:rw -w /output_pydra busybox pwd"