contrib: remove install

fanquake commented at 10:04 am on January 6, 2023: member

Now that we can build a bdb-only depends prefix (#26833), there is no need to maintain a bdb-building bash script, that does the same thing as depends, except worse, as it’s missing patches and workarounds. i.e #26623.

Someone that wants to compile bdb themselves, but doesn’t want to use other depends built libs, can do:

0make -C depends NO_BOOST=1 NO_LIBEVENT=1 NO_QT=1 NO_SQLITE=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1 NO_USDT=1
1...
2to: /path/to/bitcoin/depends/x86_64-pc-linux-gnu

which gives them a BDB only prefix, and then compile using:

0export BDB_PREFIX="/path/to/bitcoin/depends/x86_64-pc-linux-gnu"
1./autogen.sh
2./configure \
3    BDB_LIBS="-L${BDB_PREFIX}/lib -ldb_cxx-4.8" \
4    BDB_CFLAGS="-I${BDB_PREFIX}/include"

Wondering if we should extract the build bdb/legacy wallet docs somewhere, to avoid the repetition?

DrahtBot commented at 10:05 am on January 6, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, jarolrod, TheCharlatan, achow101
Concept ACK	murrayn

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26817 (doc: Remove copyright years (headers only) by MarcoFalke)

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.

DrahtBot added the label Scripts and tools on Jan 6, 2023

murrayn commented at 10:35 am on January 6, 2023: contributor

Concept ACK

in doc/build-openbsd.md:47 in 74dd72013f outdated

40@@ -41,16 +41,18 @@ pkg_add sqlite3
41 BerkeleyDB is only required to support legacy wallets.
42 
43 It is recommended to use Berkeley DB 4.8. You cannot use the BerkeleyDB library
44-from ports. However you can build it yourself, [using the installation script included in contrib/](/contrib/install_db4.sh), like so, from the root of the repository.
45+from ports. However you can build it yourself, [using depends](/depends).
46 
47 ```bash
48-./contrib/install_db4.sh `pwd`
49+make -C depends NO_BOOST=1 NO_LIBEVENT=1 NO_QT=1 NO_SQLITE=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1 NO_USDT=1

murrayn commented at 10:54 am on January 6, 2023:

This should be gmake for OpenBSD.

fanquake commented at 10:55 am on January 6, 2023:

Thanks. Fixed up.

fanquake force-pushed on Jan 6, 2023

in doc/build-unix.md:200 in 110e166e6b outdated

201-use [the installation script included in contrib/](/contrib/install_db4.sh)
202-like so:
203-
204-```shell
205-./contrib/install_db4.sh `pwd`
206+recommended to use Berkeley DB 4.8. If you have to build it yourself, and don't

murrayn commented at 11:02 am on January 6, 2023:

Wondering if this section should be titled Legacy Wallet Support rather than Berkeley DB?

fanquake force-pushed on Jan 17, 2023

doc: add new NO_* options from #26833 14ce84388f

contrib: remove install_db4.sh

Now that we can build a bdb-only depends prefix, there is no need to
maintain a bdb-building bash script, that does the same things as
depends, except worse, as it's missing patches and workarounds. i.e #26623.

44f3c7de21

fanquake force-pushed on Jan 18, 2023

fanquake marked this as ready for review on Jan 18, 2023

hebasto commented at 2:35 pm on January 20, 2023: member

Concept ACK.

in doc/build-unix.md:214 in 44f3c7de21

218-
219-Otherwise, you can build Bitcoin Core from self-compiled [depends](/depends/README.md).
220+./configure \
221+    BDB_LIBS="-L${BDB_PREFIX}/lib -ldb_cxx-4.8" \
222+    BDB_CFLAGS="-I${BDB_PREFIX}/include"
223+```

hebasto commented at 3:15 pm on January 20, 2023:

Actually, for an intention like “don’t want to use any other libraries built in depends” there is a dedicated ALLOW_HOST_PACKAGES=1 dependencies option:

0$ make -C depends NO_BOOST=1 NO_LIBEVENT=1 NO_QT=1 NO_SQLITE=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1 NO_USDT=1 ALLOW_HOST_PACKAGES=1
1$ ./configure CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site --with-gui --with-sqlite --enable-zmq --with-miniupnpc --with-natpmp --enable-usdt

fanquake commented at 4:00 pm on January 20, 2023:

ALLOW_HOST_PACKAGES=1 dependencies option:

I don’t particularly like the existence of that option in general, or want to introduce it here. So given that we acheive the same thing here (only bdb in the prefix) regardless of whether it’s used, I’d rather not add it.

fanquake commented at 4:02 pm on January 20, 2023:

That is also a somewhat more convoluted/confusing ./configure invocation, as opposed to just passing the flags.

hebasto commented at 6:08 pm on January 20, 2023:

That is also a somewhat more convoluted/confusing ./configure invocation

True.

as opposed to just passing the flags.

… which is error-prone: if a typo slipped in BDB_LIBS or BDB_CFLAGS, the configure script is not testing them and only following make will fail.

fanquake commented at 4:33 pm on January 21, 2023:

which is error-prone:

That’s hardly any worse than making users type out all this ./configure CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site --with-gui --with-sqlite --enable-zmq --with-miniupnpc --with-natpmp --enable-usdt, where any typo or forgotten flag would result in missing features that wouldn’t be noticed until runtime.

hebasto commented at 5:07 pm on January 21, 2023:

Users can read “Options used to compile and link:” output of the configure script to ensure all requested features are indeed enabled. But we cannot rely on users’ reasonable behavior :)

hebasto commented at 5:08 pm on January 21, 2023:

I agree with either approach, so labeling this comment as resolved.

hebasto approved

hebasto commented at 6:08 pm on January 20, 2023: member

ACK 44f3c7de21fd86343a2427d2e864ea2001499c5e

fanquake requested review from TheCharlatan on Jan 23, 2023

jarolrod approved

jarolrod commented at 0:49 am on January 24, 2023: member

ACK 44f3c7de21fd86343a2427d2e864ea2001499c5e

Did not test openbsd build instructions, but they are appropriate for this PR as that build doc instructed to use the db script.

TheCharlatan commented at 0:25 am on January 26, 2023: contributor

ACK 44f3c7de21fd86343a2427d2e864ea2001499c5e

Disabling the other targets to enable building only a specific build target seems a bit odd though. How about adding a $(package)_install target instead? Then we could call make bdb_install and get the same result, but with saner arguments. I made a small proof of concept here (just for building and installing, does not generate the config.site yet): https://github.com/TheCharlatan/bitcoin/pull/6 . Would this be potentially dangerous, because it skips over other installation steps that are required e.g. for otool? If so, I’d rather stick to the solution used in this PR than maintaining yet another way to use depends.

TheCharlatan approved