Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dwFlags on CreateProcessW calls #54591

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Conversation

WoLfulus
Copy link
Contributor

@WoLfulus WoLfulus commented Nov 4, 2021

Command prompt is still being shown when calling os.execute on Windows. The previous flags always results in 0 because flags should be merged with an OR operator.

Bugsquad edit: May help with #52242.

@WoLfulus WoLfulus requested a review from a team as a code owner November 4, 2021 12:53
@mhilbrunner mhilbrunner added this to the 4.0 milestone Nov 4, 2021
@mhilbrunner
Copy link
Member

This is the right fix according to the official docs. Will make sure and test.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 4, 2021
@mhilbrunner
Copy link
Member

mhilbrunner commented Nov 4, 2021

Built & tested, seems to work fine. Funnily enough, I don't actually seem to get a console window with current master though? Not sure why.

Anyway, this is correct, the current code doesn't work.

Edit: does the additional console window without this only show up in release exports? May be the case the otherwise existing default console window gets inherited. Will check.

@akien-mga akien-mga merged commit 197169b into godotengine:master Nov 4, 2021
@akien-mga
Copy link
Member

Thanks!

@WoLfulus
Copy link
Contributor Author

WoLfulus commented Nov 4, 2021

Built & tested, seems to work fine. Funnily enough, I don't actually seem to get a console window with current master though? Not sure why.

Anyway, this is correct, the current code doesn't work.

Edit: does the additional console window without this only show up in release exports? May be the case the otherwise existing default console window gets inherited. Will check.

@mhilbrunner It's deeper than that though. This doesn't fix all the problems related to command prompts showing up (for me at least), it fixes only the flag passing that was wrong.

The main problem when trying to launch an executable from godot (I'm building a game launcher) is that OS.execute will always pass a non-null r_pipe value

Error err = ::OS::get_singleton()->execute(p_path, args, &pipe, &exitcode, p_read_stderr);

This makes the process execute with _wpopen

FILE *f = _wpopen((LPCWSTR)(command.utf16().get_data()), L"r");

instead of CreateProcessW + Win32 pipe functions.

I had to comment this whole block and start using a custom built template to get rid of that command prompt as I'm not interested in the output, only the exit codes.

if (r_pipe) {
if (read_stderr) {
command += " 2>&1"; // Include stderr
}
// Add extra quotes around the full command, to prevent it from stripping quotes in the command,
// because _wpopen calls command as "cmd.exe /c command", instead of executing it directly
command = _quote_command_line_argument(command);
FILE *f = _wpopen((LPCWSTR)(command.utf16().get_data()), L"r");
ERR_FAIL_COND_V_MSG(!f, ERR_CANT_OPEN, "Cannot create pipe from command: " + command);
char buf[65535];
while (fgets(buf, 65535, f)) {
if (p_pipe_mutex) {
p_pipe_mutex->lock();
}
(*r_pipe) += String::utf8(buf);
if (p_pipe_mutex) {
p_pipe_mutex->unlock();
}
}
int rv = _pclose(f);
if (r_exitcode) {
*r_exitcode = rv;
}
return OK;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants