[BUG] Paths with spaces can cause executor to fail

[ebrahimebrahim]

Describe the bug If k-wave-python is installed to a python environment in a directory that contains a space, then execution can fail because the path to the executable can be parsed incorrectly

To Reproduce Steps to reproduce the behavior:

1. Create a venv inside a folder whose name contains a space
2. Install k-wave-python to that venv
3. Try to run_simulation

A subprocess.CalledProcessError should be raised.

This occurred on Windows.

Possible fix

I believe the problem is shell=True here: https://github.com/waltsims/k-wave-python/blob/bfd998b3ae6c1a8f2e70e14b02d0efd36882a680/kwave/executor.py#L39 Normally subprocess.Popen is smart enough to handle paths with spaces in them, but this is prevented when shell=True.

[waltsims]

Hi @ebrahimebrahim,

Thanks for bringing this to our attention. This is of course something that should not happen. I don't have a Windows development machine, so it would be very helpful if you could test your proposed changes locally and open a PR. Would you be open to that?

[ebrahimebrahim]

Yes happy to look into it! I'll see if it's a quick fix like what I suggested and open a PR if so. @peterhollender caught this bug while testing an application we were building that uses k-wave-python.

[ebrahimebrahim]

Was able to reproduce this issue on my ubuntu 22.04 system, so not windows specific:

git clone git@github.com:waltsims/k-wave-python.git
mkdir "a folder with spaces"
cd "a folder with spaces"
python -m venv .venv
. .venv/bin/activate
pip install ../k-wave-python/
python ../k-wave-python/examples/us_bmode_phased_array/us_bmode_phased_array.py
# (error described in issue description)

[djps]

I can confirm that this is the case.

I think there is another option proposed here

I will test the following which works for me when there is a space in the path to the executable.

 command = (
            f"{self.execution_options.system_string}" f'"{self.execution_options.binary_path}" -i {input_filename} -o {output_filename} {options}'
        )

[ebrahimebrahim]

The problem with adding quotes manually is that if quotes are already there then it won't work, so you have to check if there are already quotes ... and it's just brittle. That's why I preferred getting out of shell=True to fix the issue cleanly... but it seems that self.execution_options.system_string contains some environment variable setup which requires shell=True. The way we should probably handle environment variables is probably to pass them to subprocess.Popen's env parameter ... but I see that SimulationExecutionOptions.system_string is constructed in some complex platform-dependent way that I wouldn't want to break.

So with all that maybe the easiest fix is just to manually add the quotes and keep shell=True like you are suggesting @djps . If it works will you make the PR @djps ?

[waltsims]

The problem with adding quotes manually is that if quotes are already there then it won't work, so you have to check if there are already quotes ... and it's just brittle. That's why I preferred getting out of shell=True to fix the issue cleanly... but it seems that self.execution_options.system_string contains some environment variable setup which requires shell=True. The way we should probably handle environment variables is probably to pass them to subprocess.Popen's env parameter ... but I see that SimulationExecutionOptions.system_string is constructed in some complex platform-dependent way that I wouldn't want to break.

So with all that maybe the easiest fix is just to manually add the quotes and keep shell=True like you are suggesting @djps . If it works will you make the PR @djps ?

I agree with you. I have approved the brittle fix, but passing the environment variables to subprocess separately is the right way to move forward. I will open a new issue.