Bioconductor / Rhtslib

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

Properly link libhts deps #35

Closed jeroen closed 10 months ago

jeroen commented 10 months ago

libhts.a is calling several external libraries without properly linking to them. This leads to errors when cross compiling. See for example: https://github.com/r-universe/bioconductor/actions/runs/7490289085/job/20389414448

This fixes it.

hpages commented 10 months ago

Thanks @jeroen! I'm curious though why this only shows up when cross compiling and not when compiling natively like here. Here is the linking command we use on our arm Mac machine:

clang -arch arm64 -dynamiclib -Wl,-headerpad_max_install_names -undefined dynamic_lookup -L/Library/Frameworks/R.framework/Resources/lib -L/opt/R/arm64/lib -o Rhtslib.so R_init_Rhtslib.o /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Rhtslib/usrlib/libhts.a -lcurl -F/Library/Frameworks/R.framework/.. -framework R -Wl,-framework -Wl,CoreFoundation

I suspect that it works for us because we use -undefined dynamic_lookup. If I remove this, I actually get the same error than you.

Is the reason you're not using -undefined dynamic_lookup because you're cross compiling? Sorry if this is obvious but I'm lacking experience with cross compilation.

Thanks, H.

jeroen commented 10 months ago

Is the reason you're not using -undefined dynamic_lookup because you're cross compiling?

Yes exactly. We do this because when we cross compile we have to build with --no-test-load. I also tried to explain it here: https://stat.ethz.ch/pipermail/r-sig-mac/2024-January/014912.html

hpages commented 10 months ago

Thanks for the link, that helps a lot, but maybe I'm still missing a couple of things.

First I'm still not clear why we don't see this linking error on Linux. Here is the linking command we use there:

gcc -shared -L/home/biocbuild/bbs-3.19-bioc/R/lib -L/usr/local/lib -o Rhtslib.so R_init_Rhtslib.o /home/biocbuild/bbs-3.19-bioc/R/site-library/Rhtslib/usrlib/libhts.a -lcurl -L/home/biocbuild/bbs-3.19-bioc/R/lib -lR

As you can see we're not using -undefined dynamic_lookup there but everything works fine nonetheless.

Also I'm not sure why things are different for -lcurl vs -lbz2 -llzma -lz. Removing -lcurl from the linking command causes the "test-load" step to fail on both Linux and Mac. Is it because the libbz2, liblzma and libz symbols are already preloaded in the process via base R, but not the libcurl symbol, as you seem to suggest in your post on R-SIG-Mac?

So I wrote this little C program to try to dlopen() Rhtslib.so myself (following a suggestion from the Known Issues from the macOS Big Sur 11.0.1 Release Notes):

dlopen.c:

#include <stdio.h>
#include <dlfcn.h>

int main(int n, const char ** args) {
  if (n != 2) {
    printf("You must specify the path to a mach-o file.\n");
    return -2;
  }
  printf("Trying dlopen(%s,RTLD_NOW) ...\n", args[1]);
  if (dlopen(args[1], RTLD_NOW) == NULL) {
    printf("%s\n", dlerror());
    return -1;
  }
  printf("OK\n");
  return 0;
}

Running it on the Rhtslib.so file that we produce on kjohnson1 returns OK:

kjohnson1:sandbox biocbuild$ clang dlopen.c -o dlopen
kjohnson1:sandbox biocbuild$ ./dlopen /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Rhtslib/libs/Rhtslib.so
Trying dlopen(/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/Rhtslib/libs/Rhtslib.so,RTLD_NOW) ...
OK

so I'm still not convinced that there's anything "wrong" with this shared library.

Do you have any insight on that?

H.

jeroen commented 10 months ago

Also I'm not sure why things are different for -lcurl vs -lbz2 -llzma -lz. Removing -lcurl from the linking command causes the "test-load" step to fail on both Linux and Mac. Is it because the libbz2, liblzma and libz symbols are already preloaded in the process via base R, but not the libcurl symbol, as you seem to suggest in your post on R-SIG-Mac?

I think what happens in this case is that the macos build of libcurl also links to libbz2 and liblzma and hence they get loaded automatically when libcurl is loaded. So therefore "just" linking to libcurl happens to work, but your package would break if a future version of libcurl does not longer load those libs for you.

so I'm still not convinced that there's anything "wrong" with this shared library.

Well it depends on what you mean by wrong, but if we look at the list of deps with otool -L

# otool -L Rhtslib.so
Rhtslib.so:
    Rhtslib.so (compatibility version 0.0.0, current version 0.0.0)
    /usr/lib/libcurl.4.dylib (compatibility version 7.0.0, current version 9.0.0)
    /Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/lib/libR.dylib (compatibility version 4.3.0, current version 4.3.1)
    /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1775.118.101)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.100.5)

The dependency on libbz2 and liblzma is not declared, even though your package certainly calls it. So you are relying on either libcurl.4.dylib or libR.dylib to load the remaining symbols for you.

But so the entire problem with cross compiling is that we cannot test-load the binary we just compiled, because it is built for another architecture than the one we are running. So we can't really test if your package "happens to work" with incomplete linker flags, because the libs you are using are indirectly loaded some other way.

hpages commented 10 months ago

I think what happens in this case is that the macos build of libcurl also links to libbz2 and liblzma and hence they get loaded automatically when libcurl is loaded.

Interesting. I did the following experiment to convince myself:

test.c:

#include <stdio.h>
#include <bzlib.h>

int main() {
    printf("BZ2_bzlibVersion(): %s\n", BZ2_bzlibVersion());
    return 0;
}

Then:

kjohnson1:sandbox biocbuild$ clang test.c -o test
ld: Undefined symbols:
  _BZ2_bzlibVersion, referenced from:
      _main in test-8cc991.o
clang: error: linker command failed with exit code 1 (use -v to see invocation)

kjohnson1:sandbox biocbuild$ clang test.c -o test -lcurl
ld: Undefined symbols:
  _BZ2_bzlibVersion, referenced from:
      _main in test-7e20b3.o
clang: error: linker command failed with exit code 1 (use -v to see invocation)

kjohnson1:sandbox biocbuild$ clang test.c -o test -undefined dynamic_lookup -lcurl
kjohnson1:sandbox biocbuild$ ./test 
BZ2_bzlibVersion(): 1.0.8, 13-Jul-2019

kjohnson1:sandbox biocbuild$ clang test.c -o test -undefined dynamic_lookup
kjohnson1:sandbox biocbuild$ ./test 
dyld[60515]: missing symbol called
Abort trap: 6

Alright. Thanks for the explanation!

H.