-
Notifications
You must be signed in to change notification settings - Fork 632
[DEVOPS-632] ci: Build benchmarks in hydra and buildkite #3008
Conversation
5114405
to
cf0ba8f
Compare
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.
LGTM
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 left a minor naming suggestion, but it's not so serious as to be a blocker.
@@ -10,6 +10,7 @@ in | |||
, forceDontCheck ? false | |||
, enableProfiling ? false | |||
, enableDebugging ? false | |||
, enableBenchmarks ? true |
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.
You asked whether this is a good default. Yes, it makes sense. Devs should get feedback about benchmarks that don't build correctly, but we shouldn't spend time running all the benchmarks every build by default.
default.nix
Outdated
# enableBenchmarks argument is supplied. | ||
withBenchmarks = drv: if enableBenchmarks | ||
then doBenchmark (appendConfigureFlag drv "--enable-benchmarks") | ||
else drv; |
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.
Can we call this buildWithBenchmarks
to clarify it has to do with building the benchmarks rather than running them? When looking at the withBenchmarks
call sites people may mistakenly think the latter.
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.
Fixed, thanks.
Description
Benchmarks were not being built in CI and therefore were getting broken without anyone noticing.
This enables building (but not running) of benchmarks.
The CI for this PR should successfully fail because it doesn't include the merge of #3005.
I would like some feedback about the defaults in nix. Should the
enableBenchmarks
arg default to true or false? Should there be separate derivations with benchmarks enabled?Linked issue
https://iohk.myjetbrains.com/youtrack/issue/DEVOPS-632
Type of change
Developer checklist
QA Steps
Screenshots