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
Assignees
Milestone

Comments

@ALTree
Copy link
Member

@ALTree 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:

  • We had an OSX10.8 failure, an OSX10.10 failure, and failures on the darwin-arm-a1549ios and darwin-arm64-a1549ios builders. All these failures were on the recently introduced TestHttpsInsecure test. I reported the issue upstream (google/pprof#146).

  • Both the android builders were failing (failure1, failure2), again, on TestHttpsInsecure, but with a different error (related to filesystem permissions, apparently).

  • The linux-arm-arm5spacemonkey was failing on the TestHttpsInsecure test, with the same error we got on the android builders (issues with filesystem permissions).

  • Two of the plan9 builders (plan9-386 and plan9-arm) were failing (failure1, failure2), again, on TestHttpsInsecure, with the same error we got on the darwin 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
@aalexand
Copy link
Contributor

@aalexand aalexand commented Jul 21, 2017

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 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

@bradfitz bradfitz commented Jul 21, 2017

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

@aalexand aalexand commented Jul 26, 2017

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

@ALTree
Copy link
Member Author

@ALTree 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

@aalexand aalexand commented Jul 26, 2017

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

@ALTree
Copy link
Member Author

@ALTree 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

@aalexand aalexand commented Jul 26, 2017

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

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 22, 2017

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 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 Nov 3, 2017
@golang golang locked and limited conversation to collaborators Nov 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.