|
|
|
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.
Patch Set 1 #Patch Set 2 : diff -r ffe33f1f1f17bf7d9bd14d8a8931dc905a75d011 https://code.google.com/p/go #Patch Set 3 : diff -r ffe33f1f1f17bf7d9bd14d8a8931dc905a75d011 https://code.google.com/p/go #Patch Set 4 : diff -r ffe33f1f1f17bf7d9bd14d8a8931dc905a75d011 https://code.google.com/p/go #Patch Set 5 : diff -r ffe33f1f1f17bf7d9bd14d8a8931dc905a75d011 https://code.google.com/p/go #Patch Set 6 : diff -r e8e6ada28fb659fe4256739b4fb0a7357cec9dc8 https://code.google.com/p/go #Total messages: 19
|
minux
Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
|
11 years, 1 month ago (2014年11月27日 22:16:49 UTC) #1 |
Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
I propose to include this into 1.4.
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. >
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. >
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. >
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
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, " ")
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.
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
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.
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/ >
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.
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.
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.)
Ping for 1.4.
PTAL. Also added a hint to install Graphviz if dot is not found in PATH.
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 :-)
LGTM
*** 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>