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

static builds link against musl #3343

Merged
merged 3 commits into from
Dec 10, 2015
Merged

static builds link against musl #3343

merged 3 commits into from
Dec 10, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Dec 7, 2015

This works around golang/go#13470.

Review on Reviewable

@tamird tamird changed the title link against musl instead of glibc in static builds link against musl instead of glibc Dec 7, 2015
@tbg
Copy link
Member

tbg commented Dec 7, 2015

I'm in favor of pursuing this more if we care strongly about static builds. We will need to investigate potential changes in performance and talk to the RocksDB maintainers about their experience using musl. golang/go#13470 has shown that static linkage of glibc into a project such as ours is just not something we should do.

@tamird
Copy link
Contributor Author

tamird commented Dec 7, 2015

Well, this is green. @mberhault: thoughts?

@mberhault
Copy link
Contributor

no idea. I haven't encountered that particular bug.
Are there any known issues when linking against musl? If not, I'm happy to see this merged and re-evaluated if we do encounter problems.

@tbg
Copy link
Member

tbg commented Dec 7, 2015

We definitely need to make sure we always use musl, not only in static
builds. After all, that's what we want to benchmark against and generally
it seems weird to use glibc most of the time but not in production. Isn't a
decision taken lightly as far as I'm concerned.

On Mon, Dec 7, 2015, 17:42 marc notifications@github.com wrote:

no idea. I haven't encountered that particular bug.
Are there any known issues when linking against musl? If not, I'm happy to
see this merged and re-evaluated if we do encounter problems.


Reply to this email directly or view it on GitHub
#3343 (comment)
.

@tamird
Copy link
Contributor Author

tamird commented Dec 7, 2015

@tschottdorf this patch does that, at least in the build docker container.

@mberhault looks like golang/go#13492 is the ony issue on the books right now, which seems very unlikely to affect us.

@tbg
Copy link
Member

tbg commented Dec 7, 2015

Ok, but that won't do it for linux users (make STATIC=1 release) or anything else. Not sure how important that is (will someone build their own static library?).
I didn't find anything on the internet mentioning RockDB and musl - maybe @igorcanadi can chime in here with regards to how big a change this is wrt RocksDB?

@igorcanadi
Copy link

Unfortunately I'm not very familiar with musl and I'm not aware of anybody using RocksDB with musl. Maybe ask at https://www.facebook.com/groups/rocksdb.dev/?

@bdarnell
Copy link
Contributor

bdarnell commented Dec 8, 2015

The discussion on the go/glibc bug makes it clear that statically linking libc is considered an exotic use case. So let's step back for a minute: why is it important for us to build fully-static (as opposed to mostly-static) binaries? The Go compiler itself is distributed as a binary which dynamically links libc. Why can't we do the same?

@tamird
Copy link
Contributor Author

tamird commented Dec 8, 2015

I'm not an authority on this, but perhaps it allows us to run on any system, not just systems with the same libc of the same version or higher.

The work was first done in a80fd74.

@tbg
Copy link
Member

tbg commented Dec 8, 2015

Also not an authority on this, but from my point of view we did it purely for the deployment story. I'm also starting to think that distributing binaries which link only glibc dynamically is fine since that's what everyone else is doing (and since linking it statically is a bad idea with glibc), and since venturing out into unknown territory may not pay off.

@tamird tamird changed the title link against musl instead of glibc static builds link against musl; stop using static builds Dec 9, 2015
@tamird
Copy link
Contributor Author

tamird commented Dec 9, 2015

I've added a commit here that discontinues the use of static builds. If someone does try to build a static binary, we'll try to use musl (since glibc is known bad), but usual workflows (read: tests on AWS) will use dynamic linking.

PTAL

cc @mberhault

rm -rf /tmp/*

ENV CC=/usr/local/musl/bin/musl-gcc
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to this file also need to be backed out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right. Though, only this line, not the "install musl" bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not strictly necessary to remove the "install musl" bit, but it's wasteful to leave it in.

@tamird
Copy link
Contributor Author

tamird commented Dec 9, 2015

After some additional digging, I've backed out the "stop statically linking" commit - see the latest commit message for details. Also, I've removed the netgo build tag, since there's no benefit to using netgo now that we're using musl which provides proper static linking.

This works around golang/go#13470 with the
biggest hammer I could find.

Another option is to go the route of dynamic linking, but this has
unfortunate implications for our `c-*` dependencies' builds; for
instance, in a dynamic build, c-rocksdb ends up linking against more
than just glibc, greatly increasing the surface of libraries required
to run cockroach. Having run such a build, the list of dependencies
ends up being quite large:

```
$ ldd ./cockroach
	linux-vdso.so.1 (0x00007fff60bcc000)
	librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f60d0fc9000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f60d0dac000)
	libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f60d0aa0000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f60d079f000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f60d0589000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f60d01df000)
	/lib64/ld-linux-x86-64.so.2 (0x00005620684d4000)
```

We could vigilantly produce static binaries upstream and lint agianst
excessive dynamic dependencies, but the musl route is a shorter path
to correctness.
@tamird tamird changed the title static builds link against musl; stop using static builds static builds link against musl Dec 9, 2015
@tbg
Copy link
Member

tbg commented Dec 9, 2015

Following this I was leaning towards leaving out musl for the sake of simplicity, but seeing that we link against a bunch of others it doesn't seem like such a bad idea any more. Or is it "straightforward" to link in most things statically, but not all of them?

@tamird
Copy link
Contributor Author

tamird commented Dec 9, 2015

@petermattis @bdarnell @mberhault would be good to have your opinions on this. The last commit message should provide useful context.

@petermattis
Copy link
Collaborator

Do we have any evidence that the list of "standard" dynamic libs that are in that commit message are problematic?

@tamird
Copy link
Contributor Author

tamird commented Dec 10, 2015

My possibly misguided expectation is that at least /lib/x86_64-linux-gnu/libgcc_s.so.1 is problematic since it requires a gcc-like compiler to be present.

@bdarnell
Copy link
Contributor

libgcc and libstdc++ look problematic: they're not present in a base tinycorelinux (i.e. boot2docker) image.

LGTM (this unblocks some of the ec2 testing work, right?). I think for our final releases we probably want to link libc dynamically, though, so we should file a separate issue to figure out how to link everything but libc statically.

@tbg
Copy link
Member

tbg commented Dec 10, 2015

Yeah, it avoids having weird workarounds to get the PGWire test running
there. Btw, if we ever want to run on busybox, we'll have to deal with
glibc.

On Wed, Dec 9, 2015 at 8:00 PM Ben Darnell notifications@github.com wrote:

libgcc and libstdc++ look problematic: they're not present in a base
tinycorelinux (i.e. boot2docker) image.

LGTM (this unblocks some of the ec2 testing work, right?). I think for our
final releases we probably want to link libc dynamically, though, so we
should file a separate issue to figure out how to link everything but libc
statically.


Reply to this email directly or view it on GitHub
#3343 (comment)
.

@tamird
Copy link
Contributor Author

tamird commented Dec 10, 2015

ec2 testing work is not currently blocked by this; the only test that reliably hits the original bug is TestPGWire, and that isn't being run in a static binary anywhere right now.

@bdarnell
Copy link
Contributor

I don't think we'll ever need to run on an environment as minimal as the busybox docker image (which is distinct from busybox itself). In addition to libc, we also need a zoneinfo database (and presumably enough of a support structure to keep it up to date, although I guess this could just be mounted in from the host machine).

@tamird
Copy link
Contributor Author

tamird commented Dec 10, 2015

I'm inclined to merge this and file a follow-up issue as @bdarnell suggests. I'll do that after lunch tomorrow to allow time for objections.

@tbg
Copy link
Member

tbg commented Dec 10, 2015

Still pro merging. But we should file a follow-up issue to figure out how to statically link everything but glibc. @bdarnell didn't find existing resources about that on the internet. Presumably this is an issue not only for Go, so there might still be something out there (or we'll have to reach out to individuals).

tamird added a commit that referenced this pull request Dec 10, 2015
static builds link against musl
@tamird tamird merged commit d350e7a into cockroachdb:master Dec 10, 2015
@tamird tamird deleted the static-stuff branch December 10, 2015 19:02
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.

6 participants