cmake: Add external signer support by hebasto · Pull Request #79 · hebasto/bitcoin

hebasto · 2024-01-19T21:04:33Z

A new configuration option is WITH_EXTERNAL_SIGNER.

pablomartin4btc

utACK 3371f3d

hebasto · 2024-01-27T16:01:01Z

Rebased.

fanquake · 2024-01-29T14:08:29Z

This branch works without the following workarounds from the master branch:

How is the first one fixed? I'm guessing it's because these checks are no-longer being run with same warning flags? I assume the second one is because -pthread is now just always being passed or something?

hebasto · 2024-01-29T16:21:04Z

This branch works without the following workarounds from the master branch:

How is the first one fixed? I'm guessing it's because these checks are no-longer being run with same warning flags?

Thanks! Fixed.

hebasto · 2024-01-29T21:09:38Z

I assume the second one is because -pthread is now just always being passed or something?

Not so simple :)

I tried to reproduce the initial issue, which was fixed in bitcoin#24397, using for testing the 077cfff commit and commands as follows:

make -C depends -j $(nproc) NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 NO_USDT=1 CC=gcc-10 CXX=g++-10
./autogen.sh && ./configure CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site --enable-external-signer

Ubuntu 20.04, libc 2.31 -- FAIL

config.log:

configure:26954: checking whether Boost.Process can be used
configure:26967: g++-10 -std=c++17 -o conftest -pipe -O2  -fno-extended-identifiers -I/bitcoin/depends/x86_64-pc-linux-gnu/include/  -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION -L/bitcoin/depends/x86_64-pc-linux-gnu/lib  conftest.cpp  >&5
/usr/bin/ld: /tmp/ccTX7pPT.o: in function `boost::asio::detail::posix_global_impl<boost::asio::system_context>::~posix_global_impl()':
conftest.cpp:(.text._ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED2Ev[_ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED5Ev]+0xa3): undefined reference to `pthread_join'
/usr/bin/ld: conftest.cpp:(.text._ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED2Ev[_ZN5boost4asio6detail17posix_global_implINS0_14system_contextEED5Ev]+0xc4): undefined reference to `pthread_detach'
collect2: error: ld returned 1 exit status

Ubuntu 22.04, libc 2.35 -- SUCCESS

FWIW, reading libc release notes did not give a clue.

fanquake · 2024-01-29T21:17:24Z

I would guess that's because recent versions of libc now include pthread, so pthread linking is no-longer required. I don't understand how this branch can drop the workaround in other cases, unless pthread is being linked some other way.

fanquake · 2024-01-29T21:20:41Z

FWIW, reading libc release notes did not give a clue.

It's in the 2.34 release notes: https://sourceware.org/pipermail/libc-alpha/2021-August/129718.html

In order to support smoother in-place-upgrades and to simplify
the implementation of the runtime all functionality formerly
implemented in the libraries libpthread, libdl, libutil, libanl has
been integrated into libc. New applications do not need to link with
-lpthread, -ldl, -lutil, -lanl anymore.

hebasto · 2024-01-29T22:53:26Z

Addressed @fanquake's comments. Add more CI jobs for Boost.Process edge cases.

hebasto · 2024-01-29T23:00:32Z

+    set(CMAKE_REQUIRED_FLAGS "${working_compiler_werror_flag}")
+    # Boost 1.78 requires the following workaround.
+    # See: https://github.com/boostorg/process/issues/235
+    string(APPEND CMAKE_REQUIRED_FLAGS " -Wno-error=narrowing")


By default, CMake treats

include directories from the interfaces of consumed Imported Targets as system directories.

That means that we have --enable-suppress-external-warnings behaviour for granted.

If we won't bother to mirror the opposite behaviour, these 4 lines might be dropped.

fanquake · 2024-01-30T11:07:53Z

PR description needs updating?

hebasto · 2024-01-30T11:08:36Z

PR description needs updating?

Sure. Updated.

hebasto · 2024-02-09T18:46:39Z

Rebased.

theuni · 2024-02-22T20:56:02Z

+  if(WITH_EXTERNAL_SIGNER)
+    include(CheckCXXSourceCompiles)
+    set(CMAKE_REQUIRED_INCLUDES ${Boost_INCLUDE_DIR})
+    set(CMAKE_REQUIRED_FLAGS "${working_compiler_werror_flag}")


I don't think we actually want this on, do we? What if some future boost version has a non-fatal warning? Don't we only want the Wno-error in case a user has turned on Werror ?

Yes, it does not correlate with the master branch (and I don't recall why I added it).

This line has been removed in the recent push to mirror the master branch behavior.

theuni · 2024-02-23T17:45:03Z

Looks good to me other than the outstanding comments:

Werror question
@fanquake's request for a better comment re glibc.

hebasto · 2024-02-24T13:50:32Z

Thanks for review!

Looks good to me other than the outstanding comments:

Werror question

@fanquake's request for a better comment re glibc.

Updated per feedback.

pablomartin4btc

tACK aa53fe3

Optional packages:
  GUI ................................. Qt5
  external signer ..................... ON
  NAT-PMP ............................. ON
  UPnP ................................ ON
  ZeroMQ .............................. ON
  USDT tracing ........................ ON

Tested it quickly running rpc enumeratesigners successfully (doen't work without the external signer feature enabled returning Method not found).

./build/src/bitcoin-cli -signet -datadir=$HEBASTO_DATADIR enumeratesigners
error code: -1
error message:
Error: restart bitcoind with -signer=<cmd>

More tests for Boost.Process edge cases.

m3dwards · 2024-03-07T11:46:30Z

ACK 617ef5b

Copied @pablomartin4btc trick for checking if signer support is available.

With cmake -S .. -DWITH_EXTERNAL_SIGNER=off:

➜  build git:(240118-cmake-AY) ./src/bitcoin-cli -regtest -datadir=/tmp/bitcoinddata enumeratesigners
error code: -32601
error message:
Method not found

With cmake -S .. -DWITH_EXTERNAL_SIGNER=on:

➜  build git:(240118-cmake-AY) ./src/bitcoin-cli -regtest -datadir=/tmp/bitcoinddata enumeratesigners
error code: -1
error message:
Error: restart bitcoind with -signer=<cmd>

Just as an aside, when I set external signer to "on" lowercase the print out after configure is: external signer ..................... ON in uppercase but when I set it to off lowercase the print out after configure is: external signer ..................... off in lowercase. Is this a cmake quirk?

m3dwards · 2024-03-07T11:48:34Z

+      " HAVE_BOOST_PROCESS
+    )
+    if(HAVE_BOOST_PROCESS)
+      if(WIN32)


NIT: if it's WIN32 couldn't you just return early with external signer OFF and not bother checking if boost supports boost process? Would cleanup one nested if.

You are right. However, I've chosen this clumsy approach to mimic the Autotools' one for the sake of easier reviewing,

Anyway, this code will be gone after bitcoin#28981.

hebasto · 2024-03-07T16:15:00Z

@m3dwards

Just as an aside, when I set external signer to "on" lowercase the print out after configure is: external signer ..................... ON in uppercase but when I set it to off lowercase the print out after configure is: external signer ..................... off in lowercase. Is this a cmake quirk?

That is a flaw in tri-state option implementation. Anyway, it will be gone before merging into the master branch; see #83.

pablomartin4btc · 2024-03-07T16:55:40Z

I just found out that this feature is disabled for Windows build in master. For some more context see @Sjors comment.

Sjors · 2024-03-07T19:41:24Z

@pablomartin4btc indeed, though after bitcoin#28981 bringing back Windows should be doable.

pablomartin4btc reviewed Jan 25, 2024

View reviewed changes

Comment thread vcpkg.json Outdated

hebasto force-pushed the cmake-staging branch from ff2f574 to dacc6f7 Compare January 27, 2024 15:54

hebasto force-pushed the 240118-cmake-AY branch from 3371f3d to 067bb25 Compare January 27, 2024 16:00

hebasto added the enhancement New feature or request label Jan 27, 2024

hebasto force-pushed the 240118-cmake-AY branch 2 times, most recently from 8a05399 to dd11216 Compare January 29, 2024 16:20

hebasto force-pushed the 240118-cmake-AY branch from dd11216 to adc9c84 Compare January 29, 2024 22:52

hebasto commented Jan 29, 2024

View reviewed changes

hebasto force-pushed the cmake-staging branch from 17f4870 to 6367599 Compare February 9, 2024 18:38

hebasto force-pushed the 240118-cmake-AY branch from adc9c84 to d361e3a Compare February 9, 2024 18:46

hebasto force-pushed the 240118-cmake-AY branch from d361e3a to 2e86eda Compare February 9, 2024 22:18

theuni reviewed Feb 22, 2024

View reviewed changes

hebasto force-pushed the 240118-cmake-AY branch from 2e86eda to aa53fe3 Compare February 24, 2024 13:45

pablomartin4btc approved these changes Feb 29, 2024

View reviewed changes

hebasto force-pushed the cmake-staging branch from 15edb82 to 9e07ce3 Compare March 1, 2024 15:33

hebasto added 2 commits March 1, 2024 17:45

cmake: Add external signer support

0fd39c9

fixup! ci: Test CMake edge cases

617ef5b

More tests for Boost.Process edge cases.

hebasto force-pushed the 240118-cmake-AY branch from aa53fe3 to 617ef5b Compare March 1, 2024 17:46

hebasto mentioned this pull request Mar 2, 2024

cmake: Fix test/config.ini generation #109

Merged

m3dwards reviewed Mar 7, 2024

View reviewed changes

hebasto merged commit 9a8c962 into cmake-staging Mar 7, 2024

Conversation

hebasto commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jan 27, 2024

Uh oh!

fanquake commented Jan 29, 2024

Uh oh!

hebasto commented Jan 29, 2024

Uh oh!

hebasto commented Jan 29, 2024

Uh oh!

fanquake commented Jan 29, 2024

Uh oh!

fanquake commented Jan 29, 2024

Uh oh!

hebasto commented Jan 29, 2024

Uh oh!

hebasto Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jan 30, 2024

Uh oh!

hebasto commented Jan 30, 2024

Uh oh!

hebasto commented Feb 9, 2024

Uh oh!

theuni Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 24, 2024

Choose a reason for hiding this comment

Uh oh!

theuni commented Feb 23, 2024

Uh oh!

hebasto commented Feb 24, 2024

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

m3dwards commented Mar 7, 2024

Uh oh!

m3dwards Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Mar 7, 2024

Uh oh!

pablomartin4btc commented Mar 7, 2024

Uh oh!

Sjors commented Mar 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hebasto commented Jan 19, 2024 •

edited

Loading

theuni Feb 22, 2024 •

edited

Loading

hebasto Mar 7, 2024 •

edited

Loading