search

Bioconductor / Rhtslib

HTSlib high-throughput sequencing library as an R package
https://bioconductor.org/packages/Rhtslib
11 stars 12 forks source link

Linking to Rhtslib when system version also installed #5

Closed grimbough closed 5 years ago

grimbough commented 5 years ago

Describing what I think's happening in https://support.bioconductor.org/p/120529 Hopefully this is helpful going forward.

I could reproduce the errors the other users are seeing after installing a system level version of htslib via apt-get install libhts-dev. It also only manifests if I compile with gcc, my version of R compiled with clang is fine regardless.

I think the problem is that -Wl,-rpath,/Rhtslib/dir is not working quite as expected with some combo of gcc/Ubuntu/...

If I check the header of the current version of Rsamtools.so for the RPATH I see this:

readelf -d Rsamtools/src/Rsamtools.so | head -n 11

Dynamic section at offset 0x53c80 contains 31 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libhts.so.2]
 0x0000000000000001 (NEEDED)             Shared library: [libz.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libpthread.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libR.so]
 0x0000000000000001 (NEEDED)             Shared library: [libstdc++.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000000f (RUNPATH)            Library rpath: [/home/mike/R/x86_64-pc-linux-gnu-library/3.6/Rhtslib/usrlib]

Note, that it's RUNPATH rather than RPATH. There's quite a few discussion about the difference between RPATH and RUNPATH (this write up works for me) but I think the pertinent info is:

If we check the LD_LIBRARY_PATH within R I get:

> Sys.getenv("LD_LIBRARY_PATH")
[1] "/usr/lib/R/lib:/usr/lib/x86_64-linux-gnu:/usr/lib/jvm/default-java/lib/server"

and for me I have a libhts.so in /usr/lib/x86_64-linux-gnu which is presumably taking precedence but was not the version linked against during installation.

One way to restore the behaviour or really setting the RPATH is to use the -Wl,--disable-new-dtags which is what this pull request does. Presumably there's a reason this behaviour was changed, so maybe it would be preferable to link statically?

hpages commented 5 years ago

Excellent Mike. I'll take a look after breakfast ;-) Thanks for helping with this.

grimbough commented 5 years ago

No problem. I love trying to track down these weird compiler issues, and this seemed to be hitting a lot of users so it seemed like time well spent. Makes me realise how few people actually use the devel branch that this didn't seem to have cropped up before, in my head everyone's on devel!

Just shout if you'd like me to test anything on my system.

hpages commented 5 years ago

Also what's interesting is that other packages like scPipe, csaw etc... that get linked to Rhtslib have possibly been linked to the wrong libhts.so on the user machine for years. Never made a difference so far because they don't use the extra symbols (e.g. faidx_fetch_seq2) that I added recently to Rhtslib. Using static linking like you do with Rhdf5lib would totally eliminate this type of issue. I wonder if I shouldn't just do that with Rhtslib too. Also are we sure that linker option --disable-new-dtags is safe across all non-Mac-or-Windows platforms? I don't see any other BioC package using it. What do you think?

hpages commented 5 years ago

Realize now that I missed your "so maybe it would be preferable to link statically?"

So yes, let's do that. Thanks!

grimbough commented 5 years ago

I've never seen this before, so I don't know when the behaviour of rpath changed, but reading around it varies across different platforms, so maybe it's been fine for most users and Ubuntu changed 'recently'.

Static linking seems the safest to me, I wasn't sure if you had a strong reason for treating Linux & Mac differently here.

hpages commented 5 years ago

This choice was made a long time ago by someone else and I don't know the reason. I'm changing this to static linking, like on Mac. Still working on it...

lawremi commented 5 years ago

I think we should recommend static linking as best practice in all such cases. rpath is evil.

On Fri, May 3, 2019 at 9:22 AM hpages notifications@github.com wrote:

This choice was made a long time ago by someone else and I don't know the reason. I'm changing this to static linking, like on Mac. Still working on it...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Rhtslib/pull/5#issuecomment-489155690, or mute the thread https://github.com/notifications/unsubscribe-auth/AABGT3SXK7QATLUZHQHF6I3PTRRDPANCNFSM4HKUIP4A .

hpages commented 5 years ago

Done (commit db1d8e17ef5b8568fdae3fae0dc701fe2250c952). Can you guys test this on your machines? Thanks! Still need to apply to the RELEASE_3_9 branch. I'm closing the PR.