code review 4439055: syscall : add a field to ProcAttr so that StartProcess ...

syscall : add a field to ProcAttr so that StartProcess can hide the executed application on windows

The SW_HIDE parameter looks like the only way for a windows GUI application to execute a CLI subcommand without having a shell windows appearing.

[brainman, 2011-04-20 06:26:48]

You solution looks OK to me. You just have to be prepared for changes in the
future. Maybe it is better to create another, Windows specific function in
syscall:

func StartProcess2(argv0 string, argv []string, env []string, dir string, si
*StartupInfo) (pi *ProcessInformation, err int)

(StartProcess2 name just a placeholder, I'm bad with names). This way you could
do whatever you need to setup "si" parameter.

I don't have any suggestions for your second problem. Perhaps someone else will
add.

Alex

[vincent.vanackere, 2011-04-20 08:28:40]

On 2011/04/20 06:26:48, brainman wrote:
> You solution looks OK to me. You just have to be prepared for changes in the
> future. Maybe it is better to create another, Windows specific function in
> syscall:
> 
> func StartProcess2(argv0 string, argv []string, env []string, dir string, si
> *StartupInfo) (pi *ProcessInformation, err int)
> 
> (StartProcess2 name just a placeholder, I'm bad with names). This way you
could
> do whatever you need to setup "si" parameter.
> 
> I don't have any suggestions for your second problem. Perhaps someone else
will
> add.
> 
> Alex

Thanks for looking at this. 

I did the code that way because I didn't want to diverge from the way it is
already done in exec_unix.go/exec_plan9.go (and therefore I'm not sure we should
add a generic StartProcess2 unless really necessary).

Also I'd like to keep the interface simple if possible and my own 
windows programming experience has shown that typically either the
STARTF_USESHOWWINDOW/SW_HIDE combination is used, or the STARTF_USESHOWWINDOW
flag is not set.

Another alternative would be to make ProcAttr look like this if you prefer :

type ProcAttr struct {
	Dir        string
	Env        []string
	Files      []int
	Si         *StartupInfo
}

After some thought I think this last option would is probably better than the
bool flag (more future-proof). Any opinion ?

[vincent.vanackere, 2011-04-20 09:51:05]

On 2011/04/20 08:28:40, vincent.vanackere wrote:

> Another alternative would be to make ProcAttr look like this if you prefer :
> 
> type ProcAttr struct {
> 	Dir        string
> 	Env        []string
> 	Files      []int
> 	Si         *StartupInfo
> }
> 
> After some thought I think this last option would is probably better than the
> bool flag (more future-proof). Any opinion ?

Forget that (this would require tedious manual setup of
Si.Std{Input|Output|Err}) and while we're there we could as well call
CreateProcess directly...

So I'm back to my initial proposal of a single boolean flag. Ok for me to submit
in the current form ? I'd like to solve my other problems with another CL...

[brainman, 2011-04-20 10:27:10]

On 2011/04/20 09:51:05, vincent.vanackere wrote:
> 
> So I'm back to my initial proposal of a single boolean flag. Ok for me to
submit
> in the current form ? ...

You're free to do whatever you like. But I've found a bug in windows
syscall.StartProcess, so I'll be changing it anyway. If you're happy to wait, I
might have solution to your first problem.

Alex

[vincent.vanackere, 2011-04-20 11:20:44]

On 2011/04/20 10:27:10, brainman wrote:
> On 2011/04/20 09:51:05, vincent.vanackere wrote:
> > 
> > So I'm back to my initial proposal of a single boolean flag. Ok for me to
> submit in the current form ? ...
> 
> You're free to do whatever you like.

Of course, but as the defacto windows go maintainer, your opinion definitely has
some weight ;-)

> But I've found a bug in windows
> syscall.StartProcess, so I'll be changing it anyway. If you're happy to wait,
I might have solution to your first problem.
> 
> Alex

I'm happy to wait (especially if this also fixes my problem !)... Can you give
more details about the StartProcess bug ?

Vincent

[brainman, 2011-04-20 11:29:30]

On 2011/04/20 11:20:44, vincent.vanackere wrote:
> 
> Of course, but as the defacto windows go maintainer, your opinion definitely
has
> some weight ;-)

It's democracy here <g>. And I'm certainly no "defacto windows go maintainer"
<g>.

> ... Can you give
> more details about the StartProcess bug ?
> 

See http://code.google.com/p/go/issues/detail?id=1718 and
http://code.google.com/p/go/issues/detail?id=1719. It appears, if we start with
blank StartupInfo instead of filling it with GetStartupInfo the problem goes
away.

Alex

[vincent.vanackere, 2011-04-20 12:17:35]

On Wed, Apr 20, 2011 at 1:29 PM,  <alex.brainman@gmail.com> wrote:
> On 2011/04/20 11:20:44, vincent.vanackere wrote:
>
>> Of course, but as the defacto windows go maintainer, your opinion definitely
has
>> some weight ;-)
>
> It's democracy here <g>.

...I certainly hope it's more meritocracy than democracy (else a
sudden influx of inexperienced developers could lead to chaos if all
voices were really considered equal) <g>.

> And I'm certainly no "defacto windows go maintainer" <g>.

Ok, make it "one of the defacto windows go maintainers" then.

>> ... Can you give
>> more details about the StartProcess bug ?
>
>
> See http://code.google.com/p/go/issues/detail?id=1718 and
> http://code.google.com/p/go/issues/detail?id=1719. It appears, if we
> start with blank StartupInfo instead of filling it with GetStartupInfo
> the problem goes away.
>
> Alex

Starting with a blank StartupInfo doesn't fix my problem
unfortunately... BTW I've just checked into a lot of local windows
code here and can confirm that :
- every occurence initialize it from zero and none from GetStartupInfo
- almost half of the call I saw in my codebase used
STARTF_USESHOWWINDOW + SW_HIDE

Thanks for your help,

Vincent

[brainman, 2011-04-20 12:26:29]

I think your original proposal is good enough for now.
LGTM.

Alex

[vincent.vanackere, 2011-04-20 12:39:47]

On Wed, Apr 20, 2011 at 2:26 PM,  <alex.brainman@gmail.com> wrote:
> I think your original proposal is good enough for now.
> LGTM.
>

Thanks I'll submit ASAP then. One last thing : do you intend to remove
the wrong GetStartupInfo call in a separate CL ?
If you prefer I can remove this call in the same CL (with proper
credits and references to the bugs of course).

Best regards,

Vincent

[brainman, 2011-04-20 12:46:15]

On 2011/04/20 12:39:47, vincent.vanackere wrote:
> Thanks I'll submit ASAP then. 

You have no access to main repo. You have to wait for someone from Golang team
to LGTM. Then we'll submit your change.

> ... do you intend to remove
> the wrong GetStartupInfo call in a separate CL ?

Yes. http://codereview.appspot.com/4435059/

Alex

[vincent.vanackere, 2011-04-20 12:57:55]

Hello brainman, golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-04-20 15:51:08]

I don't think we want any common structs like os.ProcAttr to differ between
platforms.

Whatever new field gets added to ProcAttr should probably exist on all
operating systems and be documented well (no-op except on Windows?).

Also, document the zero value of "HideWindow" (or whatever it's named...
"ShowWindow") and consider what effect the default of zero will have on
Windows ports of code that was written without consideration for Windows.
Maybe hide should be the default?  Maybe zero should mean "automatically try
to guess" and it should actually be an enum and not a bool?


On Wed, Apr 20, 2011 at 5:57 AM, <vincent.vanackere@gmail.com> wrote:

> Hello brainman, golang-dev@googlegroups.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> http://codereview.appspot.com/4439055/
>

[rsc1, 2011-04-20 16:06:49]

This is changing syscall.ProcAttr, not os.ProcAttr.
That's fine.

[bradfitz, 2011-04-20 16:20:42]

ah, whoops.  Didn't notice the filename.

I probably missed the plan for os.StartProcess' interface in another thread.

On Wed, Apr 20, 2011 at 9:06 AM, Russ Cox <rsc@google.com> wrote:

> This is changing syscall.ProcAttr, not os.ProcAttr.
> That's fine.
>

[vincent.vanackere, 2011-04-21 08:44:29]

Hello brainman, golang-dev@googlegroups.com, bradfitzgo, rsc1 (cc:
golang-dev@googlegroups.com),

Please take another look.

[vincent.vanackere, 2011-04-21 08:48:07]

On 2011/04/21 08:44:29, vincent.vanackere wrote:
> Hello brainman, mailto:golang-dev@googlegroups.com, bradfitzgo, rsc1 (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

(updated after CL 4435059 went in, no modification)

[rsc1, 2011-04-21 13:34:56]

LGTM

Leaving for Alex to review and submit.

[brainman, 2011-04-21 13:50:02]

Please complete a CLA as described at
http://golang.org/doc/contribute.html#copyright

Thank you.

Alex

[vincent.vanackere, 2011-04-21 13:57:04]

I filled the CLA yesterday... should I do it again ?

Vincent

On Thu, Apr 21, 2011 at 3:50 PM,  <alex.brainman@gmail.com> wrote:
> Please complete a CLA as described at
> http://golang.org/doc/contribute.html#copyright
>
> Thank you.
>
> Alex
>
> http://codereview.appspot.com/4439055/
>

[rsc1, 2011-04-21 14:06:11]

On Thu, Apr 21, 2011 at 09:56, Vincent Vanackere
<vincent.vanackere@gmail.com> wrote:
> I filled the CLA yesterday... should I do it again ?

I see it; processing now.

[brainman, 2011-04-21 14:12:10]

*** Submitted as http://code.google.com/p/go/source/detail?r=183d1c6cde7b ***

syscall : add a field to ProcAttr so that StartProcess can hide the executed
application on windows

The SW_HIDE parameter looks like the only way for a windows GUI application to
execute a CLI subcommand without having a shell windows appearing.

R=brainman, golang-dev, bradfitzgo, rsc1
CC=golang-dev
http://codereview.appspot.com/4439055

Committer: Alex Brainman <alex.brainman@gmail.com>