-
Notifications
You must be signed in to change notification settings - Fork 147
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
Provide stub so apps using goleak can build unchanged with tinygo. #72
base: master
Are you sure you want to change the base?
Conversation
Tinygo can build a large subset of go programs already. Unfortunately, tinygo currently does not quite support goleak. As a temporary measure, use the tinygo build tag to provide a stub for goleak. This will expand the world of go programs that run properly on tinygo, and remove one more obstacle keeping embedded system and wasm users from adopting go and goleak. This can be reverted if/when tinygo is enhanced to fully support goleak.
Just for my education
Do you know what exactly is missing in tinygo for goleak to run? Is that even on their roadmap? Either way, I'd probably defer to @abhinav for a stamp here. |
Tinygo would have to e.g. implement runtime.Stack(). Whether or not this is on their roadmap, gracefully falling back to a no-op on tinygo feels like the right thing to do until tinygo decides to support goleak. |
For desktop systems I can see it happening some day, but for WebAssembly and embedded systems it probably won't happen. Because |
Details aside, software that uses goleak is increasingly going to be built with both go and tinygo, may as well make it easy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against supporting tinygo, but the current approach of only stubbing VerifyNone
doesn't seem ideal -- what if someone is using Find
? I think users of VerifyTestMain
will get a compile error with this approach (the method isn't included for tinygo, but no alternative is available).
Trying to reproduce the issue, when goleak is used, a tinygo test fails with:
panic: runtime error: index out of range
It looks like the panic is coming from:
func Current() Stack {
return getStacks(false)[0]
}
This is because getStacks
is unable to parse a stack (the output of runtime.Stack
is empty), so we get an empty list, but try to access index 0, causing the panic.
I'll propose another approach: when we get no stacks, return an error indicating that stack parsing failed, and have VerifyNone
log that goleak checks are skipped as the stack couldn't be parsed. That way it's not a hack specific to tinygo, but instead more general handling that could also apply to future go releases (or other go compilers) where the stack format is not recognised by goleak.
I like the idea. The message should probably be suppressed or only shown once on tinygo, though, otherwise it could dominate logs. |
2 year old PR and still didn't get merged! 🤷🏻♂️ |
@AbdullahWins, this comment by @prashantv mentions why the PR as it is cannot be merged, and what it'll take for it to get merged. |
Thanks for the fast ⏩ response and also the clarification. |
Tinygo can build a large subset of go programs already.
Unfortunately, tinygo currently does not quite support goleak.
As a temporary measure, use the tinygo build tag to provide a stub
for goleak. This will expand the world of go programs that
run properly on tinygo, and remove one more obstacle keeping
embedded system and wasm users from adopting go and goleak.
This can be reverted if/when tinygo is enhanced to fully
support goleak.
Fixes #71