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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 10706
Build 19208: Default Diff Build & Tests
Build 19207: 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
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
Fabien added inline comments.May 12 2020, 07:39
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.

Fabien updated this revision to Diff 19946.May 12 2020, 07:39

Feedback

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.

Fabien edited the test plan for this revision. (Show Details)May 13 2020, 07:38
Fabien updated this revision to Diff 19979.May 13 2020, 07:39

Split the FindJemalloc apart in D6055.

deadalnix accepted this revision.May 13 2020, 11:43
This revision is now accepted and ready to land.May 13 2020, 11:43
This revision was automatically updated to reflect the committed changes.