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: update the vendored tool from upstream #21047

Closed
ALTree opened this issue Jul 17, 2017 · 10 comments
Closed

cmd/pprof: update the vendored tool from upstream #21047

ALTree opened this issue Jul 17, 2017 · 10 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge
Milestone

Comments

@ALTree
Copy link
Member

ALTree commented Jul 17, 2017

Opening this to document a recent (failed) effort to update pprof from upstream.

The tool was successfully updated from upstream on March 1 (CL 37652). A failed attempt to a final update for go1.9 was made on June 20 (CL 46155). The update broke several builders and was immediately reverted.

The failing builders:

To sum it up: all the failures are on the TestHttpsInsecure test. Darwin and plan9 builders are failing because the test somehow generates bad/unexpected profiles(?). A linux-arm5 builder and both the android builders are failing with errors related to bad filesystem permission (I suspect the test makes assumptions about write permissions on certain folders that do not hold on some of the builders).

@ALTree ALTree added this to the Go1.10 milestone Jul 17, 2017
@ALTree ALTree added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jul 17, 2017
@aalexand
Copy link
Contributor

google/pprof#169 should fix the file system related issue on Android.

TestHttpsInsecure test is special in that it is the only test that collects the data by starting a Go server and a spinning goroutine and profiling the server. Hence these new issues found.

Is it possible to add building and testing of google/pprof at tip to the Go build matrix, "Sub-repositories at tip" section? This would continuously ensure that pprof is Go vendoring ready.

@ALTree
Copy link
Member Author

ALTree commented Jul 21, 2017

Is it possible to add building and testing of google/pprof at tip to the Go build matrix, "Sub-repositories at tip" section? This would continuously ensure that pprof is Go vendoring ready.

@bradfitz is it possible/do we want this?

@bradfitz
Copy link
Contributor

We want that, yes. And everything's possible with a bit of time. We're short on time. But if somebody wants to work on #14594, that'd be great.

@aalexand
Copy link
Contributor

@ALTree google/pprof#146 was fixed, can you try updating pprof in Go vendoring area again?

@ALTree
Copy link
Member Author

ALTree commented Jul 26, 2017

@aalexand nice! I could send a change but it won't be merged now because we're deep deep into the code freeze. I'll send a change as soon as the tree re-open for go1.10 in August.

@aalexand
Copy link
Contributor

@ALTree Could you create the change just to see if all the tests pass?

@ALTree
Copy link
Member Author

ALTree commented Jul 26, 2017

I can do that, but what platforms are you interested in? For example, we have an OSX10.8 builder but not an OSX10.8 trybot. Builders run on master after a commit, trybots can be used to test a change before committing it.

So even if I upload a change and run the trybots on it, they won't tell us if the OSX10.8 issue is gone. In fact, with the old failed update in June the trybots said "OK" on my change and only after it was merged I noticed the OSX10.8, OSX10.10 and iPhone failures (by looking at the builders webpage).

@aalexand
Copy link
Contributor

I see. Then it doesn't make much sense, no. I didn't know the set of trybot envs is limited.

@gopherbot
Copy link

Change https://golang.org/cl/57370 mentions this issue: cmd/vendor/github.com/google/pprof: refresh from upstream

gopherbot pushed a commit that referenced this issue Nov 2, 2017
Update vendored pprof to commit 4fc39a00b6b8c1aad05260f01429ec70e127252c
from github.com/google/pprof (2017-11-01).

Fixes #19380
Updates #21047

Change-Id: Ib64a94a45209039e5945acbcfa0392790c8ee41e
Reviewed-on: https://go-review.googlesource.com/57370
Run-TryBot: Alberto Donizetti <alb.donizetti@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@ALTree
Copy link
Member Author

ALTree commented Nov 3, 2017

pprof was vendored and the builders are happy (except the plan9 ones), so I'm closing this issue.

I opened #22559 for the plan9 issue.

@ALTree ALTree closed this as completed Nov 3, 2017
@golang golang locked and limited conversation to collaborators Nov 3, 2018
@rsc rsc unassigned ALTree Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

4 participants