code review 102320044: syscall: implement syscall.Getppid() on Windows

syscall: implement syscall.Getppid() on Windows

Also added a test to verify os.Getppid() works across all platforms

[inconshreveable, 2014-06-12 19:04:10]

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

I'd like you to review this change to
https://code.google.com/p/go

[gobot, 2014-06-12 19:05:52]

Replacing golang-dev with golang-codereviews.

To the author of this CL: 

If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail'
instead.

If you did not name golang-dev explicitly and it was still added to the CL,
it means your working copy of the repo has a stale codereview.cfg
(or lib/codereview/codereview.cfg).
Please run 'hg sync' to update your client to the most recent codereview.cfg.
If the most recent codereview.cfg has defaultcc set to golang-dev instead of
golang-codereviews, please send a CL correcting it.

Thanks very much.

[brainman, 2014-06-13 00:43:50]

Looks reasonable. Please, see my comments. Thank you.

Alex

https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go
File src/pkg/os/os_test.go (right):

https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go#new...
src/pkg/os/os_test.go:1298: defer Exit(0)
Please, don't use defer here. It makes me suspicious for no good reason.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go#new...
src/pkg/os/os_test.go:1304: cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"}
You want to *add* new environment variable to the current environment variables
set. Not to replace the set with 1 variable. Otherwise this might fail on
windows.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go#new...
src/pkg/os/os_test.go:1307: output, err := cmd.Output()
s/Output/CombinedOutput/

When things fail, the error message is usually in stderr, not stdout.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go#new...
src/pkg/os/os_test.go:1309: t.Fatalf("Failed to spawn child process: %v", err)
Please, display contents of "output" variable here, as well as err. Maybe use %q
to see all line ends and tabs properly.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:908: func GetProcessEntry(pid int)
(*ProcessEntry32, error) {
Why is this function exported from syscall?

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:920: 
Remove empty line. Just like the other code here.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:925: 
Remove empty line.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:938: 
Remove empty line.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/ztypes_wi...
File src/pkg/syscall/ztypes_windows.go (right):

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/ztypes_wi...
src/pkg/syscall/ztypes_windows.go:180: CS_SNAPHEAPLIST = 0x01
s/CS_SNAPHEAPLIST/TH32CS_SNAPHEAPLIST/

Lets not invent new names here. Lets keep names as is per MS documentation.

Same all other variables.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/ztypes_wi...
src/pkg/syscall/ztypes_windows.go:478: UsageCount      uint32
s/UsageCount/Usage/

Keep it with original name.

https://codereview.appspot.com/102320044/diff/40001/src/pkg/syscall/ztypes_wi...
src/pkg/syscall/ztypes_windows.go:482: ThreadCount     uint32
s/ThreadCount/Threads/

[shreveal, 2014-06-13 06:32:13]

https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go
File src/pkg/os/os_test.go (right):

https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go#new...
src/pkg/os/os_test.go:1304: cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"}
This was essentially copied from helperCommand in os/exec/exec_test.go which
sets a single environment variable and does not replace. Should that be modified
as well? I have run these tests on Windows and they pass.


On 2014/06/13 00:43:49, brainman wrote:
> You want to *add* new environment variable to the current environment
variables
> set. Not to replace the set with 1 variable. Otherwise this might fail on
> windows.

[inconshreveable, 2014-06-13 06:34:52]

On 2014/06/13 06:32:13, shreveal wrote:
> https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go
> File src/pkg/os/os_test.go (right):
> 
>
https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go#new...
> src/pkg/os/os_test.go:1304: cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"}
> This was essentially copied from helperCommand in os/exec/exec_test.go which
> sets a single environment variable and does not replace. Should that be
modified
> as well? I have run these tests on Windows and they pass.
> 
> 
> On 2014/06/13 00:43:49, brainman wrote:
> > You want to *add* new environment variable to the current environment
> variables
> > set. Not to replace the set with 1 variable. Otherwise this might fail on
> > windows.

does not *append*, rather

[inconshreveable, 2014-06-13 06:37:44]

On 2014/06/13 06:34:52, inconshreveable wrote:
> On 2014/06/13 06:32:13, shreveal wrote:
> > https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go
> > File src/pkg/os/os_test.go (right):
> > 
> >
>
https://codereview.appspot.com/102320044/diff/40001/src/pkg/os/os_test.go#new...
> > src/pkg/os/os_test.go:1304: cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"}
> > This was essentially copied from helperCommand in os/exec/exec_test.go which
> > sets a single environment variable and does not replace. Should that be
> modified
> > as well? I have run these tests on Windows and they pass.
> > 
> > 
> > On 2014/06/13 00:43:49, brainman wrote:
> > > You want to *add* new environment variable to the current environment
> > variables
> > > set. Not to replace the set with 1 variable. Otherwise this might fail on
> > > windows.
> 
> does not *append*, rather

The rest of the issues should be addressed now, PTAL. I also modified the test
to skip on NaCl like the os/exec tests do.

[brainman, 2014-06-13 07:07:42]

On 2014/06/13 06:32:13, shreveal wrote:
> This was essentially copied from helperCommand in os/exec/exec_test.go which
> sets a single environment variable and does not replace. Should that be
modified
> as well? ...

They should be, but not in this CL. Please modify your test, so it does the
right thing here.

Also, please make sure you complete contributor license agreement as described
in http://golang.org/doc/contribute.html#copyright.

Thank you.

Alex

[shreveal, 2014-06-13 18:13:45]

On 2014/06/13 07:07:42, brainman wrote:
> On 2014/06/13 06:32:13, shreveal wrote:
> > This was essentially copied from helperCommand in os/exec/exec_test.go which
> > sets a single environment variable and does not replace. Should that be
> modified
> > as well? ...
> 
> They should be, but not in this CL. Please modify your test, so it does the
> right thing here.
> 
> Also, please make sure you complete contributor license agreement as described
> in http://golang.org/doc/contribute.html#copyright.
> 
> Thank you.
> 
> Alex

Agreement signed + test updated

Thanks for your help!

[brainman, 2014-06-14 02:30:18]

LGTM

Can someone with access to contributor license agreement submissions change
CONTRIBUTORS file? Thank you.

Alex

[iant, 2014-06-14 04:09:40]

On 2014/06/14 02:30:18, brainman wrote:
>
> Can someone with access to contributor license agreement submissions change
> CONTRIBUTORS file? Thank you.

Done.

Ian

[brainman, 2014-06-14 05:27:20]

On 2014/06/14 04:09:40, iant wrote:
> Done.

Thank you.

Alex

[brainman, 2014-06-14 05:51:19]

*** Submitted as https://code.google.com/p/go/source/detail?r=927f0508bc93 ***

syscall: implement syscall.Getppid() on Windows

Also added a test to verify os.Getppid() works across all platforms

LGTM=alex.brainman
R=golang-codereviews, alex.brainman, shreveal, iant
CC=golang-codereviews
https://codereview.appspot.com/102320044

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

[gobot, 2014-06-14 06:01:18]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/adebbf1b2bceca755ef9fe2122462e7e0bf834ff

[brainman, 2014-06-14 06:41:36]

On 2014/06/14 06:01:18, gobot wrote:
> This CL appears to have broken the plan9-386-cnielsen builder.
> See http://build.golang.org/log/adebbf1b2bceca755ef9fe2122462e7e0bf834ff

Real one. I don't know how to fix it, so I will disable test to get builder
going again: https://codereview.appspot.com/105140047/.

Alex

[0intro, 2014-06-14 07:41:15]

> > This CL appears to have broken the plan9-386-cnielsen builder.
> > See
> http://build.golang.org/log/adebbf1b2bceca755ef9fe2122462e7e0bf834ff
> 
> Real one. I don't know how to fix it, so I will disable test to get
> builder going again: https://codereview.appspot.com/105140047/.

Yes, it seems Getppid() always return 0 on Plan 9 in this test.
This is rather surprising, since writing a simple program calling
os.Getppid works as expected.

term% GO_WANT_HELPER_PROCESS=1 go test -run TestGetppid
0
ok  	os	0.034s

http://play.golang.org/p/npUUya7FmI

term% go run getppid.go
7137

I will take a look later.

-- 
David du Colombier

[ality, 2014-06-14 12:43:45]

David du Colombier <0intro@gmail.com> once said:
> Yes, it seems Getppid() always return 0 on Plan 9 in this test.
> This is rather surprising, since writing a simple program calling
> os.Getppid works as expected.
>
> [...]
> 
> I will take a look later.

See my comments on issue 8206.

  Anthony

[wandakkelly_gmail.com, 2014-06-14 18:05:29]

https://groups.google.com/forum/#!topic/granite-bathroom/ia9NqQzCMFI

เมื่อ วันศุกร์ที่ 13 มิถุนายน ค.ศ. 2014, 2 นาฬิกา 5 นาที 53 วินาที UTC+7, 
go...@golang.org เขียนว่า:
>
> Replacing golang-dev with golang-codereviews. 
>
> To the author of this CL: 
>
> If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg 
> mail' instead. 
>
> If you did not name golang-dev explicitly and it was still added to the 
> CL, 
> it means your working copy of the repo has a stale codereview.cfg 
> (or lib/codereview/codereview.cfg). 
> Please run 'hg sync' to update your client to the most recent 
> codereview.cfg. 
> If the most recent codereview.cfg has defaultcc set to golang-dev 
> instead of 
> golang-codereviews, please send a CL correcting it. 
>
> Thanks very much. 
>
>
> https://codereview.appspot.com/102320044/ 
>

[wandakkelly_gmail.com, 2014-06-15 04:12:15]

https://groups.google.com/forum/#!forum/granite-bathroom

เมื่อ วันศุกร์ที่ 13 มิถุนายน ค.ศ. 2014, 2 นาฬิกา 5 นาที 53 วินาที UTC+7, 
go...@golang.org เขียนว่า:
>
> Replacing golang-dev with golang-codereviews. 
>
> To the author of this CL: 
>
> If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg 
> mail' instead. 
>
> If you did not name golang-dev explicitly and it was still added to the 
> CL, 
> it means your working copy of the repo has a stale codereview.cfg 
> (or lib/codereview/codereview.cfg). 
> Please run 'hg sync' to update your client to the most recent 
> codereview.cfg. 
> If the most recent codereview.cfg has defaultcc set to golang-dev 
> instead of 
> golang-codereviews, please send a CL correcting it. 
>
> Thanks very much. 
>
>
> https://codereview.appspot.com/102320044/ 
>