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.

Sign up for GitHub

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

Jump to bottom

fix(godeltaprof): handle renamed runtime.pprof_cyclesPerSecond #107

Merged
merged 2 commits into from
May 22, 2024
Merged

fix(godeltaprof): handle renamed runtime.pprof_cyclesPerSecond #107

merged 2 commits into from
May 22, 2024

Conversation

korniltsev
Copy link
Collaborator

@korniltsev korniltsev commented May 22, 2024 •
edited
Loading

This fixes the build on gotip. The tests are still broken and will be fixed in a followup of #103

@korniltsev korniltsev changed the title fix runtime_cyclesPerSecond rename fix gotip building May 22, 2024
@korniltsev korniltsev changed the title fix gotip building fix(godeltaprof): handle renamed runtime.pprof_cyclesPerSecond May 22, 2024
@korniltsev korniltsev marked this pull request as ready for review May 22, 2024 07:40
simonswine
simonswine approved these changes May 22, 2024
View reviewed changes
Copy link

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (- that one suggestion)

godeltaprof/compat/stub_go23_test.go
"func github.com/grafana/pyroscope-go/godeltaprof/internal/pprof.runtime_expandFinalInlineFrame(stk []uintptr) []uintptr")
}

func TestRuntimeCyclesPerSecond(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to remove this test:

pyroscope-go/godeltaprof/compat/stub_test.go

Lines 25 to 29 in ffd2615

func TestSignatureCyclesPerSecondRuntime(t *testing.T) {
checkSignature(t, "runtime/pprof",
"runtime_cyclesPerSecond",
"func runtime/pprof.runtime_cyclesPerSecond() int64")
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think all of the tests in stub_tests.go are duplicated by the version-specific tests. So I removed all tests from stub_test.go.

@korniltsev korniltsev requested a review from simonswine May 22, 2024 09:08
simonswine
simonswine approved these changes May 22, 2024
View reviewed changes
Copy link

@simonswine simonswine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@korniltsev korniltsev merged commit 491e627 into main May 22, 2024
8 of 9 checks passed
@JacobOaks
Copy link

@korniltsev do you think we can get a tagged release for godeltaprof so there is a non-tip version usable with Go 1.23 which is coming out shortly?

@korniltsev
Copy link
Collaborator Author

This rename of runtime.pprof_cyclesPerSecond was agreed to be a regression in golang 1.23 and was renamed back, both in gotip golang/go@9a8995b#diff-e9b5b306cc1007e3faa0f5027ac5bcabc39f9d3915deca7cbfa9a20324d4363cR951 and godeltaprof 181f95a#diff-0281d9755a602179e47d3b8ca616fdf90f4069f91f3bccfbbfeb53cc72d64e55R26.

There were other issues found #103 . They were also fixed both in gotip golang/go@87abb4a and godeltaprof 181f95a

Do you have any other issues?

I've just tested on go version go1.23rc2 linux/amd64 and it seem to compile godeltaprof/v0.1.7 just fine.

@JacobOaks
Copy link

I missed that #95 was included in v0.1.7. Upgrading to v0.1.7 works for me too. Thanks and sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonswine simonswine simonswine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@korniltsev @JacobOaks @simonswine