Skip to content
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

Rewrite build system for new versions and Heroku stacks #14

Closed
wants to merge 1 commit into from

Conversation

gaffneyc
Copy link
Collaborator

@gaffneyc gaffneyc commented Dec 5, 2017

This allows new versions to be built for uploading by running
make VERSION=5.0.1 rather than updating the hard coded version in
multiple places. I've also added support for building against either the
legacy cedar-14 architecture or heroku-16. While versions built on
cedar-14 appear to run on heroku-16, the newer stack should also mean a
newer and better tool chain.

I've removed the Dockerfile and switched to using Heroku's docker images
directly as this removes the need for cleaning them up afterwards. It
possibly makes debugging harder but having build.sh available should
avoid some of that.

Fixes #2
Fixes #7
Fixes #13

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 5, 2017

This is the first part of some work I've been doing today. Started as a review of what was here and... kind of spiraled. I'm also looking at making changes to bin/compile but wanted to discuss them before getting too deep into it.

The goal here was to make it easy to create builds for new versions of jemalloc. The next step that I'm working on is being able to use those builds and easily switch between them. From a high level this codebase is two things: heroku buildpack for downloading compiled binaries and tooling to build those binaries.

It should be possible to quickly and easily make binaries available based on version without needing to update the buildpack. I'm thinking we can use a set of releases (one per stack) that hosts the binaries for each version of jemalloc (I have a proof of concept on my fork).

For switching between versions, I've been able to have a JEMALLOC_VERSION env variable that will compile different versions when a new slug is compiled. I am also considering a JEMALLOC_ENABLED flag that will automate the process of setting LD_PRELOAD.

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 5, 2017

In bin/compile is it necessary to set LD_LIBRARY_PATH, LIBRARY_PATH, PKG_CONFIG, CPPPATH, and CPATH? I believe those are only necessary if someone plans to link against jemalloc but only LD_PRELOAD is required for using the library. Removing them from bin/compile would reduce the file quite a bit.

This allows new versions to be built for uploading by running
`make VERSION=5.0.1` rather than updating the hard coded version in
multiple places. I've also added support for building against either the
legacy cedar-14 architecture or heroku-16. While versions built on
cedar-14 appear to run on heroku-16, the newer stack should also mean a
newer and better tool chain.

I've removed the Dockerfile and switched to using Heroku's docker images
directly as this removes the need for cleaning them up afterwards. It
possibly makes debugging harder but having build.sh available should
avoid some of that.

Fixes #2
Fixes #7
Fixes #13
@csuhta
Copy link

csuhta commented Dec 5, 2017

👍 on the environment variables to control the build and if jemalloc should be used. That makes testing very easy too.

Using jemalloc.sh is fairly awkward right now.

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 5, 2017

I've fleshed out what I was thinking and it's sitting on master of my fork. We're slowly testing it on a couple apps and already have it running on Dead Man's Snitch.

Going to clean up some of the fork specific changes and get another PR up for consideration.

@csuhta
Copy link

csuhta commented Dec 8, 2017

@gaffneyc I was trying out your folk, and I noticed the builds for jemalloc 3.6.0 do not include a jemalloc-config script in the archive

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 8, 2017

@csuhta Interesting, I will take a look. I'm not modifying the sources at all so I'm guessing it's not part of the 3.x line. I'll see about getting a work around in place.

Going to take some time today to upstream my changes as well.

@csuhta
Copy link

csuhta commented Dec 8, 2017

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 8, 2017

Was there a config change you needed to make for it to be generated or did you add the file directly?

@csuhta
Copy link

csuhta commented Dec 8, 2017

I manually created them

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 8, 2017

Gotcha. It looks like jemalloc-config was added in 4.0.0. Is it worth support any versions older than 4.0.0 other than 3.6.0?

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 8, 2017

It may be possible to hardcode it as /app/vendor/jemalloc/lib/libjemalloc.so as it's a symlink to the specific revision. Since slugs are compiled independently on each code change, there should only ever be one version installed in /app/vendor/jemalloc

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 8, 2017

@csuhta I've pushed gaffneyc@6564f30 which should fix the issue with 3.6.0. I was able to remove the need for jemalloc-config entirely.


.PHONY: cedar-14 heroku-16

# Build for cedar stack (deprecated)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cedar-14 isn't deprecated, can just remove this comment

@nateberkopec
Copy link
Collaborator

Thank you so much for this.

I am also considering a JEMALLOC_ENABLED flag that will automate the process of setting LD_PRELOAD.

I like this, but can leave for another day/PR.

@nateberkopec
Copy link
Collaborator

Maybe also for a different PR, but to use any of this properly, we'll have to change the way this line works in bin/compile, correct?

@gaffneyc
Copy link
Collaborator Author

gaffneyc commented Dec 11, 2017

I've been planning to create a PR for what's on the master branch of my fork but have been pressed for time this last week. Thinking that PR would supersede this one and probably #11.

@gaffneyc
Copy link
Collaborator Author

@gaffneyc
Copy link
Collaborator Author

Took a couple minutes and submitted #18 which supersedes this one.

@gaffneyc gaffneyc closed this Dec 11, 2017
@gaffneyc gaffneyc deleted the jemalloc-5.0.0 branch December 11, 2017 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants