Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/pprof: add readline support #14041

Closed
valyala opened this issue Jan 20, 2016 · 6 comments
Closed

cmd/pprof: add readline support #14041

valyala opened this issue Jan 20, 2016 · 6 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@valyala
Copy link
Contributor

valyala commented Jan 20, 2016

Currently go tool pprof in interactive mode doesn't support readline features such as command history, ctrl-r, tab-completion, etc. It would be great to add such functionality.

Basic integration is quite easy - see the gist containing PoC readline patch for go tool pprof. It is based on pure go implementation of readline.

@bradfitz bradfitz changed the title Feature request: add readline support for go tool pprof interactive mode cmd/pprof: add readline support Jan 20, 2016
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jan 20, 2016
@ALTree
Copy link
Member

ALTree commented Jan 22, 2018

upstream issue: google/pprof/issues/57

Unfortunately it seems that the pprof maintainers are reluctant to add an external dependency to the tool.

@hyangah
Copy link
Contributor

hyangah commented Apr 11, 2018

The upstream pprof maintainer @aalexand proposed to utilize the golang.org/x/crypto/ssh/terminal package for readline functionality.
Go repo already vendored some packages from x/crypto (https://github.com/golang/go/tree/master/src/vendor/golang_org/x/crypto).
Is it okay to add the ssh/terminal package there?

@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 11, 2018
@gopherbot
Copy link

Change https://golang.org/cl/112436 mentions this issue: cmd/pprof: provides readline feature

@hyangah
Copy link
Contributor

hyangah commented May 9, 2018

cl/112436 implements the feature requested here. I was ok'd about vendoring x/crypto/ssh/terminal but found it depends on some packages in x/sys as well. They bring in a lot more files than I hoped and I am currently fighting for all kinds of build/vet errors discovered while vendoring. Already 1.11 dev tree is closed and I am afraid it's too late.

@agnivade agnivade added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 10, 2018
@agnivade agnivade modified the milestones: Unplanned, Go1.12 May 10, 2018
hyangah added a commit to hyangah/pprof that referenced this issue May 21, 2018
The current implementation placed the dependency on
github.com/chzyer/readline in pprof.go instead of placing
it in the default ui just because the go repository that
vendors pprof may not want the dependency.

I evaluated another option (golang.org/x/crypto/ssh/terminal)
for the Go repository and there exist subtle differences
in the behavior. Inconsistency in UI is undesirable so golang
decided to vendor the chzyer readline as well for now.

Update golang/go#14041
gopherbot pushed a commit that referenced this issue May 23, 2018
The upstream pprof implements the readline feature using
the github.com/chzyer/readline package in its pprof.go main.

It would be ideal to use the same readline support package as
the upstream for better user experience and code maintenance.
However, bringing in third-party packages requires more work
than I envisioned (e.g. clean up the vendored code to meet the
expected standard - iow don't break builders).

As a result, this change implements the similar feature
for the pprof command included in the go distribution
(cmd/pprof/pprof.go) using golang.org/x/crypto/ssh/terminal
for now.

Auto-completion is not yet supported (same in the upstream).

The feature is enabled only in linux, windows, darwin, and
only when terminal support is available.

This change brings in new vendored packages,
golang.org/x/crypto/ssh/terminal and
golang.org/x/sys/{unix,windows}.

For #14041

Change-Id: If4a790796acf2ab20f7e81268b9d9354c5a5cd2b
Reviewed-on: https://go-review.googlesource.com/112436
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 10, 2018
@tydavis
Copy link

tydavis commented Nov 21, 2019

Since this has been merged (https://go-review.googlesource.com/c/go/+/112436/) can we close the ticket?

@hyangah
Copy link
Contributor

hyangah commented Nov 22, 2019

I hoped we could find a way to use the same readline library as the upstream pprof uses before closing. But I don't think that's critical at this moment. Closing.

@hyangah hyangah closed this as completed Nov 22, 2019
@golang golang locked and limited conversation to collaborators Nov 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants