Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING941732f4ce0a: [CMAKE] Improve FindBerkeleyDB
rABC941732f4ce0a: [CMAKE] Improve FindBerkeleyDB

Summary

This diff refactors the FindBerkeleyDB.cmake module and improve it in
several ways:

It follows the cmake best practices on variable names;
It extracts a version number from the db.h header so a minimal version can be required (previously any version would match);
It creates an imported target that can be linked to propagate usage requirements (no longer needed to use the list variables);
It provides a best-effort path search to find the header and libraries, by combining around the version number (more future-proof);
It provides useful messages when the files are found;
It marks the cache variables as advanced to make it GUI friendly.

This is expected to serve as a reference to update the other FindPackage
modules in the same manner.

Test Plan

cmake -GNinja ..

Check in the output for lines similar to:

-- Found BerkeleyDB: /usr/include (found suitable version "5.3.28",
minimum required is "5.3")
-- Found BerkeleyDB libraries:
/usr/lib/x86_64-linux-gnu/libdb.so;/usr/lib/x86_64-linux-gnu/libdb_cxx.so

ninja all check

Run the Gitian builds.

Edit the src/wallet/CMakeLists.txt and change the minimum version to
19.42.

cmake -GNinja ..

Check that cmake fails to find the lib and complains with a meaningful
error message similar to:

Could NOT find BerkeleyDB: Found unsuitable version "5.3.28", but
required is at least "19.42" (found /usr/include)

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fabien added inline comments.Feb 27 2020, 20:32

cmake/modules/FindBerkeleyDB.cmake
58 ↗	(On Diff #16539)	The selection comes from the releases list: https://www.oracle.com/database/technologies/related/berkeleydb-release-history.html Our minimum version is 5.3, so I started with the 5.x branch up to the current 18.x. There is no major version between 6 and 18 (there used to be 12, but it's no longer available and used to be named 6.x anyway. Yes it's a mess). The minor version is just the latest available from the major branch, no longer evolving. Since 5.x and 6.x are (very) outdated, this is very likely that systems that still distribute these branches are providing the latest minor. 18.1 is the current version. Regarding the tests: almost every Linux I have uses 5.3. I failed to find a system with a 6.x version At least OSX with brew uses the 18.1
89 ↗	(On Diff #16539)	This is a rendering issue, because it is a tab in the regex string. The indentation is also with tabs, but rendered as spaces in phabricator.
89 ↗	(On Diff #16539)	Actually you can use the "View Options"=>"Show raw file (right)" to have the tabs rendered correctly. TIL.

Fix copyright years and put the separators on a sigle line.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing started.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9596: Diff 16549.Feb 27 2020, 20:43

Build Bitcoin-ABC / Diffs (Resource Intensive) / Bitcoin-ABC Gitian Diff Testing passed.

jasonbcox added inline comments.Feb 27 2020, 22:03

cmake/modules/FindBerkeleyDB.cmake
40 ↗	(On Diff #16539)	IMO this is better than before. If you think this is difficult to read, you could have 2-3 spaces between each item. Visually breaking them up should have a positive effect on readability without taking up vertical space.
89 ↗	(On Diff #16539)	Shouldn't the regex tab be a `\t` to make it explicit? If you do this, catching all whitespace is probably a better solution.

Improve the regex to match any number > 0 of spaces and tabs.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B9601: Diff 16555.Feb 27 2020, 22:34

Build Bitcoin-ABC / Diffs / Diff Testing passed.

deadalnix requested changes to this revision.Feb 27 2020, 23:41

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

cmake/modules/FindBerkeleyDB.cmake
27 ↗	(On Diff #16555)	You need two targets, one for the C and one for the C++ libraries.
143 ↗	(On Diff #16555)	So we are looking for the C and C++ version of the lib, if we find any then we consider we found BDB and run with it. That is not appropriate.
160 ↗	(On Diff #16555)	Why is this necessary? if the whole thing is cached, then this is redundant, and if this isn't cached, then you got to do something to not reprocess everything every time.
src/wallet/CMakeLists.txt
28 ↗	(On Diff #16555)	There is a C and a C++ lib. It seems like we need the C++ one here.

This revision now requires changes to proceed.Feb 27 2020, 23:41

Fabien planned changes to this revision.Feb 28 2020, 07:29

Fabien added inline comments.

cmake/modules/FindBerkeleyDB.cmake
27 ↗	(On Diff #16555)	This was on purpose, to accommodate Windows (and maybe others, idk) that build the C++ library in the `db` lib and have no `db_cxx`. Note that the native windows build is not supported, so it is only for future proofing. This can be 2 targets as suggested, or separated components, be will be special cased at the caller level if we introduce support for the native windows build at some point.
143 ↗	(On Diff #16555)	The db lib is made mandatory by the `find_package_handle_standard_args()` call below while the db_cxx library is optional. Once again this is to accommodate the windows weirdness.
160 ↗	(On Diff #16555)	Variables are cached but not targets. If you call the `find_package()` twice it will process the file twice. I can guard from re-running at the top of the file. This is usually not necessary as the consuming time is at the `find_path`/`find_library` level but here we have some loops and regex, so it might be worth it.

Use separate targets for C and C++.

Build Bitcoin-ABC / Diffs / Diff Testing started.

deadalnix requested changes to this revision.Feb 28 2020, 13:12

deadalnix added inline comments.

cmake/modules/FindBerkeleyDB.cmake
27 ↗	(On Diff #16555)	This is broken. A non window system that only has db installed will fail at link time. A C library cannot link db because it'll also fail at link time. This is broken.
160 ↗	(On Diff #16555)	You still parse headers and do various other crap.

This revision now requires changes to proceed.Feb 28 2020, 13:12

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9627: Diff 16600.Feb 28 2020, 13:18

Cache the version to avoid parsing the header multiple times.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B9632: Diff 16607.Feb 28 2020, 14:01

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Fabien planned changes to this revision.Feb 28 2020, 16:13

Extract the find component function into another file.
That will allow for sharing it between the various Findxxx modules.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9675: Diff 16676.Mar 2 2020, 10:47

Fix duplicated c1 message if the components are searched like c1, c2, c1.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9677: Diff 16679.Mar 2 2020, 12:10

Fabien added a child revision: D5393: [CMAKE] Improve FindEvent.Mar 2 2020, 12:56

Add missing include directories in the imported targets.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9684: Diff 16697.Mar 2 2020, 18:46

The general direction is good, but it seems like there are details to polish.

cmake/modules/ExternalLibraryHelper.cmake
22 ↗	(On Diff #16697)	What does the empty string do here?
cmake/modules/FindBerkeleyDB.cmake
152 ↗	(On Diff #16697)	You clearly have some formatting problems here.
src/wallet/CMakeLists.txt
6 ↗	(On Diff #16697)	I guess components are C and CXX, no need to repeat db.

This revision now requires changes to proceed.Mar 5 2020, 23:35

Fabien planned changes to this revision.Mar 6 2020, 07:40

Fabien added inline comments.

cmake/modules/ExternalLibraryHelper.cmake
22 ↗	(On Diff #16697)	It allows the variable after it to be empty and still have a valid command syntax. This avoids having an if case for each variable.
cmake/modules/FindBerkeleyDB.cmake
152 ↗	(On Diff #16697)	Oups, good catch !
src/wallet/CMakeLists.txt
6 ↗	(On Diff #16697)	If found it was more intuitive to use the library name for the component, but OK.