build: remove mostly pointless BOOST_PROCESS macro - #21182
Merged
Conversation
hebasto
reviewed
Feb 15, 2021
fanquake
force-pushed
the
boost_process_nonsense
branch
from
February 15, 2021 08:28
1b960f0 to
ffe3d65
Compare
Member
|
Concept ACK These vague configure errors are very common and often spook users, have done so for years: Good to get rid of one instance at least.
TIL. This is pretty good news. So the 'unique' boost parts that we need:
Right! it is better to have configure flags that semantically make sense to users not just 'use implementation specific thing'. |
Member
|
boost::multi_index depends on the boost serialization library if its serialization functions are used (which we don't). According to the documentation, if you don't, then multi_index is purely headers only. |
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Contributor
|
Concept ACK |
Member
|
In #15382 I just copied what I saw for another Boost dependency :-) |
2 tasks
Contributor
Contributor
|
cr ACK ffe3d65aaa18df9413c35e6914b42f7a400bf0b6: patch looks correct |
laanwj
reviewed
Feb 16, 2021
Performing a series of link checks for a Boost component that is header-only doesn't make much sense, and currently means we just have another confusing Boost macro in our tree. I'm not sure why this was originally done this way; maybe Sjors or luke-jr can elaborate (bitcoin#15382 (929cda5))? The macro also has the side-effect of producing confusing error messages. i.e in bitcoin#20744, the CI is currently failing with: ```bash checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes checking for boostlib >= 1.58.0 (105800)... yes checking whether the Boost::Process library is available... yes configure: error: Could not find a version of the Boost::Process library! ``` This isn't useful, given there is no such thing as a `Boost::Process` library. This PR just removes the macro entirely, but maintains a `--with-boost-process` (defaulting to off), flag to configure. Hopefully this will also be removed, in favour of `--enable-disable-external-signer` if/when bitcoin#16546 is merged.
fanquake
force-pushed
the
boost_process_nonsense
branch
from
February 17, 2021 00:04
ffe3d65 to
7bf04e3
Compare
Member
|
ACK 7bf04e3 |
Contributor
Guix builds
|
Member
|
Looks like the GUIX build failed? |
Member
|
Will increase the disk size soon(tm)(c)(2021) |
laanwj
added a commit
that referenced
this pull request
Feb 23, 2021
f75e0c1 doc: add external-signer.md (Sjors Provoost) d4b0107 rpc: send: support external signer (Sjors Provoost) 245b445 rpc: signerdisplayaddress (Sjors Provoost) 7ebc7c0 wallet: ExternalSigner: add GetDescriptors method (Sjors Provoost) fc5da52 wallet: add GetExternalSigner() (Sjors Provoost) 259f52c test: external_signer wallet flag is immutable (Sjors Provoost) 2655197 rpc: add external_signer option to createwallet (Sjors Provoost) 2700f09 rpc: signer: add enumeratesigners to list external signers (Sjors Provoost) 07b7c94 rpc: add external signer RPC files (Sjors Provoost) 8ce7767 wallet: add ExternalSignerScriptPubKeyMan (Sjors Provoost) 157ea7c wallet: add external_signer flag (Sjors Provoost) f3e6ce7 test: add external signer test (Sjors Provoost) 8cf543f wallet: add -signer argument for external signer command (Sjors Provoost) f7eb7ec test: framework: add skip_if_no_external_signer (Sjors Provoost) 87a9794 configure: add --enable-external-signer (Sjors Provoost) Pull request description: Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d). This PR lets `bitcoind` call an arbitrary command `-signer=<cmd>`, e.g. a hardware wallet driver, where it can fetch public keys, ask to display an address, and sign a transaction (using PSBT under the hood). It's design to work with https://github.com/bitcoin-core/HWI, which supports multiple hardware wallets. Any command with the same arguments and return values will work. It simplifies the manual procedure described [here](https://github.com/bitcoin-core/HWI/blob/master/docs/bitcoin-core-usage.md). Usage is documented in [doc/external-signer.md]( https://github.com/Sjors/bitcoin/blob/2019/08/hww-box2/doc/external-signer.md), which also describes what protocol a different signer binary should conform to. Use `--enable-external-signer` to opt in, requires Boost::Process: ``` Options used to compile and link: with wallet = yes with gui / qt = no external signer = yes ``` It adds the following RPC methods: * `enumeratesigners`: asks <cmd> for a list of signers (e.g. devices) and their master key fingerprint * `signerdisplayaddress <address>`: asks <cmd> to display an address It enhances the following RPC methods: * `createwallet`: takes an additional `external_signer` argument and fetches keys from device * `send`: automatically sends transaction to device and waits Usage TL&DR: * clone HWI repo somewhere and launch `bitcoind -signer=../HWI/hwi.py` * check if you can see your hardware device: `bitcoin-cli enumeratesigners` * create wallet and auto import keys `bitcoin-cli createwallet "hww" true true "" true true true` * display address on device: `bitcoin-cli signerdisplayaddress ...` * to spend, use `send` RPC and approve transaction on device Prerequisites: - [x] #21127 load wallet flags before everything else - [x] #21182 remove mostly pointless BOOST_PROCESS macro Potentially useful followups: - GUI support: bitcoin-core/gui#4 - bumpfee support - (automatically) verify (a subset of) keys on the device after import, through message signing ACKs for top commit: laanwj: re-ACK f75e0c1 Tree-SHA512: 7db8afd54762295c1424c3f01d8c587ec256a72f34bd5256e04b21832dabd5dc212be8ab975ae3b67de75259fd569a561491945750492f417111dc7b6641e77f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Performing a series of link checks for a Boost component that is
header-only doesn't make much sense, and currently means we just have
another confusing Boost macro in our tree. I'm not sure why this was
originally done this way; maybe Sjors or luke-jr can elaborate (#15382 (929cda5))?
The macro also has the side-effect of producing confusing error
messages. i.e in #20744, the CI is currently failing with:
This isn't useful, given there is no such thing as a
Boost::Processlibrary.This PR just removes the macro entirely, but maintains a
--with-boost-process(defaulting to off), flag to configure. Hopefully this will also be
removed, in favour of
--enable/disable-external-signerif/when #16546is merged.