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

Unbundle htslib #1586

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

SoapGentoo
Copy link
Contributor

Hi @alexdobin
in Gentoo, we unbundle libraries as a general principle. The htslib with STAR is very old, and using the htslib we provide helps users get the benefit of more recent htslib releases. All the changes here maintain the default workflow, so your workflow should not be impacted, we can just provide SYSTEM_HTSLIB=1 to make when building STAR downstream to avoid bundling htslib.

@SoapGentoo
Copy link
Contributor Author

@alexdobin any chance of getting this merged?

@alexdobin
Copy link
Owner

Hi David,

this is a great fix and long overdue.
Have you tested it with different BAM output options?
I will have to test it thoroughly, will try to get it done next week.

@SoapGentoo
Copy link
Contributor Author

I only tested it in Gentoo with the default options, and it worked fine there.

@alexdobin
Copy link
Owner

By default, BAM is not output and thus htslib is not used.

@SoapGentoo
Copy link
Contributor Author

@alexdobin do you have some example data I could test it on?

@mjsteinbaugh
Copy link

@SoapGentoo I'm happy to help test this and help merge into STAR if possible

@SoapGentoo
Copy link
Contributor Author

@mjsteinbaugh I just didnt get around to testing the runtime, could you try it with input data and diff the output? It should be identical

@mjsteinbaugh
Copy link

@SoapGentoo Yeah I can try applying these changes as a patch to the source code, will try it out and get back to you. Thanks for this!

@mjsteinbaugh
Copy link

For reference, there's a patch in bioconda that serves to unbundle htslib:
https://github.com/bioconda/bioconda-recipes/blob/master/recipes/star/patches/0002-donotuse_own_htslib.patch

@SoapGentoo
Copy link
Contributor Author

yup, I just find mine more concise and cleaner 😉

* $(LDFLAGS) needs to come very early in the linker line, since
  its arguments only affect trailing object files.
@mjsteinbaugh
Copy link

mjsteinbaugh commented Oct 11, 2023

@SoapGentoo This is building for me on Ubuntu 22 with GCC. I'm also trying to get this to work with clang 15 on macOS Sonoma, to no avail. It seems like there needs to be some different compiler settings. Would it be possible to help test on macOS and add support for clang?

My current build script is here, for reference:
https://github.com/acidgenomics/koopa/blob/main/lang/bash/include/install/common/shared/star.sh

@SoapGentoo
Copy link
Contributor Author

what issues are you seeing?

@mjsteinbaugh
Copy link

mjsteinbaugh commented Oct 11, 2023

Here's what I'm seeing on macOS Sonoma x86_64; Xcode CLT 15.0.0.0.1.1694021235; clang 15:

./IncludeDefine.h:33:25: note: expanded from macro 'SAMTOOLS_BGZF_H'
#define SAMTOOLS_BGZF_H <htslib/bgzf.h>
                        ^~~~~~~~~~~~~~~
<scratch space>:138:1: note: expanded from here
<htslib/bgzf.h>
^~~~~~~~~~~~~~~
1 error generated.
In file included from bam_cat.c:49:
./bam_cat.h:4:10: fatal error: 'htslib/sam.h' file not found
#include <htslib/sam.h>
         ^~~~~~~~~~~~~~
1 error generated.
Makefile:151: Depend.list: No such file or directory
gmake: *** [Makefile:150: Depend.list] Error 1

@SoapGentoo
Copy link
Contributor Author

you need to show me the compile line

@mjsteinbaugh
Copy link

mjsteinbaugh commented Oct 11, 2023

make args: --jobs=8 CXX=/usr/bin/clang++ SYSTEM_HTSLIB=1 VERBOSE=1 STARforMacStatic STARlongForMacStatic

declare -x CPPFLAGS=" -I/opt/koopa/app/xz/5.4.4/include -I/opt/koopa/app/zlib/1.3/include -I/opt/koopa/app/htslib/1.18/include -I/opt/koopa/app/zlib/1.3/include -I/opt/koopa/app/xz/5.4.4/include"
declare -x LDFLAGS=" -L/opt/koopa/app/xz/5.4.4/lib -Wl,-rpath,/opt/koopa/app/xz/5.4.4/lib -L/opt/koopa/app/zlib/1.3/lib -Wl,-rpath,/opt/koopa/app/zlib/1.3/lib -L/opt/koopa/app/htslib/1.18/lib -Wl,-rpath,/opt/koopa/app/htslib/1.18/lib"
declare -x LDLIBS=" -llzma -lz -lhts"
declare -x LIBRARY_PATH="/opt/koopa/app/htslib/1.18/lib:/opt/koopa/app/zlib/1.3/lib:/opt/koopa/app/xz/5.4.4/lib:/usr/lib"
declare -x PKG_CONFIG_PATH="/opt/koopa/app/htslib/1.18/lib/pkgconfig:/opt/koopa/app/zlib/1.3/lib/pkgconfig:/opt/koopa/app/xz/5.4.4/lib/pkgconfig"

And stdout and stderr logs are attached:
stderr.log
stdout.log

@SoapGentoo
Copy link
Contributor Author

SoapGentoo commented Oct 11, 2023

$ PKG_CONFIG="pkg-config --static" PKG_CONFIG_PATH=/Users/soap/Desktop/prefix/lib/pkgconfig/ \
  make CC=gcc-mp-12 CXX=g++-mp-12 SYSTEM_HTSLIB=1 \
  VERBOSE=1 LDFLAGS=" -static-libstdc++ -static-libgcc -Wl,-ld_classic" -j8 STARlongForMacStatic

the -Wl,-ld_classic is necessary since Xcode 15 lld seems to have a bug. I get

$ otool -L STARlong
STARlong:
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.100.3)

In general, I don't believe there should be static targets in a makefile, since static linking requires intricate toolchain knowledge and trying to make it work for all cases is a lost cause. Also the zlib and htslib in both cases are not from Macports but self-compiled (in order to disable the xz-utils dep).

@mjsteinbaugh
Copy link

mjsteinbaugh commented Oct 11, 2023

Where's gcc-mp-12 / g++-mp-12 from? Can we use clang and clang++ instead?

Update: mp = MacPorts here

@SoapGentoo
Copy link
Contributor Author

I'm using GCC from Macports, since you can't link libc++ statically, only libstdc++. You can use clang too, but you will always have a dependency on libc++, which means you need to compile it on an oldest version of macOS that you want to support.

@mjsteinbaugh
Copy link

With clang, I keep hitting the missing header issue:

./bam_cat.h:4:10: fatal error: 'htslib/sam.h' file not found
#include <htslib/sam.h>

Not sure how to fix this easily

@SoapGentoo
Copy link
Contributor Author

Not using pkg-config and trying to second guess all the include flags is your first issue. I've just tried using Macports Clang (since Apple's Clang doesn't support OpenMP) and it worked just as well.

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.

None yet

3 participants