Skip to content

build(jemalloc): Install Jemalloc if missing via Makefile. #6463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Sep 16, 2020

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Sep 16, 2020

This PR adds a jemalloc target to makefile to install jemalloc. The make install command will install jemalloc if it's not already installed.

The make jemalloc target currently fails on docker with command sudo not found. I don't have a solution for it.

The change doesn't work on macOS or Windows. Dgraph builds with jemalloc by default on Linux only.


This change is Reviewable

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim, @manishrjain, and @vvbalaji-dgraph)


dgraph/Makefile, line 98 at r1 (raw file):

ldconfig ; \

Does this need sudo too? I get this error locally, and it happens in CI too.

$ ldconfig                   
/sbin/ldconfig.real: Can't create temporary cache file /etc/ld.so.cache~: Permission denied

Do we need to run ldconfig? Isn't sudo make install sufficient?

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jarifibrahim, @manishrjain, and @vvbalaji-dgraph)


dgraph/Makefile, line 66 at r1 (raw file):

ldconfig -p

We're using the static library (libjemalloc.a), not the shared library (libjemalloc.so), so we shouldn't need to use ldconfig, as it doesn't tell us about the .a file:

$ ldconfig -p | grep jemalloc
	libjemalloc.so.2 (libc6,x86-64) => /usr/local/lib/libjemalloc.so.2
	libjemalloc.so (libc6,x86-64) => /usr/local/lib/libjemalloc.so

We should check if the path to the archive exists: /usr/local/lib/libjemalloc.a

@danielmai danielmai changed the title feat(makefile): Add jemalloc target build(jemalloc): Install Jemalloc if missing via Makefile. Sep 16, 2020
@danielmai danielmai merged commit 02b5118 into master Sep 16, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/makefile branch September 18, 2020 13:09
danielmai added a commit that referenced this pull request Sep 18, 2020
This PR adds a jemalloc target to makefile to install jemalloc. The make install command will install jemalloc if it's not already installed.

The make jemalloc target currently fails on docker with command sudo not found. I don't have a solution for it.

The change doesn't work on macOS or Windows. Dgraph builds with jemalloc by default on Linux only.

Co-authored-by: Daniel Mai <daniel@dgraph.io>
(cherry picked from commit 02b5118)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants