Bump sqlite3 #16094
Merged
Bump sqlite3 #16094
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TL;DR: stop building static binary that may fail Linker flag --unresolved-symbols=ignore-in-shared-libs was added in commit 06d0843 two years ago for the static build case, presumably to avoid dealing with problem of missing libraries. For the record, this is what ld(1) man page says: > --unresolved-symbols=method > Determine how to handle unresolved symbols. There are four > possible values for method: > ......... > ignore-in-shared-libs > Report unresolved symbols that come from regular object files, > but ignore them if they come from shared libraries. This can > be useful when creating a dynamic binary and it is known that > all the shared libraries that it should be referencing are > included on the linker's command line. Here, the flag is not used for its purpose ("creating a dynamic binary") and does more harm than good. Instead of complaining about missing symbols as it should do if some libraries are missing from LIBS/LDFLAGS, it lets ld create a binary with unresolved symbols, ike this: $ readelf -s bundles/1.7.1/binary/docker-1.7.1 | grep -w UND ........ 21029: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND dlopen ......... Such binary is working just fine -- until code calls one of those functions, then it crashes (for apparently no reason, i.e. it is impossible to tell why from the diagnistics printed). In other words, adding this flag allows to build a static binary with missing libraries, hiding the problem from both a developer (who forgot to add a library to #cgo: LDFLAGS -- I was one such developer a few days ago when I was working on ploop graphdriver) and from a user (who expects the binary to work without crashing, and it does that until the code calls a function in one of those libraries). Removing the flag immediately unveils the problem (as it should): /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libsqlite3.a(sqlite3.o): In function `unixDlError': (.text+0x20971): undefined reference to `dlerror' /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libsqlite3.a(sqlite3.o): In function `unixDlClose': (.text+0x8814): undefined reference to `dlclose' The problem is, gosqlite package says: #cgo LDFLAGS: -lsqlite3 which is enough for dynamic linking, as indirect dependencies (i.e. libraries required by libsqlite3.so) are listed in .so file and will be resolved dynamically by ldd upon executing the binary. For static linking though, one has to list all the required libraries, both direct and indirect. For libraries with pkgconfig support the list of required libraries can be obtained with pkg-config: $ pkg-config --libs sqlite3 # dynamic linking case -lsqlite3 $ pkg-config --libs --static sqlite3 # static case -lsqlite3 -ldl -lpthread It seems that all one has to do is to fix gosqlite this way: -#cgo LDFLAGS: -lsqlite3 +#cgo pkg-config: sqlite3 Unfortunately, cmd/go doesn't know that it needs to pass --static flag to pkg-config in case of static linking (see golang/go#12058). So, for one, one has to do one of these things: 1. Patch sqlite.go like this: -#cgo LDFLAGS: -lsqlite3 +#cgo pkg-config: --static sqlite3 (this is exactly what I do in goploop, see kolyshkin/goploop@e9aa072f51) 2. Patch sqlite.go like this: -#cgo LDFLAGS: -lsqlite3 +#cgo LDFLAGS: -lsqlite3 -ldl -lpthread (I would submit this patch to gosqlite but it seems that https://code.google.com/p/gosqlite/ is deserted and not maintained, and patching it here is not right as it is "vendored") 3. Explicitly add -ldl for the static link case. This is what this patch does. 4. Fork sqlite to github and maintain it there. Personally I am not ready for that, as I'm neither a Go expert nor gosqlite user. Now, #3 doesn't look like a clear solution, but nevertheless it makes the build much better than it was before. Signed-off-by: Kir Kolyshkin <kir@openvz.org>
|
ping @tianon @LK4D4 @aaronlehmann |
|
Nice, LGTM 👍 |
|
@jfrazelle I think |
| @@ -13,7 +13,7 @@ clone git github.com/go-check/check 64131543e7896d5bcc6bd5a76287eb75ea96c673 | |||
| clone git github.com/gorilla/context 14f550f51a | |||
| clone git github.com/gorilla/mux e444e69cbd | |||
| clone git github.com/kr/pty 5cf931ef8f | |||
| clone git github.com/mattn/go-sqlite3 b4142c444a8941d0d92b0b7103a24df9cd815e42 | |||
| clone git github.com/mattn/go-sqlite3 3b3f1d01b2696af5501697c35629048c227586ab | |||
There was a problem hiding this comment.
Mind switching this to the updated version tag @mattn created for us (v1.1.0; mattn/go-sqlite3#235 (comment))? ❤️ 😍
|
FWIW, I just opened moby/libnetwork#499 so we don't vendor test specific functions anymore. |
|
cool ya because the vendoring script was adding that file |
Signed-off-by: Jessica Frazelle <acidburn@docker.com>
jessfraz
force-pushed
the
bump-sqlite3
branch
from
67c015b
to
46df9e4
Compare
|
updated |
|
🤘 LGTM |
|
LGTM |
|
🤘 |
kolyshkin
added a commit
to kolyshkin/moby
that referenced
this pull request
Sep 9, 2015
1. Flag -ldl was added by me in commit 15aad5d. No one (including me) noticed that since sqlite3 is now being built with OMIT_LOAD_EXTENTION (see moby#16094 and commit 46df9e4, as well as mattn/go-sqlite3#235) we no longer need -ldl. 2. Flag -lpthread was added by @crosbymichael in commit a263e07, I can't figure out why. I tried rebuilding with this flag removed and it's working just fine, so let's assume the flag is no longer needed. PS ideally these should be two separate commits, but let's go wild! Cc: Michael Crosby <crosbymichael@gmail.com> Signed-off-by: Kir Kolyshkin <kir@openvz.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the dlopen error when compiling statically
I also included @kolyshkin commit from #15387 to show we can not ignore unresoved symbols and get none now :)