-
Notifications
You must be signed in to change notification settings - Fork 275
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
Fix broken benchmarks #205
Conversation
Thanks!
Do you something to share (e.g., a screenshot) to see what this is about? My first feeling is that nicer benchmarks would be more helpful upstream (but yeah, C...)
Hm, it's indeed ugly. Not sure if we want this. |
The "naive" output (from running |
The no-std test was broken because of this PR: rust-lang/rust#67502 And tests on stable are failing because of: #203 |
Can we make the "unstable" feature imply recovery and rand? |
Done. that also solved the conflicts :) |
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.
ACK
Travis is done. I dunno why it's not updating on the PR. |
I've seen Travis status failing to update a few times in the past week (on upstream)... Another question: Why are my review comments gone here in the PR? :/ |
That's really weird. I can see you reviewed it but I can't see the actual review/text |
Currently the benchmarks in recovery weren't compiling.
I fixed and made travis compile them too.
I experimented with switching to criterion which looks pretty cool (works on stable, produces html plots, remembers last results to show you improvements/regressions etc.)
But dev-dependencies aren't allowed to be optional so this will break the tests on 1.22.0
rust-lang/cargo#1596
(We could add it to the regular dependencies though, like @TheBlueMatt is doing with mutagen https://github.com/rust-bitcoin/rust-lightning/pull/453/files#r368478034 )