util+doc: Clearer errors when attempting to run examples + polished docs

[hodlinator]

Found while reviewing #201.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, Eunovo, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[hodlinator]

Let me know if you see any merit to the Drahtbot recommendation (#213 (comment)):

Its return values -> Return values ["Its" incorrectly refers to the preceding expression; use "Return values" to make the sentence grammatical and clear]

[ryanofsky]

Code review ACK 91606b2. Thanks for the PR! Left some suggestions below, but overall I think this is an improvement.

Let me know if you see any merit to the Drahtbot recommendation (#213 (comment))

Not too important, I think it'd make sense to replace "Its return values" with "The return values." Currently it seems like "its" refers to capnp::RpcSystem which doesn't really make sense.

[ryanofsky]

In commit "example: Hint at possible problem when binaries are missing" (10d31a6)

This is an improvement, but if this could be updated to just handle errors from the Spawn() calls below and suggest binaries may be missing then, I think that would be better. Better generally to handle errors than try to predict them IMO. And in theory this change could also trigger errors unnecessarily if if binaries were not in the PWD but were in the PATH.

Also would be good to follow bitcoin style and either use a single-line if or add braces around multiline if.

[hodlinator]

Oof, I misremembered your suggestion and ended up moving the error to inside ExecProcess() in util.cpp instead. Let me know if you prefer something closer to your suggestion, no problem.

[ryanofsky]

re: #213 (comment)

Oof, I misremembered your suggestion and ended up moving the error to inside ExecProcess() in util.cpp instead. Let me know if you prefer something closer to your suggestion, no problem.

This is pretty good. I think your approach makes sense and is a clear improvement. I made my suggestion forgetting that the exec error is only available in the child process, and it would take extra effort to send the errno back to the parent process to use in an exception. Apparently there is a pattern for doing this using a temporary pipe and cloexec but it's more complicated and probably would be better to do in a followup.

[ryanofsky]

Code review ACK ec03a96 just addressing previous comments since last review: changing the missing executable check and updating markdown docs. All changes look very good now

[ryanofsky]

re: #213 (comment)

Oof, I misremembered your suggestion and ended up moving the error to inside ExecProcess() in util.cpp instead. Let me know if you prefer something closer to your suggestion, no problem.

This is pretty good. I think your approach makes sense and is a clear improvement. I made my suggestion forgetting that the exec error is only available in the child process, and it would take extra effort to send the errno back to the parent process to use in an exception. Apparently there is a pattern for doing this using a temporary pipe and cloexec but it's more complicated and probably would be better to do in a followup.

[Eunovo]

ACK ec03a96

[theuni]

utACK ec03a96