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

build: no-longer fail default configure if BDB isn't available #23168

Merged
merged 1 commit into from
Oct 11, 2021
Merged

build: no-longer fail default configure if BDB isn't available #23168

merged 1 commit into from
Oct 11, 2021

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Oct 4, 2021

Inline with moving to descriptor (sqlite) wallets by default for 0.23,
this adapts the build system so that a default ./configure invocation
no-longer fails if BDB isn't present. Currently, if configure is run
with no options, and no BDB is present, we'll fail with:

checking for Berkeley DB C++ headers... no
configure: error: libdb_cxx headers missing, Bitcoin Core requires this library for BDB wallet support (--without-bdb to disable BDB wallet support)

If descriptor wallets are to be the default, this behaviour no longer
makes sense, as a builder should be able to configure and build, to use
a wallet, without BDB installed, and without passing additional
arguments, i.e --without-bdb or --with-incompatible-bdb, to
configure.

With this change, running configure will no-longer fail, but will
instead print:

checking for Berkeley DB C++ headers... no
configure: WARNING: libdb_cxx headers missing
configure: WARNING: Bitcoin Core requires this library for BDB (legacy) wallet support
configure: WARNING: Passing --without-bdb will suppress this warning
checking for sqlite3 >= 3.7.17... yes
checking whether to build wallet with support for sqlite... yes

@jarolrod
Copy link
Member

jarolrod commented Oct 4, 2021

strong concept ack

@practicalswift
Copy link
Contributor

practicalswift commented Oct 4, 2021 •
edited
Loading

Strong Strong Concept ACK

This will greatly improve the first time Bitcoin Core build experience by allowing the expected and familiar ./configure && make routine :)

darosior
darosior reviewed Oct 4, 2021
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Concept ACK

@laanwj
Copy link
Member

laanwj commented Oct 4, 2021

What about --with-incompatible-bdb this is hardwired into my fingers now—

without passing additional arguments, i.e --without-bdb or --with-incompatible-bdb, to configure.

Thanks 😄

@amadeuszpawlik
Copy link
Contributor

Very nice!

@hebasto
Copy link
Member

hebasto commented Oct 4, 2021

Concept ACK.

@hebasto
Copy link
Member

hebasto commented Oct 4, 2021

Maybe also replace an error with a warning and disabling BDB support for the following case:

$ ./autogen.sh 
$ ./configure --quiet
configure: error: Found Berkeley DB other than 4.8, required for portable BDB wallets (--with-incompatible-bdb to ignore or --without-bdb to disable BDB wallet support)

?

@fanquake
Copy link
Member Author

fanquake commented Oct 4, 2021 •
edited
Loading

and disabling BDB support for the following case:

What do you mean by disabling BDB support? I think this should remain as an error for now.

On second thought, we probably could make this into a warning, and not build when the incompatible flag is missing. Still forcing builders to opt into using incompatible versions but no-longer failing if some bdb is installed.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 5, 2021

Guix builds

File commit 9e530c6
(master)		 commit 0711554
(master and this pull)
*.tar.gz 1b1aae06add6cb19... 6dfa8b20e03eff49...
guix_build.log 6b3a2a468bf3c035... a599afdfd0d29c8c...
guix_build.log.diff 16ebe0b94d003750...

@fanquake
build: no-longer fail default configure if BDB isn't available
747cd17 
Inline with moving to descriptor (sqlite) wallets by default for 0.23,
this adapts the build system so that a default `./configure` invocation
no-longer fails if BDB isn't present. Currently, if configure is run
with no options, and no BDB is present, we'll fail with:
```bash
checking for Berkeley DB C++ headers... no
configure: error: libdb_cxx headers missing, Bitcoin Core requires this library for BDB wallet support (--without-bdb to disable BDB wallet support)
```

If descriptor wallets are to be the default, this behaviour no longer
makes sense, as a builder should be able to configure and build, to use
a wallet, without BDB installed, and without passing additional
arguments, i.e `--without-bdb` or `--with-incompatible-bdb`, to
configure.

With this change, running configure will no-longer fail, and will
instead print:
```bash
checking for Berkeley DB C++ headers... no
configure: WARNING: libdb_cxx headers missing
configure: WARNING: Bitcoin Core requires this library for BDB (legacy) wallet support
configure: WARNING: Passing --without-bdb will suppress this warning
checking for sqlite3 >= 3.7.17... yes
checking whether to build wallet with support for sqlite... yes
```
@fanquake fanquake force-pushed the dont_fail_no_bdb_default_configure branch from d9841d6 to 747cd17 Compare October 5, 2021 03:39
@fanquake
Copy link
Member Author

fanquake commented Oct 5, 2021

Maybe also replace an error with a warning and disabling BDB support for the following case:

I've done this now.

hebasto
hebasto reviewed Oct 10, 2021
View reviewed changes
build-aux/m4/bitcoin_find_bdb48.m4
],[
AC_MSG_ERROR([Found Berkeley DB other than 4.8, required for portable BDB wallets (--with-incompatible-bdb to ignore or --without-bdb to disable BDB wallet support)])
AC_MSG_WARN([Found Berkeley DB other than 4.8])
AC_MSG_WARN([BDB (legacy) wallets opened by this build would not be portable!])
Copy link
Member

Choose a reason for hiding this comment

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

In this case the build won't support BDB wallets at all (use_bdb=no).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's intended. We don't want to fail in the case --with-incompatible-bdb isn't passed.

Isn't that exactly what you asked for: #23168 (comment)?

Copy link
Member

@hebasto hebasto Oct 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't that exactly what you asked for: #23168 (comment)?

Yes, it is.

My concerns are about message text, not about logic.

I mean "BDB (legacy) wallets" cannot be "opened by this build", no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the messaging is fine as is. We are just warning that there is an incompatible BDB present. Lack of BDB wallet support is also part of the configure summary.

hebasto
hebasto approved these changes Oct 10, 2021
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 747cd17, tested on Linux Mint 20.2 (x86_64) with the (un)installed system packages libdb-dev and libdb++-dev.

@fanquake fanquake merged commit d1e63aa into bitcoin:master Oct 11, 2021
@fanquake fanquake deleted the dont_fail_no_bdb_default_configure branch October 11, 2021 00:40
@katesalazar
Copy link
Contributor

Once decided BDB is going out,
the earliest people gets the shock about removing it,
the better. This is a good idea. 👍

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 11, 2021
@fanquake
Merge bitcoin#23168: build: no-longer fail default configure if BDB i…
db3af6d 
…sn't available

747cd17 build: no-longer fail default configure if BDB isn't available (fanquake)

Pull request description:

  Inline with moving to descriptor (sqlite) wallets by default for 0.23,
  this adapts the build system so that a default `./configure` invocation
  no-longer fails if BDB isn't present. Currently, if configure is run
  with no options, and no BDB is present, we'll fail with:
  ```bash
  checking for Berkeley DB C++ headers... no
  configure: error: libdb_cxx headers missing, Bitcoin Core requires this library for BDB wallet support (--without-bdb to disable BDB wallet support)
  ```

  If descriptor wallets are to be the default, this behaviour no longer
  makes sense, as a builder should be able to configure and build, to use
  a wallet, without BDB installed, and without passing additional
  arguments, i.e `--without-bdb` or `--with-incompatible-bdb`, to
  configure.

  With this change, running configure will no-longer fail, but will
  instead print:
  ```bash
  checking for Berkeley DB C++ headers... no
  configure: WARNING: libdb_cxx headers missing
  configure: WARNING: Bitcoin Core requires this library for BDB (legacy) wallet support
  configure: WARNING: Passing --without-bdb will suppress this warning
  checking for sqlite3 >= 3.7.17... yes
  checking whether to build wallet with support for sqlite... yes
  ```

ACKs for top commit:
  hebasto:
    ACK 747cd17, tested on Linux Mint 20.2 (x86_64) with the (un)installed system packages `libdb-dev` and `libdb++-dev`.

Tree-SHA512: ae316d71ad0803c9d4b02a5fedcade08242650d987cc047840493ba4a881e71ff48b099075bb7c325307d44744fcdeccb57f7fa8db4135c81a5835841f562afa
@hebasto hebasto mentioned this pull request Sep 1, 2022
Closed
16 tasks
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@darosior darosior darosior left review comments

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@fanquake @jarolrod @practicalswift @laanwj @amadeuszpawlik @hebasto @DrahtBot @katesalazar @darosior @maflcko