Page MenuHomePhabricator

[CMAKE] Build checkblock benchmark
ClosedPublic

Authored by Fabien on Jun 18 2019, 21:16.

Details

Summary

This has been left apart due to the required .raw to .h transformation.

Test Plan
ninja bench-bitcoin

The benchmark should build and run with no issue.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix requested changes to this revision.Jun 23 2019, 00:08
deadalnix added inline comments.
src/bench/CMakeLists.txt
22 ↗(On Diff #9523)

It's better to add a custom target and set the dependencies on it so you don't run the command every time.

src/bench/data/convert-raw-to-header.sh
10 ↗(On Diff #9523)

You may want to look into src/test/data/generate_header.py and how it is used.

This revision now requires changes to proceed.Jun 23 2019, 00:08
src/bench/CMakeLists.txt
22 ↗(On Diff #9523)

I think a custom command is a better fit here, as there is no point in building the raw.h file apart from the whole bench executable (the same is used for generate_header.py, for the same reason I guess).
The command won't run every time, only when there is a change on the dependencies (I'll make sure to improve this) or when the output is required by another target, which is how the command gets run in this case as the .h files are dependencies for the bitcoin-bench target.

src/bench/data/convert-raw-to-header.sh
10 ↗(On Diff #9523)

Good idea ! Using a redirection will make the script much simpler.

Improve dependencies, use file redirection to make the script simpler.

deadalnix requested changes to this revision.Jun 25 2019, 15:33
deadalnix added inline comments.
src/bench/data/convert-raw-to-header.sh
6 ↗(On Diff #9647)

There are no dependency on hexdump. I don't understand why the approach taken by src/test/data/generate_header.py is not preferable.

This revision now requires changes to proceed.Jun 25 2019, 15:33
Fabien planned changes to this revision.Jun 25 2019, 15:50
Fabien added inline comments.
src/bench/data/convert-raw-to-header.sh
6 ↗(On Diff #9647)

The dependency exists as long as autotools is still supported, the script is just the direct translation of the Makefile.bench.include scipt (only special autotools variables names are adapted).
That said, I don't think the generate_headers.py solution (translation of the bash script to a python script) is worst, it's even a better long-term solution as it effectively removes the hexdump dependency when autotools support gets dropped. I will do the translation.

Translate the conversion script to python

deadalnix added inline comments.
src/bench/data/convert-raw-to-header.py
18 ↗(On Diff #9660)

En Taro Adun!

This revision is now accepted and ready to land.Jun 25 2019, 23:19
This revision was automatically updated to reflect the committed changes.