FAQ
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


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/

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),
},

Search Discussions

  • Minux Ma at Nov 8, 2012 at 2:43 pm
  • Brad Fitzpatrick at Nov 8, 2012 at 3:44 pm
    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, 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),
    },

  • Minux at Nov 8, 2012 at 3:54 pm

    On Thu, Nov 8, 2012 at 11:34 PM, Brad Fitzpatrick 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.
  • Brad Fitzpatrick at Nov 8, 2012 at 4:20 pm
    On Thu, Nov 8, 2012 at 9:54 AM, minux wrote:
    On Thu, Nov 8, 2012 at 11:34 PM, Brad Fitzpatrick 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.
  • Joel Sing at Nov 8, 2012 at 4:26 pm

    On 9 November 2012 02:54, minux wrote:
    On Thu, Nov 8, 2012 at 11:34 PM, Brad Fitzpatrick 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
  • Dave at Nov 8, 2012 at 4:30 pm
    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#newcode158
    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.

    http://codereview.appspot.com/6819113/
  • Bradfitz at Nov 8, 2012 at 4:33 pm
    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#newcode169
    ssh/test/test_unix_test.go:169: User: username,
    username() and pull the above out into a function?

    https://codereview.appspot.com/6819113/
  • Jsing at Nov 11, 2012 at 3:47 pm
    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#newcode158
    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#newcode169
    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.

    https://codereview.appspot.com/6819113/
  • Jsing at Nov 11, 2012 at 3:52 pm
    *** 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


    http://codereview.appspot.com/6819113/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 8, '12 at 2:35p
activeNov 11, '12 at 3:52p
posts10
users4
websitegolang.org

4 users in discussion

Jsing: 4 posts Bradfitz: 3 posts Minux: 2 posts Dave: 1 post

People

Translate

site design / logo © 2022 Grokbase