code review 180380043: cmd/pprof/internal/commands: add command to open browse...

cmd/pprof/internal/commands: add command to open browser on windows

While we're at there, also add a message to prompt the user to install
Graphviz if "dot" command is not found.

Fixes issue 9178.

[minux, 2014-11-27 22:16:49]

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

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

[minux, 2014-11-27 22:19:44]

I propose to include this into 1.4.

[adg, 2014-11-27 22:28:32]

LGTM

On Fri Nov 28 2014 at 9:25:26 AM minux <minux@golang.org> wrote:

> I propose to include this into 1.4.
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[bradfitz, 2014-11-27 22:31:13]

Can "cmd /c start" only be on Windows? And open only be on Darwin? Plus the
cmd one has spaces in it... that's kinda weird.
On Nov 27, 2014 2:16 PM, <minux@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/pprof/internal/commands: add command to open browser on windows
>
> Fixes issue 9178.
>
> Please review this at https://codereview.appspot.com/180380043/
>
> Affected files (+2, -1 lines):
>   M src/cmd/pprof/internal/commands/commands.go
>
>
> Index: src/cmd/pprof/internal/commands/commands.go
> ===================================================================
> --- a/src/cmd/pprof/internal/commands/commands.go
> +++ b/src/cmd/pprof/internal/commands/commands.go
> @@ -79,7 +79,7 @@
>  }
>
>  // List of web browsers to attempt for web visualization
> -var browsers = []string{"chrome", "google-chrome", "firefox",
> "/usr/bin/open"}
> +var browsers = []string{"chrome", "google-chrome", "firefox",
> "/usr/bin/open", "cmd /c start"}
>
>  // NewCompleter creates an autocompletion function for a set of commands.
>  func NewCompleter(cs Commands) Completer {
> @@ -174,6 +174,7 @@
>                 if err = format(input, tempFile, ui); err != nil {
>                         return err
>                 }
> +               tempFile.Close() // on windows, if the file is Open, start
> cannot access it.
>                 // Try visualizers until one is successful
>                 for _, v := range visualizers {
>                         // Separate command and arguments for exec.Command.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[adg, 2014-11-27 22:36:25]

invokeVisualizer splits the command into arguments with strings.Split and
picks the first one that works.

On Fri Nov 28 2014 at 9:31:16 AM Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> Can "cmd /c start" only be on Windows? And open only be on Darwin? Plus
> the cmd one has spaces in it... that's kinda weird.
> On Nov 27, 2014 2:16 PM, <minux@golang.org> wrote:
>
>> Reviewers: rsc,
>>
>> Message:
>> Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> cmd/pprof/internal/commands: add command to open browser on windows
>>
>> Fixes issue 9178.
>>
>> Please review this at https://codereview.appspot.com/180380043/
>>
>> Affected files (+2, -1 lines):
>>   M src/cmd/pprof/internal/commands/commands.go
>>
>>
>> Index: src/cmd/pprof/internal/commands/commands.go
>> ===================================================================
>> --- a/src/cmd/pprof/internal/commands/commands.go
>> +++ b/src/cmd/pprof/internal/commands/commands.go
>> @@ -79,7 +79,7 @@
>>  }
>>
>>  // List of web browsers to attempt for web visualization
>> -var browsers = []string{"chrome", "google-chrome", "firefox",
>> "/usr/bin/open"}
>> +var browsers = []string{"chrome", "google-chrome", "firefox",
>> "/usr/bin/open", "cmd /c start"}
>>
>>  // NewCompleter creates an autocompletion function for a set of commands.
>>  func NewCompleter(cs Commands) Completer {
>> @@ -174,6 +174,7 @@
>>                 if err = format(input, tempFile, ui); err != nil {
>>                         return err
>>                 }
>> +               tempFile.Close() // on windows, if the file is Open,
>> start cannot access it.
>>                 // Try visualizers until one is successful
>>                 for _, v := range visualizers {
>>                         // Separate command and arguments for
>> exec.Command.
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>  --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[brainman, 2014-11-27 22:38:56]

This certainly fixes weblist command (as reported in issue 9178). (You should
say as much in your CL description).

Unfortunately, I don't think weblist command is that interesting - you can just
do list. web command is interesting, but you change does not fix that. I am not
certain your approach will work for web command, until I see web command
implemented on windows. I don't think your change is critical enough to go in
now. I am not even sure it is even valid until we have clear plan for web
command.

Alex

[minux, 2014-11-27 22:41:16]

On 2014/11/27 22:31:13, bradfitz wrote:
> Can "cmd /c start" only be on Windows? And open only be on Darwin?
Yes. we can make browsers a function that return a list of candidates.
I intentionally kept the current CL small to maximize its chance of being
included in 1.4.
Anyway, I will prepare another patch set that make the code do correct thing.

> Plus the cmd one has spaces in it... that's kinda weird.
I'm feeling the same when I found out that invokeVisualizer does
    args := strings.Split(v, " ")

[minux, 2014-11-27 22:48:20]

On 2014/11/27 22:38:56, brainman wrote:
> Unfortunately, I don't think weblist command is that interesting - you can
just
> do list. web command is interesting, but you change does not fix that. I am
not
> certain your approach will work for web command, until I see web command
Have you installed graphviz and put it into your PATH on windows?
(http://www.graphviz.org/pub/graphviz/stable/windows/graphviz-2.38.zip)
I have no problem of using the web command after this change.

[brainman, 2014-11-27 23:02:40]

On 2014/11/27 22:48:20, minux wrote:
> Have you installed graphviz and put it into your PATH on windows?

I didn't - cmd/pprof didn't suggest anything like that. I do now. And web
command works now. Thank you. So LGTM.

But I think we can do better, then expect users to install external command and
add it into their PATH. I am not an expert here, but how hard can it be to
generate the file to be viewed?

Alex

[minux, 2014-11-27 23:09:29]

On 2014/11/27 23:02:40, brainman wrote:
> I didn't - cmd/pprof didn't suggest anything like that.
This is problem that we should fix (it should al least given an error that dot
command
is not found). I guess we need more than one CLs to make pprof more user
friendly.

> But I think we can do better, then expect users to install external command
and
> add it into their PATH. I am not an expert here, but how hard can it be to
> generate the file to be viewed?
we need to duplicate the entire dot command provided by graphviz in Go.
certainly doable, but will need a lot work.

[bradfitz, 2014-11-27 23:11:01]

Hard. There's a conference on the theory like every other year because not
enough progress is made per year to justify a yearly conference.
 On Nov 27, 2014 3:02 PM, <alex.brainman@gmail.com> wrote:

> On 2014/11/27 22:48:20, minux wrote:
>
>> Have you installed graphviz and put it into your PATH on windows?
>>
>
> I didn't - cmd/pprof didn't suggest anything like that. I do now. And
> web command works now. Thank you. So LGTM.
>
> But I think we can do better, then expect users to install external
> command and add it into their PATH. I am not an expert here, but how
> hard can it be to generate the file to be viewed?
>
> Alex
>
> https://codereview.appspot.com/180380043/
>

[minux, 2014-11-27 23:31:25]

OK, PTAL.

Patch set 4 is the minimal one, and patch set 5 is more ideal.
Please state which patch set are you reviewing, thanks.

[cookieo9, 2014-11-30 03:56:41]

On 2014/11/27 23:31:25, minux wrote:
> OK, PTAL.
> 
> Patch set 4 is the minimal one, and patch set 5 is more ideal.
> Please state which patch set are you reviewing, thanks.

#5 works for me. Out of curiosity, is there a way to try out patch #4 via the
codereview plugin, or do I have to apply it manually?

Also, if you did the OS-specific stuff in an init method, then the patch would
be modifying fewer existing lines, as the existing code could still use the
browsers global variable. 

Otherwise #5 does the job. LGTM.

[minux, 2014-12-01 03:20:49]

Thanks for confirmation.

On Sat, Nov 29, 2014 at 10:56 PM, <cookieo9@gmail.com> wrote:

> #5 works for me. Out of curiosity, is there a way to try out patch #4
> via the codereview plugin, or do I have to apply it manually?
>
To the best of my knowledge, you have to apply it manually. Although
there is simple way that involves just copying the raw patch URL of
that patch set and paste to "hg import" command, like this:
hg import --no-commit
https://codereview.appspot.com/download/issue180380043_60001.diff

(It could used whether you have the codereview plugin enabled or not.)

[minux, 2014-12-01 19:35:49]

Ping for 1.4.

[minux, 2014-12-02 23:41:42]

PTAL.

Also added a hint to install Graphviz if dot is not found in PATH.

[smyrman, 2014-12-03 22:40:43]

On 2014/12/02 23:41:42, minux wrote:
> PTAL.
> 
> Also added a hint to install Graphviz if dot is not found in PATH.

While changing this to have OS specific values -- would it make sense to also
prefer the OS default browser? E.g. prepend the OS specific open commands
instead of appending them? Please ignore this message if you feel that now is
not the time :-)

[rsc, 2014-12-04 16:23:37]

LGTM

[rsc, 2014-12-04 16:24:31]

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

cmd/pprof/internal/commands: add command to open browser on windows

While we're at there, also add a message to prompt the user to install
Graphviz if "dot" command is not found.

Fixes issue 9178.

LGTM=adg, alex.brainman, cookieo9, rsc
R=rsc, adg, bradfitz, alex.brainman, cookieo9, smyrman
CC=golang-codereviews
https://codereview.appspot.com/180380043

Committer: Russ Cox <rsc@golang.org>