code review 5653055: os,syscall: fix windows build

os,syscall: fix windows build

make syscall.ProcAttr.Files be []uintptr

all.bash passes on Linux.
things seem to compile on GOOS={darwin,windows}

[bradfitz, 2012-02-10 06:30:40]

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

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

[bradfitz, 2012-02-10 06:33:30]

I have to leave for dinner in a few minutes here, so feel free to submit if
you think this is the right solution.

I'm not sure we want to change syscall.ProcAttr.Files from []int to
[]uintptr, though.

If we don't, we need to break src/pkg/os/exec_posix.go (or at least part of
it) into unix-vs-windows files, though.

I could imagine making a helper function syscallFds(fds []uintptr) []int
for unix and []syscall.Handle for windows in two separate pkg os files.

Not sure which is more desirable.


On Fri, Feb 10, 2012 at 5:30 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> os,syscall: fix windows build
>
> make syscall.ProcAttr.Files be []uintptr
>
> all.bash passes on Linux.
> things seem to compile on GOOS={darwin,windows}
>
> Please review this at
http://codereview.appspot.com/**5653055/<http://codereview.appspot.com/5653055/>
>
> Affected files:
>  M src/pkg/os/exec_posix.go
>  M src/pkg/os/file_windows.go
>  M src/pkg/syscall/exec_bsd.go
>  M src/pkg/syscall/exec_linux.go
>  M src/pkg/syscall/exec_unix.go
>  M src/pkg/syscall/exec_windows.**go
>
>
> Index: src/pkg/os/exec_posix.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec_posix.go
> +++ b/src/pkg/os/exec_posix.go
> @@ -38,7 +38,7 @@
>                sysattr.Env = Environ()
>        }
>        for _, f := range attr.Files {
> -               sysattr.Files = append(sysattr.Files, int(f.Fd()))
> +               sysattr.Files = append(sysattr.Files, f.Fd())
>        }
>
>        pid, h, e := syscall.StartProcess(name, argv, sysattr)
> Index: src/pkg/os/file_windows.go
> ==============================**==============================**=======
> --- a/src/pkg/os/file_windows.go
> +++ b/src/pkg/os/file_windows.go
> @@ -70,7 +70,7 @@
>                syscall.CloseOnExec(r)
>        }
>
> -       return NewFile(r, name), nil
> +       return NewFile(uintptr(r), name), nil
>  }
>
>  func openDir(name string) (file *File, err error) {
> @@ -79,7 +79,7 @@
>        if e != nil {
>                return nil, &PathError{"open", name, e}
>        }
> -       f := NewFile(r, name)
> +       f := NewFile(uintptr(r), name)
>        f.dirinfo = d
>        return f, nil
>  }
> @@ -313,7 +313,7 @@
>        syscall.CloseOnExec(p[1])
>        syscall.ForkLock.RUnlock()
>
> -       return NewFile(p[0], "|0"), NewFile(p[1], "|1"), nil
> +       return NewFile(uintptr(p[0]), "|0"), NewFile(uintptr(p[1]), "|1"),
> nil
>  }
>
>  // TempDir returns the default directory to use for temporary files.
> Index: src/pkg/syscall/exec_bsd.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/exec_bsd.go
> +++ b/src/pkg/syscall/exec_bsd.go
> @@ -39,8 +39,10 @@
>                i      int
>        )
>
> -       // guard against side effects of shuffling fds below.
> -       fd := append([]int(nil), attr.Files...)
> +       fd := make([]int, len(attr.Files))
> +       for i, ufd := range attr.Files {
> +               fd[i] = int(ufd)
> +       }
>
>        darwin := runtime.GOOS == "darwin"
>
> Index: src/pkg/syscall/exec_linux.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/exec_linux.**go
> +++ b/src/pkg/syscall/exec_linux.**go
> @@ -40,7 +40,10 @@
>        )
>
>        // guard against side effects of shuffling fds below.
> -       fd := append([]int(nil), attr.Files...)
> +       fd := make([]int, len(attr.Files))
> +       for i, ufd := range attr.Files {
> +               fd[i] = int(ufd)
> +       }
>
>        // About to call fork.
>        // No more allocation or calls of non-assembly functions.
> Index: src/pkg/syscall/exec_unix.go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/exec_unix.go
> +++ b/src/pkg/syscall/exec_unix.go
> @@ -101,9 +101,9 @@
>  // ProcAttr holds attributes that will be applied to a new process started
>  // by StartProcess.
>  type ProcAttr struct {
> -       Dir   string   // Current working directory.
> -       Env   []string // Environment.
> -       Files []int    // File descriptors.
> +       Dir   string    // Current working directory.
> +       Env   []string  // Environment.
> +       Files []uintptr // File descriptors.
>        Sys   *SysProcAttr
>  }
>
> Index: src/pkg/syscall/exec_windows.**go
> ==============================**==============================**=======
> --- a/src/pkg/syscall/exec_**windows.go
> +++ b/src/pkg/syscall/exec_**windows.go
> @@ -220,7 +220,7 @@
>  type ProcAttr struct {
>        Dir   string
>        Env   []string
> -       Files []Handle
> +       Files []uintptr
>        Sys   *SysProcAttr
>  }
>
>
>
>

[mattn, 2012-02-10 06:43:16]

LGTM

On 2012/02/10 06:33:30, bradfitz wrote:
> I have to leave for dinner in a few minutes here, so feel free to submit if
> you think this is the right solution.
> 
> I'm not sure we want to change syscall.ProcAttr.Files from []int to
> []uintptr, though.
> 
> If we don't, we need to break src/pkg/os/exec_posix.go (or at least part of
> it) into unix-vs-windows files, though.
> 
> I could imagine making a helper function syscallFds(fds []uintptr) []int
> for unix and []syscall.Handle for windows in two separate pkg os files.
> 
> Not sure which is more desirable.
> 
> 
> On Fri, Feb 10, 2012 at 5:30 PM, <mailto:bradfitz@golang.org> wrote:
> 
> > Reviewers: http://golang-dev_googlegroups.com,
> >
> > Message:
> > Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://go.googlecode.com/hg/
> >
> >
> > Description:
> > os,syscall: fix windows build
> >
> > make syscall.ProcAttr.Files be []uintptr
> >
> > all.bash passes on Linux.
> > things seem to compile on GOOS={darwin,windows}
> >
> > Please review this at
>
http://codereview.appspot.com/**5653055/%3Chttp://codereview.appspot.com/5653...>
> >
> > Affected files:
> >  M src/pkg/os/exec_posix.go
> >  M src/pkg/os/file_windows.go
> >  M src/pkg/syscall/exec_bsd.go
> >  M src/pkg/syscall/exec_linux.go
> >  M src/pkg/syscall/exec_unix.go
> >  M src/pkg/syscall/exec_windows.**go
> >
> >
> > Index: src/pkg/os/exec_posix.go
> > ==============================**==============================**=======
> > --- a/src/pkg/os/exec_posix.go
> > +++ b/src/pkg/os/exec_posix.go
> > @@ -38,7 +38,7 @@
> >                sysattr.Env = Environ()
> >        }
> >        for _, f := range attr.Files {
> > -               sysattr.Files = append(sysattr.Files, int(f.Fd()))
> > +               sysattr.Files = append(sysattr.Files, f.Fd())
> >        }
> >
> >        pid, h, e := syscall.StartProcess(name, argv, sysattr)
> > Index: src/pkg/os/file_windows.go
> > ==============================**==============================**=======
> > --- a/src/pkg/os/file_windows.go
> > +++ b/src/pkg/os/file_windows.go
> > @@ -70,7 +70,7 @@
> >                syscall.CloseOnExec(r)
> >        }
> >
> > -       return NewFile(r, name), nil
> > +       return NewFile(uintptr(r), name), nil
> >  }
> >
> >  func openDir(name string) (file *File, err error) {
> > @@ -79,7 +79,7 @@
> >        if e != nil {
> >                return nil, &PathError{"open", name, e}
> >        }
> > -       f := NewFile(r, name)
> > +       f := NewFile(uintptr(r), name)
> >        f.dirinfo = d
> >        return f, nil
> >  }
> > @@ -313,7 +313,7 @@
> >        syscall.CloseOnExec(p[1])
> >        syscall.ForkLock.RUnlock()
> >
> > -       return NewFile(p[0], "|0"), NewFile(p[1], "|1"), nil
> > +       return NewFile(uintptr(p[0]), "|0"), NewFile(uintptr(p[1]), "|1"),
> > nil
> >  }
> >
> >  // TempDir returns the default directory to use for temporary files.
> > Index: src/pkg/syscall/exec_bsd.go
> > ==============================**==============================**=======
> > --- a/src/pkg/syscall/exec_bsd.go
> > +++ b/src/pkg/syscall/exec_bsd.go
> > @@ -39,8 +39,10 @@
> >                i      int
> >        )
> >
> > -       // guard against side effects of shuffling fds below.
> > -       fd := append([]int(nil), attr.Files...)
> > +       fd := make([]int, len(attr.Files))
> > +       for i, ufd := range attr.Files {
> > +               fd[i] = int(ufd)
> > +       }
> >
> >        darwin := runtime.GOOS == "darwin"
> >
> > Index: src/pkg/syscall/exec_linux.go
> > ==============================**==============================**=======
> > --- a/src/pkg/syscall/exec_linux.**go
> > +++ b/src/pkg/syscall/exec_linux.**go
> > @@ -40,7 +40,10 @@
> >        )
> >
> >        // guard against side effects of shuffling fds below.
> > -       fd := append([]int(nil), attr.Files...)
> > +       fd := make([]int, len(attr.Files))
> > +       for i, ufd := range attr.Files {
> > +               fd[i] = int(ufd)
> > +       }
> >
> >        // About to call fork.
> >        // No more allocation or calls of non-assembly functions.
> > Index: src/pkg/syscall/exec_unix.go
> > ==============================**==============================**=======
> > --- a/src/pkg/syscall/exec_unix.go
> > +++ b/src/pkg/syscall/exec_unix.go
> > @@ -101,9 +101,9 @@
> >  // ProcAttr holds attributes that will be applied to a new process started
> >  // by StartProcess.
> >  type ProcAttr struct {
> > -       Dir   string   // Current working directory.
> > -       Env   []string // Environment.
> > -       Files []int    // File descriptors.
> > +       Dir   string    // Current working directory.
> > +       Env   []string  // Environment.
> > +       Files []uintptr // File descriptors.
> >        Sys   *SysProcAttr
> >  }
> >
> > Index: src/pkg/syscall/exec_windows.**go
> > ==============================**==============================**=======
> > --- a/src/pkg/syscall/exec_**windows.go
> > +++ b/src/pkg/syscall/exec_**windows.go
> > @@ -220,7 +220,7 @@
> >  type ProcAttr struct {
> >        Dir   string
> >        Env   []string
> > -       Files []Handle
> > +       Files []uintptr
> >        Sys   *SysProcAttr
> >  }
> >
> >
> >
> >

[brainman, 2012-02-10 06:51:25]

It still fails to build. Now in net:
...
# net
pkg\net\sendfile_windows.go:59: cannot use f.Fd() (type uintptr) as type
syscall.Handle in assignment
...

Alex

[bradfitz, 2012-02-10 06:57:00]

Hello golang-dev@googlegroups.com, mattn.jp@gmail.com, alex.brainman@gmail.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-02-10 06:58:31]

Fixed sendfile_windows.go now.

Now it builds with:

$ GOOS=windows go build os syscall net
$ GOOS=darwin go build os syscall
(can't test net, because of cgo)


On Fri, Feb 10, 2012 at 5:57 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, mattn.jp@gmail.com,
> alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5653055/<http://codereview.appspot.com/5653055/>
>

[mattn, 2012-02-10 07:54:04]

please include change of net/sendfile_windows.go

diff -r caf7dca9d711 src/pkg/net/sendfile_windows.go
--- a/src/pkg/net/sendfile_windows.go	Fri Feb 10 18:49:19 2012 +1100
+++ b/src/pkg/net/sendfile_windows.go	Fri Feb 10 16:53:15 2012 +0900
@@ -56,7 +56,7 @@
 	var o sendfileOp
 	o.Init(c, 'w')
 	o.n = uint32(n)
-	o.src = f.Fd()
+	o.src = syscall.Handle(f.Fd())
 	done, err := iosrv.ExecIO(&o, 0)
 	if err != nil {
 		return 0, err, false


On 2012/02/10 06:58:31, bradfitz wrote:
> Fixed sendfile_windows.go now.
> 
> Now it builds with:
> 
> $ GOOS=windows go build os syscall net
> $ GOOS=darwin go build os syscall
> (can't test net, because of cgo)
> 
> 
> On Fri, Feb 10, 2012 at 5:57 PM, <mailto:bradfitz@golang.org> wrote:
> 
> > Hello mailto:golang-dev@googlegroups.com, mailto:mattn.jp@gmail.com,
> > mailto:alex.brainman@gmail.com (cc: mailto:golang-dev@googlegroups.com),
> >
> > Please take another look.
> >
> >
> >
>
http://codereview.appspot.com/**5653055/%3Chttp://codereview.appspot.com/5653...>
> >

[brainman, 2012-02-10 08:29:18]

On 2012/02/10 06:58:31, bradfitz wrote:
> Fixed sendfile_windows.go now.
> 
> Now it builds with:
> 
> $ GOOS=windows go build os syscall net
> $ GOOS=darwin go build os syscall
> (can't test net, because of cgo)
> 

All packages build now, but test in os/exec still does not compile:

# os/exec
pkg\os\exec\exec_test.go:157: cannot use fd (type int) as type syscall.Handle in
function argument
FAIL    os/exec [build failed]

GOOS=windows go test -c os/exec

will compile it for you.

Alex

[rsc1, 2012-02-10 21:06:27]

LGTM

Seems fine.  I'm not too worried about syscall.

[bradfitz, 2012-02-10 21:47:28]

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

os,syscall: fix windows build

make syscall.ProcAttr.Files be []uintptr

all.bash passes on Linux.
things seem to compile on GOOS={darwin,windows}

R=golang-dev, mattn.jp, alex.brainman, rsc
CC=golang-dev
http://codereview.appspot.com/5653055