[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

Branch

cmake_find_event

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9758
Build 17395: Default Diff Build & Tests
Build 17394: arc lint + arc unit

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.