Bug when used for paths with spaces

[pderouen]

This code does not work when the command contains a path with spaces:

o2/rust/origen/src/core/os.rs

Lines 24 to 26 in c4662b5

	let mut c = Command::new("cmd");
	c.arg("/C").arg(&cmd);
	return c;

According to this comment using cmd /C command_to_execute argument is "normally" not needed. But, I suppose we have a case where it is needed.
https://stackoverflow.com/questions/44757893/cmd-c-doesnt-work-in-rust-when-command-includes-spaces

If I construct the return value exactly the same way as it's done for Linux, I don't have any issue. What led to the need for executing like this on Windows?

[pderouen]

I'm working on this on the CmdSpaceBug branch. I'm able to get through installation of poetry now when running origen setup. But, the command string for origen i is getting setup wrong.

Error (quotes are being placed around " run python -c "):

'C:\Users\Paul Standard/.poetry/bin/poetry" run python -c "from' is not recognized as an internal or external command, operable program or batch file.


Correct cmd line (origen interactive starts):

cmd /C C:\Users\Paul^ Standard/.poetry/bin/poetry run python -c "from origen.boot import origen; origen('interactive');"


[coreyeng]

I'm not a fan of Rust's command builder. The second line states: A default configuration can be generated using Command::new(program), where program gives a path to the program to be executed., and in the example on the page is where is the /C came from. Rust doesn't just run the commands as if you were at the terminal like every other language does.

In the case of running setup, I had to have my environment setup very particularly or it wouldn't work, which annoyed me. The /C helps all of this. For supporting spacing, you might just need to parse the input before running the command and escape them, which it looks like you've done.

Part of the problem I was running into was Rust would eat the quotes in some cases, but not others. I never really figured out what it was doing, I just got something that worked. My only goal for #10 was just to get O2 from immediately panicking/crashing the terminal (depending on what command was run) to being usable.

[pderouen]

Fair enough. I'm fighting through origen i at the moment. The ^ escape can't be a global fix. It will stop the interpreter from seeing a space as the divider between arguments. I think my issue with the interactive launcher is related to quote escapes.

[pderouen]

I just re-read the issue thread in the rust repo rust-lang/rust#29494

...it's been open for 5 years! I would assume that everybody who cares created a workaround using CreateProcessW, whatever that is, and then ignored the issue.

[pderouen]

CreateProcessW doesn't seem to be documented anywhere. How about bypassing the underlying bug completely by dumping the command and arguments into a batch file and then running the batch file? The underlying bug is in the way Rust builds the final command string. There's no way to reliably escape the strings... and no way to stop Rust from trying to do it.

[coreyeng]

I was really close to doing that myself. I decided against it because I wasn't sure how well that'd go over. Creating and executing a .bat seems like a red flag. I think another option I looked into was setting an environment variable then executing from that. I think we could also dump contents to a Python file, then use that .py file as an argument.

The \C should treat everything after it like standard powershell though. Can you not just escape the spaces there? Once I started using \C, most things worked themselves out.

[pderouen]

I can't get the command string for interactive origen to work no matter how many escape characters I use. I can get the whole command to print when it errors out with this code:

arg.replace(";", "^;").replace(" ", "^ ").replace("(", "^(")

But, no matter what I do Rust is inserting some extra double quotes. I guess I'll look into .py file creation instead.

[coreyeng]

My first thought on a .bat is that we should avoid it. If I was using something that was creating a .bat and running it, I'd be a bit skeptical of what it was doing and why that'd be necessary. But, maybe others have a differing opinion. It'd definitely work and could then can be used for more than just Python commands.

[coreyeng]

I get the feeling that if we're having this much trouble with just a python -c ..., trying to automate, say, Git, will be painful and we might devolve to just using python -c "import os; os.system(...'). Rust does have some Git bindings but so did Ruby and we still had to wrap our own/extend what was already available.

DesignSync I doubt Rust would have a wrapper for.

[pderouen]

I would say that we just should not support paths with spaces. But, all of the program binaries will live in a folder named Program Files. I think my current issue is a little extra complicated because the last argument passed to poetry has to be treated as 1 argument (which is 2 lines of python code). But, it's full of spaces semicolons and parenthesis. All of which can be delimiters.

I'm not ready to throw in the towel yet.

[pderouen]

Well, I can beat it into submission if I dump the python code into a .py file and then execute it using a relative path. Is this good enough? If not, we may need to rethink the boot process, or trial and error our way into using CreateProcessW. I was able to locate it in the Rust source code. I couldn't easily tell how to use it.

For debug I'm putting the "launcher" code in work_space/.origen/launcher.py. Is there a better place?

[ginty]

Are we just talking about the Powershell case here? If so then I don't mind what hack you need to come up with.
I would say that the proposal to use a single file called launcher.py is not going to work well in an LSF or similar multiple-launch situation. Probably the filename needs a unique identifier and then maybe pass that into __origen__ and have that delete its launcher as its first operation.

[pderouen]

We're calling the windows command shell (cmd) to execute. The root of the issue is with the way Rust builds the execution string. It's attempting to provide all of the window dressing needed so that the command line is interpreted correctly by Windows. But, it's not getting it right.

After thinking about this some more, I think we should hold off on implementing a hacky solution. I think in the end we may need to make our own Windows process launcher. For development I'm fine manually launching.

[pderouen]

Hey, a breakthrough:

The only problem is that it's also launching a new shell (probably because I ran it from within powershell).

[ginty]

@pderouen your ability to persevere is second to none 👍

[pderouen]

LOL... I think it's a disorder. I don't know when to quit.

I plan to expand our existing wrapper to handle booting origen from windows using that powershell string. I'm not happy about getting the new command shell window though. But, maybe it'll do until I figure out how to manually launch the process. I couldn't find the documentation on CreateProcessW because it's not part of Rust. It's c:

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw