[CMAKE] Improve FindEvent
ClosedPublic
Actions

Authored by Fabien on Mar 2 2020, 12:56.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING591a342ec391: [CMAKE] Improve FindEvent
rABC591a342ec391: [CMAKE] Improve FindEvent

Summary

This diff uses the facilities introduced in D5339.

It allows for checking the minimum version on a best effort basis: it
requires to run a libevent function so is not practical when cross
compiling.
It also separates the targets for libevent and libevent_pthreads,
and improves the messages.

Depends on D5339.

Test Plan

cmake -GNinja ..

Check the output prints both the event and event_pthread library
paths, as well as the include path.

ninja all check

Run the Gitian builds.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Mar 2 2020, 12:56

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 2 2020, 12:56

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9678: Diff 16680.Mar 2 2020, 13:07

Some stuff could be improve, but it looks good overall.

cmake/modules/FindEvent.cmake
56 ↗	(On Diff #16680)	There a a bunch of other modules: Core, Extra, OpenSSL and pthreads . Event by itself is Core+Extra.
83 ↗	(On Diff #16680)	Wouldn't that be preferable to ship this in its own file?
96 ↗	(On Diff #16680)	0.0.0 would be better, no? Because this will pass all check that verify version is X or more.
src/CMakeLists.txt
508 ↗	(On Diff #16680)	IMO this whole thing would just look better if a second call to find_package + target_link_libraries was issued in its own branch is the target isn't windows.
569 ↗	(On Diff #16680)	This drops the pthread linkage. Not sure if intentional or not, but I want to point it out.

This revision now requires changes to proceed.Mar 6 2020, 00:38

Fabien planned changes to this revision.Mar 6 2020, 08:10

Fabien added inline comments.

cmake/modules/FindEvent.cmake
56 ↗	(On Diff #16680)	Should we support all modules, even if they are not used in our code ? It is easy to add them, but that will remain unused/untested code.
96 ↗	(On Diff #16680)	This is intended to succeed by default. If there is no way to determine a version, the user should still be able to build on his system. This is a rare case that would only happen when cross compiling on a system with no pkg-config file available.
src/CMakeLists.txt
508 ↗	(On Diff #16680)	I changed this for the exact opposed reason, I found it looked better without having the duplicated find_package call ! I will update so you can compare both versions.
569 ↗	(On Diff #16680)	Yes it is. Looking at the code only server code has a need for event_pthreads.