arun11299 / cpp-subprocess

Subprocessing with modern C++
Other
449 stars 90 forks source link

Issue with quotes on windows #90

Open santaclose opened 11 months ago

santaclose commented 11 months ago

I'm trying to run this command explorer /select,"C:\Windows" using subprocess::call, it should open a file explorer window with the "Windows" folder selected. However, the library always ends up surrounding all arguments with double quotes, and my double quotes are prepended with an extra backslash, causing the file explorer to open the "Documents" folder (no idea why). So, how should I use the library to run this specific command? I tried passing both, a vector and a string to the call command. Maybe something needs to be tweaked so arguments are not always surrounded with double quotes? image image

santaclose commented 11 months ago

This is how I'm doing it: image image

arun11299 commented 11 months ago

@santaclose Is your last PR related to this ? Windows support has been a 100% community effort for this repo since I haven't had a windows machine for far too long. Appreciate if you could find the issue and open a PR.

On a side note, you could try raw string

std::string command = R"(explorer /select,"C:\\Windows\")";
santaclose commented 11 months ago

Your code snippet with he raw string didn't work either, I'll try to figure out what windows is doing with the arguments. My latest commit might not be the best solution. image

I know that it works if szCmdLine ends up containing this exact string: explorer /select,"C:\Windows" , which also works from the command prompt.

santaclose commented 11 months ago

So, explorer /select,"C:\Windows" works and explorer "/select,C:\Windows" doesn't work, and I don't know why. I made a little python script just to se what the process was receiving as arguments, and for both it should be the same 🤔. Maybe these windows apps do something different.

image

santaclose commented 11 months ago

Seems to me the issue can be solved just by removing \" from the list of characters that need quoting: L" \t\n\v\"" --> L" \t\n\v" image

But there must be a reason why @xoviat added it. And also, why was the force argument always being set to true?

xoviat commented 11 months ago

This may have been implemented incorrectly. See here for the correct implementation: https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way

xoviat commented 11 months ago

In fact it appears that the initial implementation was correct and your PR may have introduced a bug. It also appears that you are attempting to use the shell functionality on Windows which is not currently supported.

santaclose commented 11 months ago

thanks for the info, the only commit that was meant to be merged was the one with the call function overload, the solution for the quote issue is something temporary I pushed to my branch.

Then, from what you say, I understand I need shell functionality to run the explorer executable with the select argument? why is this?

xoviat commented 11 months ago

With the code before your PR was merged, you should have been able to pass the executable and arguments as an initializer list, not as one command line string. If that was not the case I can look into the issue further.

santaclose commented 11 months ago

you mean like this? subprocess::call("explorer", "/select,\"" + path.u8string() + "\""); fails to compile with this error:

1>C:\Users\san\Desktop\scex\vendor\cpp-subprocess\subprocess.hpp(1377,7): error C2668: 'subprocess::detail::ArgumentDeducer::set_option': ambiguous call to overloaded function

this is at commit af23f338801ed19696da42b1f9b97f8e21dec5d6

santaclose commented 11 months ago

for reference, this python code seems to work fine:

import subprocess
subprocess.run(r'explorer /select,"C:\Windows"')

i think this library should work when given that same string

xoviat commented 11 months ago

see here for example: https://github.com/arun11299/cpp-subprocess/pull/91/files#diff-48ccdd15ee47b10be5729d81fb1ee3d6b0a37e113ffc53877a203300d8188b4dR43

i think this library should work when given that same string

in principle I agree. however, I think the more important functionality is to sanitize arguments when given a list. this is a large part of the reason I added this in the first place and hard to do right.

xoviat commented 11 months ago

I'm not sure exactly what python does but it may select behavior based on whether it's passed a list or not.

santaclose commented 11 months ago

yeah probably. Also, Is there a reason why you were forcing quotes? that was pretty much bypassing most of the function logic.

xoviat commented 11 months ago

@arun11299 I will work on this more after the CI pr is merged. I think it's important to have CI running first to prevent regressions.