log: fix call depth for go1.12 panics #36144
Conversation
|
This change is |
pkg/util/log/crash_reporting.go
Outdated
| // - panic.go // not present in go1.12 | ||
| // - panic() | ||
| // so ReportPanic should pop four frames. | ||
| if ver := runtime.Version(); strings.HasPrefix(ver, "go1.12") { |
There was a problem hiding this comment.
I could see doing something fancier here like parsing into an integer. Feedback appreciated.
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/util/log/crash_reporting.go, line 96 at r1 (raw file):
Previously, ajwerner wrote…
I could see doing something fancier here like parsing into an integer. Feedback appreciated.
This is ugly. I don't have any better ideas, though. I suppose you could do this statically with build tags.
ajwerner
force-pushed
the
ajwerner/log-failure-in-112
branch
2 times, most recently
from
62a4745 to
420b4b3
Compare
This PR fixes the following test failure on Linux:
```
$ make test IGNORE_GOVERS=1 PKG=./pkg/util/log
--- FAIL: TestCrashReportingPacket (0.20s)
--- FAIL: TestCrashReportingPacket/#00 (0.00s)
crash_reporting_packet_test.go:155: expected crash_reporting_packet_test.go:85: boom, got testing.go:865: boom
--- FAIL: TestCrashReportingPacket/#1 (0.00s)
crash_reporting_packet_test.go:155: expected crash_reporting_packet_test.go:93: baam, got testing.go:865: baam
FAIL
```
Interestingly this is not quite the same failure noted in cockroachdb#35792, maybe that's
MacOS specific?
Release note: None
ajwerner
force-pushed
the
ajwerner/log-failure-in-112
branch
from
420b4b3 to
84bd549
Compare
The build tags worked better than I had expected given Updated accordningly. |
There was a problem hiding this comment.
Reviewable status:
|
bors r+ |
36144: log: fix call depth for go1.12 panics r=ajwerner a=ajwerner
This PR fixes the following test failure on Linux:
```
$ make test IGNORE_GOVERS=1 PKG=./pkg/util/log
--- FAIL: TestCrashReportingPacket (0.20s)
--- FAIL: TestCrashReportingPacket/#00 (0.00s)
crash_reporting_packet_test.go:155: expected crash_reporting_packet_test.go:85: boom, got testing.go:865: boom
--- FAIL: TestCrashReportingPacket/#1 (0.00s)
crash_reporting_packet_test.go:155: expected crash_reporting_packet_test.go:93: baam, got testing.go:865: baam
FAIL
```
Interestingly this is not quite the same failure noted in #35792, maybe that's
MacOS specific?
Release note: None
Co-authored-by: Andrew Werner <[email protected]>
Build succeeded |
This PR fixes the following test failure on Linux:
Interestingly this is not quite the same failure noted in #35792, maybe that's
MacOS specific?
Release note: None