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 linking with minimap2 subproject #11

Closed
wants to merge 1 commit into from
Closed

Fix static linking with minimap2 subproject #11

wants to merge 1 commit into from

Conversation

jmarshall
Copy link
Contributor

Attempting to build pbmm2 statically fails with

[5/5] Linking target src/pbmm2.
FAILED: src/pbmm2 
[…]
duplicate symbol _ks_ksmall_heap in:
    src/25a6634@@pbmm2@exe/.._third-party_bam_sort.c.o
    subprojects/minimap2/libminimap2.a(map.c.o)
duplicate symbol _ks_heapmake_heap in:
    src/25a6634@@pbmm2@exe/.._third-party_bam_sort.c.o
    subprojects/minimap2/libminimap2.a(map.c.o)
ld: 2 duplicate symbols for architecture x86_64

This is because both source files use KSORT_INIT with the same name, namely heap.

https://github.com/lh3/minimap2/blob/1739a260fb044eded97b3271cd5204c79363fa76/map.c#L80

KSORT_INIT(heap, heap1_t, heap_lt)

Some klib facilities (e.g. KHASH_INIT2 and KHASH_DECLARE) allow code to specify static and/or extern to control how the generated klib functions might be shared with or hidden from other translation units. Others, like KSORT_INIT, do not and don't even use static, and the onus is on the programmer to ensure that the names are different and they don't clash. (As is usual for klib, this is under-documented…)

This patch shows an example of renaming one of them so they're different. With this, pbmm2 can be built with the non-system libraries linked statically:

$ otool -L src/pbmm2 
src/pbmm2:
	/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.8)
	/usr/lib/libbz2.1.0.dylib (compatibility version 1.0.0, current version 1.0.5)
	/usr/lib/liblzma.5.dylib (compatibility version 6.0.0, current version 6.3.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.60.2)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 307.5.0)

Of course a good citizen would leave HTSlib linked dynamically so it can be shared and updated as appropriate, but this would be useful for the other components that don't change independently.

The name given to KSORT_INIT determines the names of the functions
generated to implement the heap. Distinct heaps in different translation
units need different names in order for the translation units to be
able to be (static-)linked together.
@armintoepfer
Copy link
Member

You won't believe it, I just talked to @pb-dseifert if we want to solve the static linking issue before cutting v1.0.0. We'll have a look at your solution, merge it internally, and sync it back to github. Thank you for your patch!

@jmarshall
Copy link
Contributor Author

It appears this was merged as 5c82c99 via 6558442.

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