Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cmake: Switch from tri-state options to boolean. Stage THREE #164

Merged
merged 1 commit into from
May 4, 2024
Merged

cmake: Switch from tri-state options to boolean. Stage THREE #164

merged 1 commit into from
May 4, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Apr 22, 2024

This PR is a continuation of #161 and #162 and tackles with the WITH_SQLITE and WITH_BDB options.

For the latter the default value OFF is suggested.

The #83 (comment):

Shouldn't WITH_SQLITE=ON imply ENABLE_WALLET? What (extra) does ENABLE_WALLET do here? I'm not sure we'll actually want to keep it as an option.

has been addressed.

@hebasto hebasto mentioned this pull request Apr 22, 2024
Closed
@hebasto
Copy link
Owner Author

hebasto commented Apr 22, 2024

cc @achow101

@hebasto hebasto added the enhancement New feature or request label Apr 23, 2024
@hebasto hebasto mentioned this pull request Apr 24, 2024
Merged
@hebasto hebasto added this to the User Interface milestone Apr 27, 2024
hebasto added a commit that referenced this pull request May 1, 2024
@hebasto
Merge #169: cmake: Switch from tri-state options to boolean. Stage FOUR
3c26fec 
5de23c7 fixup! cmake: Add preset for MSVC build (Hennadii Stepanov)
09b1d5b fixup! ci: Test CMake edge cases (Hennadii Stepanov)
820a538 cmake [KILL 3-STATE]: Switch `WITH_QRENCODE` to boolean w/ default ON (Hennadii Stepanov)
017aeda cmake [FIXUP]: Move `find_program(brew)` out from introspection file (Hennadii Stepanov)

Pull request description:

  This PR is a continuation of #161, #162 and #164 and tackles with the `WITH_QRENCODE`  options.

  It becomes enabled when building GUI with the default value `ON`.

Top commit has no ACKs.

Tree-SHA512: 9cd923ef89e0a485330205db4389e42c001cbd6683a14b1e0c665d8e790e8bb2cae7b8b1929818443aea8eb5cbf6cafa79f4e44f41a893f54a5cdc09abc49dc1
@hebasto
Copy link
Owner Author

hebasto commented May 1, 2024

Rebased.

TheCharlatan
TheCharlatan approved these changes May 4, 2024
View reviewed changes
Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

CMakeLists.txt
)
if(WARN_INCOMPATIBLE_BDB)
message(WARNING "If this is intended, pass \"-DWARN_INCOMPATIBLE_BDB=OFF\".\n"
"Passing \"-DWITH_BDB=OFF\" will suppress this warning."
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just moved from before, but it is kind of a weird suggestion to suppress something by switching of an entire feature.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It came from

bitcoin/build-aux/m4/bitcoin_find_bdb48.m4

Lines 48 to 66 in eb0bdbd

if test "$bdbpath" = "X"; then
use_bdb=no
AC_MSG_RESULT([no])
AC_MSG_WARN([libdb_cxx headers missing])
AC_MSG_WARN(AC_PACKAGE_NAME[ requires this library for BDB (legacy) wallet support])
AC_MSG_WARN([Passing --without-bdb will suppress this warning])
elif test "$bdb48path" = "X"; then
BITCOIN_SUBDIR_TO_INCLUDE(BDB_CPPFLAGS,[${bdbpath}],db_cxx)
AC_ARG_WITH([incompatible-bdb],[AS_HELP_STRING([--with-incompatible-bdb], [allow using a bdb version other than 4.8])],[
AC_MSG_WARN([Found Berkeley DB other than 4.8])
AC_MSG_WARN([BDB (legacy) wallets opened by this build will not be portable!])
use_bdb=yes
],[
AC_MSG_WARN([Found Berkeley DB other than 4.8])
AC_MSG_WARN([BDB (legacy) wallets opened by this build would not be portable!])
AC_MSG_WARN([If this is intended, pass --with-incompatible-bdb])
AC_MSG_WARN([Passing --without-bdb will suppress this warning])
use_bdb=no
])

Any suggestions to improve the behavior?

@hebasto
Copy link
Owner Author

hebasto commented May 4, 2024

@TheCharlatan

lgtm

Sorry for rebasing. There was a conflict.

Mind approving once more?

TheCharlatan
TheCharlatan approved these changes May 4, 2024
View reviewed changes
Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2b4ad50

@hebasto hebasto merged commit f918ccb into cmake-staging May 4, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheCharlatan TheCharlatan TheCharlatan approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
UI, 23th May
Development

Successfully merging this pull request may close these issues.
2 participants
@hebasto @TheCharlatan