Support spaces in run cmd args (#2330)

## Changes
Current implementation only accepts a full command line, which it splits
on spaces. This breaks when the command line contains arguments with
spaces, such as `'some stuff'` (note the enclosing quotes)
The new implementation adds support for argument lists, which are passes
'as is' to `Popen`

### Linked issues
Resolves #1859 

### Functionality
None

### Tests
- [x] enabled integration tests

---------

Co-authored-by: Eric Vergnaud <[email protected]>
Co-authored-by: Andrew Snare <[email protected]>

Author: ericvergnaud

src/databricks/labs/ucx/framework/utils.py

@@ -29,8 +29,9 @@ def escape_sql_identifier(path: str, optional: bool | None = True) -> str:
     return ".".join(escaped)
 
 
-def run_command(command: str) -> tuple[int, str, str]:
-    logger.info(f"Invoking command: {command}")
-    with subprocess.Popen(command.split(), stdout=subprocess.PIPE, stderr=subprocess.PIPE) as process:
+def run_command(command: str | list[str]) -> tuple[int, str, str]:
+    args = command.split() if isinstance(command, str) else command
+    logger.info(f"Invoking command: {args!r}")
+    with subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) as process:
         output, error = process.communicate()
         return process.returncode, output.decode("utf-8"), error.decode("utf-8")

src/databricks/labs/ucx/source_code/python_libraries.py

@@ -2,7 +2,6 @@
 from __future__ import annotations
 
 import logging
-import shlex
 import tempfile
 import zipfile
 from collections.abc import Callable
@@ -25,7 +24,9 @@
 class PythonLibraryResolver(LibraryResolver):
     # TODO: https://github.com/databrickslabs/ucx/issues/1640
 
-    def __init__(self, allow_list: KnownList, runner: Callable[[str], tuple[int, str, str]] = run_command) -> None:
+    def __init__(
+        self, allow_list: KnownList, runner: Callable[[str | list[str]], tuple[int, str, str]] = run_command
+    ) -> None:
         self._allow_list = allow_list
         self._runner = runner
 
@@ -71,13 +72,19 @@ def _resolve_libraries(path_lookup: PathLookup, *libraries: str) -> list[str]:
         return libs
 
     def _install_pip(self, *libraries: str) -> list[DependencyProblem]:
-        install_command = (
-            f"pip --disable-pip-version-check install {shlex.join(libraries)} -t {self._temporary_virtual_environment}"
-        )
-        return_code, stdout, stderr = self._runner(install_command)
+        args = [
+            "pip",
+            "--disable-pip-version-check",
+            "install",
+            *libraries,
+            "-t",
+            str(self._temporary_virtual_environment),
+        ]
+        return_code, stdout, stderr = self._runner(args)
         logger.debug(f"pip output:\n{stdout}\n{stderr}")
         if return_code != 0:
-            problem = DependencyProblem("library-install-failed", f"'{install_command}' failed with '{stderr}'")
+            command = " ".join(args)
+            problem = DependencyProblem("library-install-failed", f"'{command}' failed with '{stderr}'")
             return [problem]
         return []
 

tests/integration/source_code/test_libraries.py

@@ -48,7 +48,6 @@ def test_build_notebook_dependency_graphs_installs_pypi_packages(simple_ctx):
     assert maybe.graph.path_lookup.resolve(Path("hyperopt"))
 
 
-@pytest.mark.xfail(reason="Spaces in path are not handled by subprocess when quoted")
 @pytest.mark.parametrize("notebook", ("pip_install_demo_wheel_with_spaces_in_target_directory",))
 def test_build_notebook_dependency_graphs_fails_installing_when_spaces(simple_ctx, notebook):
     ctx = simple_ctx.replace(path_lookup=MockPathLookup())

tests/unit/source_code/test_python_libraries.py

@@ -7,7 +7,8 @@
 
 def test_python_library_resolver_resolves_library(mock_path_lookup):
     def mock_pip_install(command):
-        assert command.startswith("pip --disable-pip-version-check install anything -t")
+        command_str = command if isinstance(command, str) else " ".join(command)
+        assert command_str.startswith("pip --disable-pip-version-check install anything -t")
         return 0, "", ""
 
     python_library_resolver = PythonLibraryResolver(KnownList(), mock_pip_install)