[DEPENDS] Add jemalloc to the depends
ClosedPublic
Actions

Authored by Fabien on May 11 2020, 22:37.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING89125792866d: [DEPENDS] Add jemalloc to the depends
rABC89125792866d: [DEPENDS] Add jemalloc to the depends

Summary

This allow to build jemalloc as a static dependency. It also fixes the
missing dependencies for a successful static build on Posix platforms.
Next step is to enable it by default and use it for our release build.

Test Plan

cd depends
make build-all

Cross build for all platforms with jemalloc enabled and run the
binaries (requires D6055).

Diff Detail

Repository

rABC Bitcoin ABC

Branch

depends_jemalloc

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 10712
Build 19218: Default Diff Build & Tests
Build 19217: arc lint + arc unit

Event Timeline

Fabien created this revision.May 11 2020, 22:37

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 11 2020, 22:37

Fabien requested review of this revision.May 11 2020, 22:37

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

deadalnix requested changes to this revision.May 11 2020, 22:51

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

cmake/modules/FindJemalloc.cmake
46 ↗	(On Diff #19932)	Threads::Threads See https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/leveldb/CMakeLists.txt$99-101
depends/README.md
98 ↗	(On Diff #19932)	Put it next to the other NO_FOOBAR
depends/packages/jemalloc.mk
8 ↗	(On Diff #19932)	--disable-libdl ?

This revision now requires changes to proceed.May 11 2020, 22:51

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10706: Diff 19932.May 11 2020, 22:51

Fabien added inline comments.May 12 2020, 07:39

depends/packages/jemalloc.mk
8 ↗	(On Diff #19932)	From the release notes: `Add configure option --disable-libdl to enable fully static builds.` so that seems a good idea.

Feedback

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) started.

Build Bitcoin-ABC / Diffs / Diff Testing (build-diff) passed.

Harbormaster completed remote builds in B10712: Diff 19946.May 12 2020, 07:51

Fabien added a child revision: D6042: Use jemalloc as a default.May 12 2020, 13:03

t seems like adding jemalloc to the depend is dragged down by poor integration into the build system. It is unclear to me why you packaged both together.

cmake/modules/FindJemalloc.cmake
44	IMO you might as well link it on windows as well, no? AFAIK, we will need the threading library on windows too to use jemalloc.
51	If we build the depends without dl, then we should not add it as a dependency.

This revision now requires changes to proceed.May 12 2020, 15:27

I will split this diff

cmake/modules/FindJemalloc.cmake
44	Windows don't need anything special like the `-pthreads` flag, so it's not strictly required, but it doesn't hurt either and make it a bit simpler. I'll change that.
51	This is OK for the version built from the depends, but not for other distributions that may have it as a dependency, such as debian. Since it's part of the libc I think it's fine to always link it, even if it is not used.