The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31930.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewing ba3bb09a12d77b467aec8833e316ee4b5e45ee75
IMO, it would be simpler and more consistent to just update instructions like:
0-cmake --preset dev-mode -DWITH_USDT=OFF
1+cmake --preset dev-mode -DWITH_USDT=OFF -DWITH_MULTIPROCESS=OFF
It is true that files in src/ipc/ may contained translated _() strings in the future, but I don’t think they do right now. In the longer term, the suggestion to use depends should not be needed, because #31741 should allow easily building src/ipc/ code without depends. #31741 also removes the WITH_MULTIPROCESS option and replaces it with an ENABLE_IPC option, and #31802 replaces depends MULTIPROCESS option with a NO_MULTIPROCESS options, so these instructions will need to be updated in the future anyway, and I think it would be best to keep them simple and not involve the depends system if not needed.
Updates:
This is a follow-up of #31731.
Technically this change fixes the preset configuration
execution failure as it needs multiprocess to be enabled,
so we disable it using -DWITH_MULTIPROCESS=OFF.
This code will need to be updated in PRs #31741 and #31802.
I think after #31741 we should be able to drop the extra argument.
I thought we’d need to replace it by -DENABLE_IPC=OFF (as it will be ON in CMakePresets.json) and I was getting errors of missing config files (CapnProtoConfig.cmake & capnproto-config.cmake) when I run the instructions in #31741, but the problem was that I needed to install the Cap’n Proto for multiprocess (as in the documentation), so your statement is correct, I’m updating this PR’s description accordingly. Thanks!
