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

Fix static builds #305

Merged
merged 2 commits into from
Dec 12, 2016
Merged

Fix static builds #305

merged 2 commits into from
Dec 12, 2016

Conversation

gjedeer
Copy link

@gjedeer gjedeer commented Dec 7, 2016

I'm porting tuntox to work with TokTok c-toxcore and couldn't get it to link statically - dynamic linking works.

I'm not sure if the solution in this pull request is valid but it definitely makes the static linking work.


This change is Reviewable

@iphydf iphydf modified the milestones: v0.1.0, v0.1.1 Dec 10, 2016
@iphydf
Copy link
Member

iphydf commented Dec 10, 2016

I don't know if this is correct, either. What does Libs.private do?

@gjedeer
Copy link
Author

gjedeer commented Dec 12, 2016

From man pkg-config:

Libs.private:

This line should list any private libraries in use. Private libraries are libraries which are not exposed through your library, but are needed in the case of static linking. This differs from Requires.private in that it references libraries that do not have package files installed.

@GrayHatter
Copy link

@gjedeer I'd like to see the error output you're getting. Just to be sure this is our problem, and not how you're building.

That said, after reading this https://autotools.io/pkgconfig/dependencies.html#pkgconfig.static-link It makes me think that adding the same libs to Libs.Private just causes pkg-config to recursively search through the required libs to make sure that everything is supplied to the compiler. That leads me to two conclusions. 1) This shouldn't be harmful, and 2) if adding this to your build script fixes the issue; you're not including the correct deps for tuntox yourself.

+@GrayHatter


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@GrayHatter GrayHatter self-assigned this Dec 12, 2016
@gjedeer
Copy link
Author

gjedeer commented Dec 12, 2016

The Makefile is as simple as they come.

With the Libs.private change, the linker is executed like this:

gcc -o tuntox client.o log.o gitversion.o mach.o main.o util.o -lpthread -g -pthread -lm -static -lrt -L/usr/local/lib -lpthread -lrt -ltoxcrypto -ltoxnetwork -ltoxdht -ltoxnetcrypto -ltoxfriends -ltoxmessenger -ltoxgroup -ltoxcore -lpthread -lrt -ltoxcrypto -ltoxnetwork -ltoxdht -ltoxnetcrypto -ltoxfriends -ltoxmessenger -ltoxgroup -ltoxcore -lsodium

and pkg-config outputs as following:

➜  ~ pkg-config --cflags libsodium toxcore       
-I/usr/local/include
➜  ~ pkg-config --static --libs libsodium toxcore
-L/usr/local/lib -lpthread -lrt -ltoxcrypto -ltoxnetwork -ltoxdht -ltoxnetcrypto -ltoxfriends -ltoxmessenger -ltoxgroup -ltoxcore -lpthread -lrt -ltoxcrypto -ltoxnetwork -ltoxdht -ltoxnetcrypto -ltoxfriends -ltoxmessenger -ltoxgroup -ltoxcore -lsodium

Without Libs.private:

gcc -o tuntox client.o log.o gitversion.o mach.o main.o util.o -lpthread -g -pthread -lm -static -lrt -L/usr/local/lib -lpthread -lrt -ltoxcrypto -ltoxnetwork -ltoxdht -ltoxnetcrypto -ltoxfriends -ltoxmessenger -ltoxgroup -ltoxcore -lsodium

and pkg-config outputs as following:

➜  ~ pkg-config --cflags libsodium toxcore
-I/usr/local/include
➜  ~ pkg-config --static --libs libsodium toxcore
-L/usr/local/lib -lpthread -lrt -ltoxcrypto -ltoxnetwork -ltoxdht -ltoxnetcrypto -ltoxfriends -ltoxmessenger -ltoxgroup -ltoxcore -lsodium

The former links correctly, the latter yields a stream of errors starting with:

/usr/local/lib/libtoxcore.a(tox.c.o): In function `tox_new':
tox.c:(.text+0x63d): undefined reference to `ip_init'
tox.c:(.text+0x669): undefined reference to `addr_resolve_or_parse_ip'
tox.c:(.text+0x6b1): undefined reference to `new_messenger'
tox.c:(.text+0x6c1): undefined reference to `new_groupchats'
tox.c:(.text+0x6d2): undefined reference to `kill_messenger'
tox.c:(.text+0x74a): undefined reference to `messenger_load'
tox.c:(.text+0x783): undefined reference to `load_secret_key'
/usr/local/lib/libtoxcore.a(tox.c.o): In function `tox_kill':
tox.c:(.text+0x7ef): undefined reference to `kill_groupchats'
tox.c:(.text+0x7fb): undefined reference to `kill_messenger'
......

I have no idea what makes my change work with --static, it's the same output but all -l flags are duplicated.

@gjedeer
Copy link
Author

gjedeer commented Dec 12, 2016

Can it be that the order of -l flags is wrong and that's why duplication resolves the build (but if that's the case, why does the dynamic build work?)

@gjedeer
Copy link
Author

gjedeer commented Dec 12, 2016

The typical UNIX linker works from left to right, so put all your dependent libraries on the left, and the ones that satisfy those dependencies on the right of the link line. You may find that some libraries depend on others while at the same time other libraries depend on them. This is where it gets complicated.

sauce

@gjedeer
Copy link
Author

gjedeer commented Dec 12, 2016

Sorry for flooding but I resolved this in a much simpler way: moving -ltoxcore to the front. Updated PR accordingly.

@iphydf
Copy link
Member

iphydf commented Dec 12, 2016

:lgtm_strong:

Can you rebase this on master?


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@gjedeer
Copy link
Author

gjedeer commented Dec 12, 2016

Done

@iphydf iphydf assigned iphydf and unassigned nurupo and GrayHatter Dec 12, 2016
@iphydf iphydf merged commit ba476e8 into TokTok:master Dec 12, 2016
@iphydf iphydf modified the milestones: v0.1.0, v0.1.1 Dec 12, 2016
@nurupo
Copy link
Member

nurupo commented Dec 14, 2016

:lgtm_strong:


Reviewed 2 of 2 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

This pull request was closed.
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.

4 participants