-
Notifications
You must be signed in to change notification settings - Fork 38.6k
build: remove mostly pointless BOOST_PROCESS macro #21182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1b960f0 to
ffe3d65
Compare
|
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'. |
|
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. |
|
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. |
|
Concept ACK |
|
In #15382 I just copied what I saw for another Boost dependency :-) |
Sjors
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK ffe3d65
I'm using this in #16546 now (will rebase that after this is merged).
|
cr ACK ffe3d65aaa18df9413c35e6914b42f7a400bf0b6: patch looks correct |
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.
ffe3d65 to
7bf04e3
Compare
|
ACK 7bf04e3 |
7bf04e3 build: remove mostly pointless BOOST_PROCESS macro (fanquake) Pull request description: 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. ACKs for top commit: laanwj: ACK 7bf04e3 Tree-SHA512: b270a0250f32df2078f986c165b8977967d8c06df80bf2773f3442f74b395a3bfa6544af1024d9b6524d90d47a0f6304194b3aced0e2ecb88e75916da945ccb6
Guix builds
|
|
Looks like the GUIX build failed? |
|
Will increase the disk size soon(tm)(c)(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
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.