Created attachment 102759 [details] [review]
patch

Created attachment 102957 [details] [review]
patch

Updated patch, previous one was broken if envp was non-null.

I figure the recommended way to spawn processes without console is to patch their binaries:
http://mail.gnome.org/archives/gtk-app-devel-list/2008-January/msg00117.html
In that case this patch isn't going to fly probably.

Hey, don't despair yet. This bug has been open for less than a month, for chrissake.

Could you tell us in detail exactly what scenario it is that you intend to fix, please. I think I know, but please do write it up.

The intention in gspawn-win32.c is that if the process already has a console (either because it is a console process and hasn't done anything special, or it is a "GUI" app but has allocated itself a console), then the gspawn-win32-helper-console.exe (which, as the name indicate, is the "console" version) is used, and it should then use the same console as the calling process. 

On the other hand if the process does not have a console (either because it is a "GUI" app that hasn't done anything special, or it is a console app that has got rid of its console), then gspawn-win32-helper.exe is used, and it is a "GUI" app it doesn't open any console either.

Does the above not work? Note that I said nothing yet about the actual process that the calling code wants to start yet.

Now, if the actual process the user then wants to start is a "console" or "GUI" app will then direct whether it needs a console window or not. I have on purpose let it be that program's own business whether it wants a console or not. But you probably are right (if that is what you want), that it would be useful if there was a way in GLib to force the actual started process to never have any console in any case even if it wants one.

There are some issues with your patch, though.

Firstly, why did you hide the fix for the bug that read_helper_report() forgot to set the GError properly in one case in this otherwise unrelated patch? (Luckily I happened to notice that problem a week ago while working on another roblem in gspawn-win32.c.) You should have opened a separate bug for this issue and not silently slipped the fix into this unrelated patch.

Secondly, if we use CreateProcessW() directly and not the C library spawn functions, we should inherit the inheritable C file descriptor to HANDLE mapping just like the C library does. (Then the trick to pass HANDLEs instead of file desriptors on the command line will be unnecessary, too.)

Unfortunately I am not sure whether it is possible to do it in the same way, as used code has no knowledge of which C file descriptors are marked to be noninheritable, does it? Isn't that information internal to the C library only? I would say that inheriting C file descriptors is a feature apps certainly expect to work and not something we can regress away. Of course, it could be documented that if one uses the G_SPAWN_WIN32_NO_CONSOLE_EVER_DAMMIT flag (or some other combination of stuff, see below), then C file descriptors are not inherited. But that does sound a bit random.

And if we start using CreateProcessW() directly in this always-hide-console case, we probably should do it also in other cases where possible. For instance, if the started process should have a different working directory than the calling process, but otherwise the helper process wouldn't needed, then if we use CreateProcessW() we don't need the helper process.

All in all, I hope you realize from this much too verbose comment that the issue is not as clear-cut as it perhaps seems to you...
 

(In reply to comment #5)
> Could you tell us in detail exactly what scenario it is that you intend to fix,
> please. I think I know, but please do write it up.
[snip]
> Does the above not work? Note that I said nothing yet about the actual process
> that the calling code wants to start yet.

I am running a console application (such as grep.exe) from a GUI application, with redirected stdin/stdout/stderr, so using g_spawn_* I get a console window which appears, displays nothing, waits and disappears.

> Now, if the actual process the user then wants to start is a "console" or "GUI"
> app will then direct whether it needs a console window or not. I have on
> purpose let it be that program's own business whether it wants a console or
> not. But you probably are right (if that is what you want), that it would be
> useful if there was a way in GLib to force the actual started process to never
> have any console in any case even if it wants one.

That's the thing, not all console programs *want* console. Many programs do want stdin/stdout or are aware that their standard input and output can be redirected, but few want the win32 console window. And as I am running those programs, I know for sure whether *I* want a console.

> There are some issues with your patch, though.
> 
> Firstly, why did you hide the fix for the bug that read_helper_report() forgot
> to set the GError properly in one case in this otherwise unrelated patch?
> (Luckily I happened to notice that problem a week ago while working on another
> roblem in gspawn-win32.c.) You should have opened a separate bug for this issue
> and not silently slipped the fix into this unrelated patch.

I didn't hide it, it hid from me!


> Secondly, if we use CreateProcessW() directly and not the C library spawn
> functions, we should inherit the inheritable C file descriptor to HANDLE
> mapping just like the C library does. (Then the trick to pass HANDLEs instead
> of file desriptors on the command line will be unnecessary, too.)
> 
> Unfortunately I am not sure whether it is possible to do it in the same way, as
> used code has no knowledge of which C file descriptors are marked to be
> noninheritable, does it? Isn't that information internal to the C library only?

Even if we knew which file descriptors are inheritable, it'd be impossible to recreate them in the child process, no?

> I would say that inheriting C file descriptors is a feature apps certainly
> expect to work and not something we can regress away. Of course, it could be
> documented that if one uses the G_SPAWN_WIN32_NO_CONSOLE_EVER_DAMMIT flag (or
> some other combination of stuff, see below), then C file descriptors are not
> inherited. But that does sound a bit random.

Yes it does. It's a downside. But I don't think it's a big deal (since this hidden-console thing is a pure addition).

> And if we start using CreateProcessW() directly in this always-hide-console
> case, we probably should do it also in other cases where possible. For
> instance, if the started process should have a different working directory than
> the calling process, but otherwise the helper process wouldn't needed, then if
> we use CreateProcessW() we don't need the helper process.

Sounds right.

> All in all, I hope you realize from this much too verbose comment that the
> issue is not as clear-cut as it perhaps seems to you...

What seemed clear-cut to me is that you truly believed that patching a binary was a way to spawn processes with hidden console ;)

Just to chime in -- what would make sense from my point of view is a mode that didn't inherit file handles / descriptors mode other than stdin/stdout/stderr.  This is the case I use all of the time and maps to the win32 api quite easily.  I think code for this case would be much easier to understand and debug.

Apologies if this has already been discussed or implemented -- I haven't had time to check out what is in svn now.

(In reply to comment #7)
> Just to chime in -- what would make sense from my point of view is a mode that
> didn't inherit file handles / descriptors mode other than stdin/stdout/stderr. 

Is it possible? STARTUPINFO docs say you have to use bInheritHandles = TRUE in CreateProcess() to be able to pass stdin/stdout handles to the child process. But that argument will also make the child process inherit all inheritable handles (and all handles created under the hood by open() and friends must be inheritable).

In any case making child process not inherit anything but stdin/out/err is what glib *should* do when you don't use G_SPAWN_LEAVE_DESCRIPTORS_OPEN, so if it's possible then of course glib should do it. I think.

msdn is down so I can't look right now, but I will try to find time to do so.  It looks as though the direct win32 api functions that create handles makes them non-inheritable by default, but the CRT does.  I _think_ there should be a way to make this work, but there may not be a way to do it.

(In reply to comment #9)
> msdn is down so I can't look right now, but I will try to find time to do so. 
> It looks as though the direct win32 api functions that create handles makes
> them non-inheritable by default, but the CRT does.  I _think_ there should be a
> way to make this work, but there may not be a way to do it.

Hmm, I now think the above is wrong -- there seems to be no way to only inherit stdin / stdout / stderr if those handles are not stdin / stdout / stderr of the parent process.  The closest I found to a clear confirmation of this is http://support.microsoft.com/kb/315939  The kb article does point to a workaround for the not G_SPAWN_LEAVE_DESCRIPTORS_OPEN case using an intermediary process, but one would either have to lose the pid of the child or wait for the intermediate process to get the pid of the child.

> Even if we knew which file descriptors are inheritable, 
> it'd be impossible to recreate them in the child process, no?

That happens automatically when you spawn a child program if both parent and child are written in C and use the Microsoft C library. Look in dospawn.c and ioinit.c in the Microsoft C library sources, either from an old Platform SDK or Visual Studio.

But glib isn't C library, so e.g. if we know what file descriptor 18 is, glib can't create a file descriptor 18.

> if we know what file descriptor 18 is, glib can't create a file descriptor 18.

No, glib can't, but the C library can and does. So what is your point?

If a process is started using one of the spawn* or exec* class of functions in one of the Microsoft C libraries, and the process that is started uses the one or several of the Microsoft C libraries (because the .exe is linked to a such and/or one or several of the DLLs that the process uses is linked to a such) then in the started process's C libraries the file descriptor will be recreated.

This won't work if the process is started directly with CreateProcess() without taking care of the file descriptor inheritance. And then again, taking care of file descriptor inheritance properly requires knowing which file descriptors are set to be noninheritable. And that information is available only inside the C library.

I am not strictly against the functionality proposed in the patch, but as the patch current is, it is not acceptable. 

The patch adds the flag G_SPAWN_WIN32_HIDDEN_CONSOLE which is defined only #ifdef G_OS_WIN32. So thus clearly code that intends to use this flag is either a priori for Windows only, or it needs to have #ifdefs. Thus the code that would use the proposed G_SPAWN_WIN32_HIDDEN_CONSOLE flag could as well call CreateProcess() itself and take the respondibility of knowing what is going on.

It would require adding some amount of exceptions and verbiage to the documentation (which already lacks mentions of many of the Windows details) to make it clear that if the proposed G_SPAWN_WIN32_HIDDEN_CONSOLE flag is used then many of the other flags are meaningless. And if they are, I repeat, what is the point then of using g_spawn* in the first place, why not just call CreateProcess() in the code itself.

My take on this issue is this:

The g_spawn* API does not abstract all possible details and tricks related to subprocess creation available on the various platforms. And some of the g_spawn* API is clearly designed from a UNIX point of view, like the child_setup function which makes sense only on UNIX with its fork(). If GLib-using code needs platform-specific things to happen when creating a subprocess, go ahead and use ifdefs in the code and call the actual platform-specific subprocess creation APIs instead. Sorry. Should probably resolve this as WONTFIX.

(In reply to comment #13)
> ... Thus the code that
> would use the proposed G_SPAWN_WIN32_HIDDEN_CONSOLE flag could as well call
> CreateProcess() itself and take the respondibility of knowing what is going on.

And code using g_spawn can just use fork() and exec(), piece of cake. Sorry, "as well" is bull. I sure can call CreateProcess; but it's a huge difference for me whether to add three lines of code with the flag (two of them being #ifdef and #endif) or to implement spawning process and attaching GIOChannel and figuring out whether child watch will work with what I got from CreateProcess and I don't know what else.

(In reply to comment #13)
> Sorry. Should probably resolve this as WONTFIX.

Done.

So, do we consider it normal that the empty console flashes up and disappears? Is this something that should be documetned? 

Murray: no, it is definitely annoying. A well thought out patch that takes all points discussed above into consideration would certainly be considered. Not a patch that just happens to work for some trivial case but in general breaks implied or explicit API contracts.

Could you demonstrate how to make file descriptors inherited when you use CreateProcess? That might answer your question from #11 and #13. 
A new flag is 100% backward-compatible, breaks nothing, works always; well thought out patch reimplementing g_spawn with CreateProcess will not be backward-compatible. What exactly do you mean by that good and nice patch?

See dospawn.c. You will notice that not all information about C file handles is available outside the C library.

As I already said, as the flag is Win32-only, any code that uses it would obviosuly need to do that inside #ifdef G_OS_WIN32, and then it could as well call CreateProcess() directly. The flag should be made cross-platform (but a no-op on Unix). And its exact semantics of it should be documented, including the fact that using it breaks C file descriptor inheritance.

So, all this patch needs is making the G_SPAWN_WIN32_HIDDEN_CONSOLE a no-op on non-Win32, and documenting the fact that it won't work with descriptor inheritance? And after that we can at least finally live happily without our flashing console windows?

Created attachment 163607 [details] [review]
An updated patch

I'm attaching an updated patch. What I did:
* updated it so it applies cleanly against GLib git master;
* made G_SPAWN_WIN32_HIDDEN_CONSOLE defined on all platforms (ignored under non-win32);
* added some (hopefully correct) documentation describing the flag.

I did a compilation test but didn't do anything beyond that.

Isn't still the problem that I mentioned in comment #5 present in the patch, i.e. C file descriptors won't be inherited in the code path that uses CreateProcess() directly?

I wonder how secret Microsoft considers the way that is implemented? Probably they would not mind if one duplicated the functionality in the C library that makes C file descriptors inherited. But I am not a lawyer...

(I.e. the "secret" is how the cbReserved2 and lpReserved2 fields in the STARTUPINFO struct is used to pass the mapping from inherited C file descriptors to Win32 file handles. Se comment in ioinit.c in the Microsoft C library sources.)

(About finding out which C file descriptos are supposed to be inherited and which not, I take back what I said in comment #5. One can know that indirectly. It seems that the C library is careful to match the inheritability of C file descriptors with the inheritability of the corresponding Win32 file handles when creating such mappings (in open(), pipe(), and dup() at least). And one can easily get at the latter information.)

But if we document clearly that using G_SPAWN_WIN32_HIDDEN_CONSOLE will mean C file descriptors other than 0, 1 and 2 won't be inherited, I guess that is OK, even if ugly.

Created attachment 163611 [details] [review]
An updated patch (clearer docs)

I'm attaching a new patch - same as earlier, but the documentation clarifies stdin/stdout/stderr inheritance.

Tor, regarding your point about descriptor inheritance: I didn't touch the original implementation (so no thoughts about how to implement what you said), but the fact that no descriptors are inherited is documented already (see the patch).

For me, the result is that finally I can spawn third-party CLI programs from my application (e.g. smartctl, tw_cli (this one has no source available)). I don't need the extra functionality as long as I can read their output. I'm sure lots of people use these functions this way (at least judging by google code search) and are willing to deal with the file descriptor loss. Since the existing functionality is untouched, I don't see anything awfully wrong here.

Any plan to include Alexander's fix in Glib (with an updated patch against the latest Glib of course)?

Having the flashing console every time a process is spawned is really bothering people, furthermore if the spawned process it's a long process and someone closes the console the process will terminate.

PyChess is a GUI program written in pygobject. It starts chess engines (console programs!) to play/analyze with GLib.spawn_async(), but don't want to show their console at all. It's ok on Linux, but not on Windows. Consider applying Alexander's patch, please!

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/119.