Skip to content

contrib: remove install_db4.sh #26834

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
Merged
achow101 merged 2 commits into from
Jan 27, 2023
Merged

contrib: remove install_db4.sh #26834

achow101 merged 2 commits into from
Jan 27, 2023
+20 −271

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Jan 6, 2023 •
edited
Loading

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:

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
...
to: /path/to/bitcoin/depends/x86_64-pc-linux-gnu

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

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

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 6, 2023 •
edited
Loading

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.

@fanquake fanquake mentioned this pull request Jan 6, 2023
Merged
@murrayn
Copy link
Contributor

murrayn commented Jan 6, 2023

Concept ACK

murrayn
murrayn reviewed Jan 6, 2023
View reviewed changes
@fanquake fanquake force-pushed the remove_install_db4 branch from 74dd720 to 110e166 Compare January 6, 2023 10:55
murrayn
murrayn reviewed Jan 6, 2023
View reviewed changes
doc/build-unix.md Outdated
Copy link
Contributor

@murrayn murrayn Jan 6, 2023

Choose a reason for hiding this comment

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

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

@DrahtBot DrahtBot mentioned this pull request Jan 6, 2023
Closed
@jonatack jonatack mentioned this pull request Jan 9, 2023
Closed
@DrahtBot DrahtBot mentioned this pull request Jan 14, 2023
Merged
@fanquake fanquake force-pushed the remove_install_db4 branch 2 times, most recently from 2651472 to e20d101 Compare January 17, 2023 10:19
@fanquake
doc: add new NO_* options from bitcoin#26833
14ce843
@fanquake
contrib: remove install_db4.sh
44f3c7d 
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 bitcoin#26623.
@fanquake fanquake marked this pull request as ready for review January 18, 2023 16:59
@hebasto hebasto mentioned this pull request Jan 20, 2023
Closed
16 tasks
@hebasto
Copy link
Member

hebasto commented Jan 20, 2023

Concept ACK.

hebasto
hebasto reviewed Jan 20, 2023
View reviewed changes
hebasto
hebasto approved these changes Jan 20, 2023
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 44f3c7d

@fanquake fanquake requested a review from sedited January 23, 2023 12:08
jarolrod
jarolrod approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK 44f3c7d

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

@sedited
Copy link
Contributor

sedited commented Jan 26, 2023

ACK 44f3c7d

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): sedited#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.

@achow101
Copy link
Member

achow101 commented Jan 27, 2023

ACK 44f3c7d

@achow101 achow101 merged commit 483a4bb into bitcoin:master Jan 27, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 27, 2023
@achow101
Merge bitcoin#26834: contrib: remove install_db4.sh
485b014
@fanquake fanquake deleted the remove_install_db4 branch January 28, 2023 15:19
fanquake added a commit that referenced this pull request Feb 16, 2023
@fanquake
Merge #26773: doc: FreeBSD build doc updates to reflect removal of in…
75f0e0b 
…stall_db4.sh

c572eae update the freebsd build doc to reflect recent changes to DB4 install process (Murray Nesbitt)

Pull request description:

  This PR introduces documentation changes needed to keep up with #26834.

ACKs for top commit:
  fanquake:
    ACK c572eae - have not tested, but looks ok.

Tree-SHA512: 42a79e7b45834916b1b738db524b51b9ff4fde8348ba66fc331ff6603532dd9fce73ea392eef97d31112326c6d60ec2c5c7c29e66aab33aaf846aab8aea1d1aa
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 16, 2023
@fanquake
Merge bitcoin#26773: doc: FreeBSD build doc updates to reflect remova…
2805e9f 
…l of install_db4.sh

c572eae update the freebsd build doc to reflect recent changes to DB4 install process (Murray Nesbitt)

Pull request description:

  This PR introduces documentation changes needed to keep up with bitcoin#26834.

ACKs for top commit:
  fanquake:
    ACK c572eae - have not tested, but looks ok.

Tree-SHA512: 42a79e7b45834916b1b738db524b51b9ff4fde8348ba66fc331ff6603532dd9fce73ea392eef97d31112326c6d60ec2c5c7c29e66aab33aaf846aab8aea1d1aa
@Sjors
Copy link
Member

Sjors commented Feb 17, 2023

Post merge: just noticed the script was gone, but the bdb-only-depends approach works.

@jonatack
Copy link
Member

jonatack commented Feb 17, 2023

Made a number of updates to https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests that include this change.

@willcl-ark willcl-ark mentioned this pull request May 2, 2023
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Feb 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

@sedited sedited sedited approved these changes

+2 more reviewers

@murrayn murrayn murrayn left review comments

@jarolrod jarolrod jarolrod approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Scripts and tools

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@fanquake @DrahtBot @murrayn @hebasto @sedited @achow101 @Sjors @jonatack @jarolrod