code review 7094049: cmd/go: add helpful error message when vcs is not found.

cmd/go: add helpful error message when vcs is not found.
Fixes issue 4652.

[stratus, 2013-01-12 06:18:38]

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

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

[minux1, 2013-01-12 07:55:42]

https://codereview.appspot.com/7094049/diff/4002/src/cmd/go/vcs.go
File src/cmd/go/vcs.go (right):

https://codereview.appspot.com/7094049/diff/4002/src/cmd/go/vcs.go#newcode183
src/cmd/go/vcs.go:183: _, err := os.Stat(v.cmd)
you need to use os/exec.LookPath for this task.

https://codereview.appspot.com/7094049/diff/4002/src/cmd/go/vcs.go#newcode185
src/cmd/go/vcs.go:185: fmt.Fprintf(os.Stderr, "Failed to stat %s. Make sure %s
is installed.\n", v.cmd, v.name)
it's better to say this:
failed to find command %s, please make sure %s is installed.

i.e. don't mention stat, as it might not be known to all.

[bradfitz, 2013-01-12 07:55:52]

I don't think this is sufficient.

The user who was confused didn't know that hg meant "Mercurial", nor that
that meant http://mercurial.selenic.com/wiki/Download.

I think the error message should link to an indirection page we maintain
under https://code.google.com/p/go-wiki/w/list (GoGetTools?) explaining
(linking) where to download git, hg, vcs, svn for each platform.

e.g. "The go tool could not find the program 'hg' (Mercurial).  See
https://code.google.com/p/go-wiki/wiki/GoGetTools"


On Fri, Jan 11, 2013 at 10:18 PM, <gustavorfranco@gmail.com> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/go: add helpful error message when vcs is not found.
> Fixes issue 4652.
>
> Please review this at
https://codereview.appspot.**com/7094049/<https://codereview.appspot.com/7094...
>
> Affected files:
>   M src/cmd/go/vcs.go
>
>
> Index: src/cmd/go/vcs.go
> ==============================**==============================**=======
> --- a/src/cmd/go/vcs.go
> +++ b/src/cmd/go/vcs.go
> @@ -180,6 +180,11 @@
>                 args[i] = expand(m, arg)
>         }
>
> +       _, err := os.Stat(v.cmd)
> +       if err != nil {
> +               fmt.Fprintf(os.Stderr, "Failed to stat %s. Make sure %s is
> installed.\n", v.cmd, v.name)
> +       }
> +
>         cmd := exec.Command(v.cmd, args...)
>         cmd.Dir = dir
>         if buildX {
> @@ -189,7 +194,7 @@
>         var buf bytes.Buffer
>         cmd.Stdout = &buf
>         cmd.Stderr = &buf
> -       err := cmd.Run()
> +       err = cmd.Run()
>         out := buf.Bytes()
>         if err != nil {
>                 if verbose || buildV {
>
>
>

[stratus, 2013-01-12 19:42:19]

Thanks for your prompt review.

I've updated the patchset using exec.LookPath() and the suggested message.
I don't have access to write GoGetTools wiki page. Is this possible to
obtain it?


On Fri, Jan 11, 2013 at 11:55 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> I don't think this is sufficient.
>
> The user who was confused didn't know that hg meant "Mercurial", nor that
> that meant http://mercurial.selenic.com/wiki/Download.
>
> I think the error message should link to an indirection page we maintain
> under https://code.google.com/p/go-wiki/w/list (GoGetTools?) explaining
> (linking) where to download git, hg, vcs, svn for each platform.
>
> e.g. "The go tool could not find the program 'hg' (Mercurial).  See
> https://code.google.com/p/go-wiki/wiki/GoGetTools"
>
>
> On Fri, Jan 11, 2013 at 10:18 PM, <gustavorfranco@gmail.com> wrote:
>
>> Reviewers: bradfitz,
>>
>> Message:
>> Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> cmd/go: add helpful error message when vcs is not found.
>> Fixes issue 4652.
>>
>> Please review this at
https://codereview.appspot.**com/7094049/<https://codereview.appspot.com/7094...
>>
>> Affected files:
>>   M src/cmd/go/vcs.go
>>
>>
>> Index: src/cmd/go/vcs.go
>> ==============================**==============================**=======
>> --- a/src/cmd/go/vcs.go
>> +++ b/src/cmd/go/vcs.go
>> @@ -180,6 +180,11 @@
>>                 args[i] = expand(m, arg)
>>         }
>>
>> +       _, err := os.Stat(v.cmd)
>> +       if err != nil {
>> +               fmt.Fprintf(os.Stderr, "Failed to stat %s. Make sure %s
>> is installed.\n", v.cmd, v.name)
>> +       }
>> +
>>         cmd := exec.Command(v.cmd, args...)
>>         cmd.Dir = dir
>>         if buildX {
>> @@ -189,7 +194,7 @@
>>         var buf bytes.Buffer
>>         cmd.Stdout = &buf
>>         cmd.Stderr = &buf
>> -       err := cmd.Run()
>> +       err = cmd.Run()
>>         out := buf.Bytes()
>>         if err != nil {
>>                 if verbose || buildV {
>>
>>
>>
>


-- 
-- Gustavo "stratus" Franco

[rsc, 2013-01-18 20:23:59]

https://codereview.appspot.com/7094049/diff/4/src/cmd/go/vcs.go
File src/cmd/go/vcs.go (right):

https://codereview.appspot.com/7094049/diff/4/src/cmd/go/vcs.go#newcode185
src/cmd/go/vcs.go:185: fmt.Fprintf(os.Stderr,
fmt.Fprintf(os.Stderr, "go: missing %s command. See
http://golang.org/s/gogetcmd\n", v.name)

[bradfitz, 2013-01-19 16:51:43]

On Fri, Jan 18, 2013 at 12:23 PM, <rsc@golang.org> wrote:

>
>
https://codereview.appspot.**com/7094049/diff/4/src/cmd/go/**vcs.go<https://c...
> File src/cmd/go/vcs.go (right):
>
> https://codereview.appspot.**com/7094049/diff/4/src/cmd/go/**
>
vcs.go#newcode185<https://codereview.appspot.com/7094049/diff/4/src/cmd/go/vcs.go#newcode185>
> src/cmd/go/vcs.go:185: fmt.Fprintf(os.Stderr,
> fmt.Fprintf(os.Stderr, "go: missing %s command. See
> http://golang.org/s/gogetcmd\n**", v.name)
>
>
https://codereview.appspot.**com/7094049/<https://codereview.appspot.com/7094...
>

I've created an initial version of
https://code.google.com/p/go-wiki/wiki/GoGetTools

[minux1, 2013-01-27 11:21:09]

this issue has been sat idle for 8 days,
perhaps we can just submit after applying
Russ' comment?

could someone please add gustavorfranco@gmail.com
to CONTRIBUTORS?

[bradfitz, 2013-01-27 17:28:23]

No CLA has been submitted.

On Sun, Jan 27, 2013 at 3:21 AM, <minux.ma@gmail.com> wrote:

> this issue has been sat idle for 8 days,
> perhaps we can just submit after applying
> Russ' comment?
>
> could someone please add gustavorfranco@gmail.com
> to CONTRIBUTORS?
>
>
https://codereview.appspot.**com/7094049/<https://codereview.appspot.com/7094...
>

[rsc, 2013-01-28 17:56:46]

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

Thanks.

[rsc, 2013-01-28 18:03:10]

Never mind, I see that you've done that now.

[stratus, 2013-01-29 04:44:58]

Hi Russ,

I don't remember completing a CLA. Do I need to do that even as a
googler? If yes, I will tackle this tomorrow.

thanks,

On Mon, Jan 28, 2013 at 10:03 AM, Russ Cox <rsc@golang.org> wrote:
> Never mind, I see that you've done that now.
>
>



-- 
-- Gustavo "stratus" Franco

[bradfitz, 2013-01-29 04:49:13]

No, you don't.

I've already added you to the CONTRIBUTORS file in
https://code.google.com/p/go/source/detail?r=646ee022106


On Mon, Jan 28, 2013 at 8:44 PM, Gustavo Franco
<gustavorfranco@gmail.com>wrote:

> Hi Russ,
>
> I don't remember completing a CLA. Do I need to do that even as a
> googler? If yes, I will tackle this tomorrow.
>
> thanks,
>
> On Mon, Jan 28, 2013 at 10:03 AM, Russ Cox <rsc@golang.org> wrote:
> > Never mind, I see that you've done that now.
> >
> >
>
>
>
> --
> -- Gustavo "stratus" Franco
>

[stratus, 2013-01-29 05:34:07]

Thanks Brad.

I've changed it to fit Russ' comments. PTAL.

On Mon, Jan 28, 2013 at 8:49 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> No, you don't.
>
> I've already added you to the CONTRIBUTORS file in
> https://code.google.com/p/go/source/detail?r=646ee022106
>
>
> On Mon, Jan 28, 2013 at 8:44 PM, Gustavo Franco <gustavorfranco@gmail.com>
> wrote:
>>
>> Hi Russ,
>>
>> I don't remember completing a CLA. Do I need to do that even as a
>> googler? If yes, I will tackle this tomorrow.
>>
>> thanks,
>>
>> On Mon, Jan 28, 2013 at 10:03 AM, Russ Cox <rsc@golang.org> wrote:
>> > Never mind, I see that you've done that now.
>> >
>> >
>>
>>
>>
>> --
>> -- Gustavo "stratus" Franco
>
>



-- 
-- Gustavo "stratus" Franco

[bradfitz, 2013-01-29 05:47:02]

https://codereview.appspot.com/7094049/diff/16002/src/cmd/go/vcs.go
File src/cmd/go/vcs.go (right):

https://codereview.appspot.com/7094049/diff/16002/src/cmd/go/vcs.go#newcode188
src/cmd/go/vcs.go:188: }
do we want a "return nil, err" here?  or an os.Exit(1) ?

proceeding further seems futile.

[stratus, 2013-01-29 06:03:03]

Good point, Brad. I've add a "return nil ,err".

On Mon, Jan 28, 2013 at 9:47 PM,  <bradfitz@golang.org> wrote:
>
> https://codereview.appspot.com/7094049/diff/16002/src/cmd/go/vcs.go
> File src/cmd/go/vcs.go (right):
>
>
https://codereview.appspot.com/7094049/diff/16002/src/cmd/go/vcs.go#newcode188
> src/cmd/go/vcs.go:188: }
> do we want a "return nil, err" here?  or an os.Exit(1) ?
>
> proceeding further seems futile.
>
> https://codereview.appspot.com/7094049/



-- 
-- Gustavo "stratus" Franco

[bradfitz, 2013-01-29 16:19:46]

LGTM

On Mon, Jan 28, 2013 at 10:02 PM, Gustavo Franco
<gustavorfranco@gmail.com>wrote:

> Good point, Brad. I've add a "return nil ,err".
>
> On Mon, Jan 28, 2013 at 9:47 PM,  <bradfitz@golang.org> wrote:
> >
> > https://codereview.appspot.com/7094049/diff/16002/src/cmd/go/vcs.go
> > File src/cmd/go/vcs.go (right):
> >
> >
>
https://codereview.appspot.com/7094049/diff/16002/src/cmd/go/vcs.go#newcode188
> > src/cmd/go/vcs.go:188: }
> > do we want a "return nil, err" here?  or an os.Exit(1) ?
> >
> > proceeding further seems futile.
> >
> > https://codereview.appspot.com/7094049/
>
>
>
> --
> -- Gustavo "stratus" Franco
>

[bradfitz, 2013-01-29 16:20:47]

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

cmd/go: add helpful error message when vcs is not found.
Fixes issue 4652.

R=bradfitz, minux.ma, rsc
CC=golang-dev
https://codereview.appspot.com/7094049

Committer: Brad Fitzpatrick <bradfitz@golang.org>