This commit proposes documentation that would have been helpful to me when setting up RapidCheck to work correctly with Bitcoin Core. I tested these instructions repeatedly with Linux (Debian 4.19 / GCC 8.3) and macOS (10.14.6 / AppleClang 10).
The markdown version can be seen at https://github.com/jonatack/bitcoin/blob/rapidcheck-documentation/doc/rapidcheck.md.
17 | +a counter-example. For example, if the input is an integer, RapidCheck tries to 18 | +find the smallest integer for which the property is false. 19 | + 20 | +## Running 21 | + 22 | +Once RapidCheck is installed correctly, Bitcoin Core will automatically run the
If RapidCheck is installed, Bitcoin Core will automatically run the
Done
22 | +Once RapidCheck is installed correctly, Bitcoin Core will automatically run the 23 | +property-based tests with the unit tests during `make check`, unless the 24 | +`--without-rapidcheck` flag is passed when configuring. 25 | + 26 | +For more information, run `./configure --help` and see `--with-rapidcheck` under 27 | +"Optional Packages".
I don't think this needs to be quoted.
Done
49 | + ``` 50 | + 51 | + You should see: 52 | + 53 | + ```shell 54 | + -- The CXX compiler identification is <GNU/AppleClang/etc x.y.z>
I don't think we need all of this build output here.
I think you're right. The build output was helpful for me in debugging my CMake command but it is resolved now.
26 | +For more information, run `./configure --help` and see `--with-rapidcheck` under 27 | +"Optional Packages". 28 | + 29 | +## Setup 30 | + 31 | +The following instructions have been tested with Linux Debian 4.19/GCC 8.3 and
Can you drop the all the version numbers from here.
Done
29 | +## Setup 30 | + 31 | +The following instructions have been tested with Linux Debian 4.19/GCC 8.3 and 32 | +macOS 10.14.6/AppleClang 10. 33 | + 34 | +1. Ensure cmake is installed with `apt-get install cmake` on Linux or `brew
Instead of 1., with OS specific install instructions, you could add a "requires CMake" note to 3..
Done
Concept ACK. Thanks for writing more documentation. Some of it seems a bit verbose. i.e I don't think we need a dump of the rapidcheck build output in here (that's also likely to change, and wont look the same for everyone anyways). I left a few comments inline.
@Christewart I see you've seen this PR already, but would be good to get your review / thoughts as well.
Looks good to me, thanks for adding documentation!
ACK 8616c81f08c8118003e035808b3f237b3bb211a9 - Thanks for updating.
17 | +a counter-example. For example, if the input is an integer, RapidCheck tries to 18 | +find the smallest integer for which the property is false. 19 | + 20 | +## Running 21 | + 22 | +If RapidCheck is installed, Bitcoin Core will automatically run the
Do we want to run these by default? This forces everyone to install rapidcheck. I think that is reasonable when we have a pre-configured way to easily install rapidcheck. AFAIK this isn't this case, unless I missed a PR.
Currently, RapidCheck is auto-configured only if librapidcheck.a is detected, which IIUC doesn't force anyone to install it. I think this is outside the scope of the doc which addresses the current situation. FWIW I'm working on updating the RapidCheck Makefile to maybe/hopefully autobuild cleanly and can update this doc in that PR if needed.
(Rapidcheck auto-configure as seen in configure.ac:L134-138 and 1280-1292)
Thanks for reviewing!