Page MenuHomePhabricator

[DEPENDS] Add jemalloc to the depends
ClosedPublic

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

Details

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 10706
Build 19208: Default Diff Build & Tests
Build 19207: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.May 11 2020, 22:37
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
depends/README.md
98

Put it next to the other NO_FOOBAR

depends/packages/jemalloc.mk
8

--disable-libdl ?

This revision now requires changes to proceed.May 11 2020, 22:51
depends/packages/jemalloc.mk
8

From the release notes: Add configure option --disable-libdl to enable fully static builds. so that seems a good idea.

deadalnix requested changes to this revision.May 12 2020, 15:27

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 ↗(On Diff #19946)

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 ↗(On Diff #19946)

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
Fabien planned changes to this revision.May 13 2020, 07:15

I will split this diff

cmake/modules/FindJemalloc.cmake
44 ↗(On Diff #19946)

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 ↗(On Diff #19946)

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.

Split the FindJemalloc apart in D6055.

This revision is now accepted and ready to land.May 13 2020, 11:43