code review 6819113: go.crypto/ssh: make tests work on OpenBSD

go.crypto/ssh: make tests work on non-cgo platforms.

user.Current() currently requires cgo - if an error is returned
attempt to get the username from the environment.

[jsing, 2012-11-08 14:35:48]

Hello golang-dev@googlegroups.com,

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

[minux1, 2012-11-08 14:43:48]

LGTM

[bradfitz, 2012-11-08 15:34:11]

Kinda gross and won't get updated when openbsd gets cgo.

Can we query for cgo at runtime somehow? Alternatively, there's a cgo build
tag.
 On Nov 8, 2012 6:35 AM, <jsing@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go.**crypto<https://code.google.com/p/go.crypto>
>
>
> Description:
> go.crypto/ssh: make tests work on OpenBSD
>
> OpenBSD does not have a working user.Current() since this currently
> requires CGO support. Attempt to get the username via the USER
> environment variable instead.
>
> Please review this at
http://codereview.appspot.com/**6819113/<http://codereview.appspot.com/6819113/>
>
> Affected files:
>   M ssh/test/test_unix_test.go
>
>
> Index: ssh/test/test_unix_test.go
> ==============================**==============================**=======
> --- a/ssh/test/test_unix_test.go
> +++ b/ssh/test/test_unix_test.go
> @@ -23,6 +23,7 @@
>         "os/exec"
>         "os/user"
>         "path/filepath"
> +       "runtime"
>         "testing"
>         "text/template"
>         "time"
> @@ -151,14 +152,25 @@
>  }
>
>  func clientConfig() *ssh.ClientConfig {
> -       user, err := user.Current()
> -       if err != nil {
> -               panic(err)
> +       var username string
> +       if runtime.GOOS == "openbsd" {
> +               // OpenBSD does not have a working user.Current() since
> this
> +               // currently requires CGO support.
> +               username = os.Getenv("USER")
> +       } else {
> +               user, err := user.Current()
> +               if err != nil {
> +                       panic(err)
> +               }
> +               username = user.Username
> +       }
> +       if username == "" {
> +               panic("Unable to get username")
>         }
>         kc := new(keychain)
>         kc.keys = append(kc.keys, rsakey)
>         config := &ssh.ClientConfig{
> -               User: user.Username,
> +               User: username,
>                 Auth: []ssh.ClientAuth{
>                         ssh.ClientAuthKeyring(kc),
>                 },
>
>
>

[minux1, 2012-11-08 15:54:48]

On Thu, Nov 8, 2012 at 11:34 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Kinda gross and won't get updated when openbsd gets cgo.
>
> Can we query for cgo at runtime somehow? Alternatively, there's a cgo
> build tag.
>
The dependency is much more subtle than that.
however, your concern is also valid.

I'd suggest we first call os/user.Current, and if it is "" or error occurs,
try os.Getenv("USER"), if still "", panic.

[bradfitz, 2012-11-08 16:20:50]

On Thu, Nov 8, 2012 at 9:54 AM, minux <minux.ma@gmail.com> wrote:

>
> On Thu, Nov 8, 2012 at 11:34 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> Kinda gross and won't get updated when openbsd gets cgo.
>>
>> Can we query for cgo at runtime somehow? Alternatively, there's a cgo
>> build tag.
>>
> The dependency is much more subtle than that.
> however, your concern is also valid.
>
> I'd suggest we first call os/user.Current, and if it is "" or error occurs,
> try os.Getenv("USER"), if still "", panic.
>

Yes, that SGTM.

[jsing, 2012-11-08 16:26:39]

On 9 November 2012 02:54, minux <minux.ma@gmail.com> wrote:
>
> On Thu, Nov 8, 2012 at 11:34 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>>
>> Kinda gross and won't get updated when openbsd gets cgo.
>>
>> Can we query for cgo at runtime somehow? Alternatively, there's a cgo
>> build tag.
>
> The dependency is much more subtle than that.
> however, your concern is also valid.

Right. The underlying issue is that user.Current() currently has no
non-cgo implementation.

> I'd suggest we first call os/user.Current, and if it is "" or error occurs,
> try os.Getenv("USER"), if still "", panic.

Works for me, although I think checking for an error is sufficient -
if we got a User with an empty username then that is probably a valid
failure case.

PTAL

[dfc, 2012-11-08 16:30:47]

LGTM modulo comment.

http://codereview.appspot.com/6819113/diff/5002/ssh/test/test_unix_test.go
File ssh/test/test_unix_test.go (right):

http://codereview.appspot.com/6819113/diff/5002/ssh/test/test_unix_test.go#ne...
ssh/test/test_unix_test.go:158: // user.Current() currently requires CGO support
- if it
// user.Current() requires cgo. If an error is returned
// try to get the username from the environment.

[bradfitz, 2012-11-08 16:33:24]

LGTM

https://codereview.appspot.com/6819113/diff/5002/ssh/test/test_unix_test.go
File ssh/test/test_unix_test.go (right):

https://codereview.appspot.com/6819113/diff/5002/ssh/test/test_unix_test.go#n...
ssh/test/test_unix_test.go:169: User: username,
username() and pull the above out into a function?

[jsing, 2012-11-11 15:47:07]

https://codereview.appspot.com/6819113/diff/5002/ssh/test/test_unix_test.go
File ssh/test/test_unix_test.go (right):

https://codereview.appspot.com/6819113/diff/5002/ssh/test/test_unix_test.go#n...
ssh/test/test_unix_test.go:158: // user.Current() currently requires CGO support
- if it
On 2012/11/08 16:30:48, dfc wrote:
> // user.Current() requires cgo. If an error is returned
> // try to get the username from the environment.

I've merged most of your suggestion.

https://codereview.appspot.com/6819113/diff/5002/ssh/test/test_unix_test.go#n...
ssh/test/test_unix_test.go:169: User: username,
On 2012/11/08 16:33:24, bradfitz wrote:
> username() and pull the above out into a function?

Yup, done.

[jsing, 2012-11-11 15:52:31]

*** Submitted as
http://code.google.com/p/go/source/detail?r=cf51e694ba75&repo=crypto ***

go.crypto/ssh: make tests work on non-cgo platforms.

user.Current() currently requires cgo - if an error is returned
attempt to get the username from the environment.

R=golang-dev, minux.ma, bradfitz, dave
CC=golang-dev
http://codereview.appspot.com/6819113