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

Musl support for htslib 1.10.2 #193

Merged
merged 66 commits into from
Jun 2, 2020
Merged

Musl support for htslib 1.10.2 #193

merged 66 commits into from
Jun 2, 2020

Conversation

brainstorm
Copy link
Member

This is a followup on #184 and #191 to restoring musl support for htslib 1.10.2 now that it requires openssl and curl (because of hfile_s3 et al).

@brainstorm
Copy link
Member Author

@nlhepler @johanneskoester Looks like I'm at the last linking step and hts-sys goes into gcc's ld when it should go for /usr/local/musl/bin/x86_64-linux-musl-ld instead:

https://github.com/rust-bio/rust-htslib/pull/193/checks?check_run_id=518274763#step:5:252

Do you have any good advice to fix that last thing up? The container has a full musl-cross-compiled toolchain with all the deps needed and I could compile hts-sys/htslib manually from within the container... I thought about just going like that towards the end of the building phase for the container:

# rm /usr/bin/ld && ln -sf /usr/local/musl/bin/x86_64-linux-musl-ld /usr/bin/ld

But I'm pretty sure there are much better ways to get this fixed properly? Any build.rs variable/flag/option that would force using the musl's ld binary instead of the system's one?

@brainstorm
Copy link
Member Author

brainstorm commented Mar 21, 2020

Now I managed to compile all the way up to the wrapper.h, looking into ways to pass -I/usr/local/musl/x86_64-linux-musl/include flags to rust bindgen since I'm getting:

rust-htslib$ cross build --target x86_64-unknown-linux-musl
(...)
running: "x86_64-linux-musl-gcc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-g" "-fno-omit-frame-pointer" "-m64" "-static" "-fPIC" "-I" "/target/x86_64-unknown-linux-musl/debug/build/bzip2-sys-97a3fa03963e25fd/out/include" "-I" "/usr/local/include" "-o" "/target/x86_64-unknown-linux-musl/debug/build/hts-sys-2bfd79b404a41047/out/wrapper.o" "-c" "wrapper.c"
exit code: 0
AR_x86_64-unknown-linux-musl = None
AR_x86_64_unknown_linux_musl = None
TARGET_AR = None
AR = Some("x86_64-linux-musl-ar")
running: "x86_64-linux-musl-ar" "crs" "/target/x86_64-unknown-linux-musl/debug/build/hts-sys-2bfd79b404a41047/out/libwrapper.a" "/target/x86_64-unknown-linux-musl/debug/build/hts-sys-2bfd79b404a41047/out/wrapper.o"
exit code: 0
cargo:rustc-link-lib=static=wrapper
cargo:rustc-link-search=native=/target/x86_64-unknown-linux-musl/debug/build/hts-sys-2bfd79b404a41047/out
cargo:warning=couldn't execute `llvm-config --prefix` (error: No such file or directory (os error 2))
cargo:warning=set the LLVM_CONFIG_PATH environment variable to a valid `llvm-config` executable

--- stderr
fatal: not a git repository: /target/x86_64-unknown-linux-musl/debug/build/hts-sys-2bfd79b404a41047/out/htslib/../../.git/modules/htslib
fatal: not a git repository: /target/x86_64-unknown-linux-musl/debug/build/hts-sys-2bfd79b404a41047/out/htslib/../../.git/modules/htslib
./htslib/htslib/hts.h:31:10: fatal error: 'stddef.h' file not found
./htslib/htslib/hts.h:31:10: fatal error: 'stddef.h' file not found, err: true
thread 'main' panicked at 'Unable to generate bindings.: ()', hts-sys/build.rs:95:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

/cc @pmarks @nlhepler

@brainstorm
Copy link
Member Author

@johanneskoester Seems to work now, but linting is happening outside cross containers... would it be ok for you to move the linting inside cross instead of the host system?

…ously according to James and Rob from the htslib team
…container, way more lightweight and less messy build process, also includes MUSL as base compiler
…support -> musl_support (refusing to allow an OAuth App to create or update workflow without scope)'
@brainstorm
Copy link
Member Author

brainstorm commented May 19, 2020

@johanneskoester Did you change any of the GitHub actions configuration for this repo recently (including personal access tokens)? I'm now getting the following when pushing to this branch (yesterday it was fine):

$ git push
Counting objects: 28, done.
Delta compression using up to 40 threads.
Compressing objects: 100% (14/14), done.
Writing objects: 100% (20/20), 2.11 KiB | 0 bytes/s, done.
Total 20 (delta 10), reused 0 (delta 0)
remote: Resolving deltas: 100% (10/10), completed with 6 local objects.
To https://github.com/brainstorm/rust-htslib.git
 ! [remote rejected] musl_support -> musl_support (refusing to allow an OAuth App to create or update workflow `.github/workflows/rust.yml` without `workflow` scope)

@brainstorm
Copy link
Member Author

@johanneskoester Both MUSL and GNU are now working thanks to Cross and the docker containers under docker/... unfortunately I cannot push any changes to .github/workflows/rust.yml anymore, so this PR will be stalled until this gets addressed, unfortunately :-S

brainstorm and others added 2 commits May 22, 2020 13:38
…nup the different Dockerfiles tested but no longer needed. build.rs now runs make clean before reconfiguring htslib for better reproducible builds
@brainstorm brainstorm mentioned this pull request Jun 2, 2020
2 tasks
@brainstorm
Copy link
Member Author

It turns out that the errors above where caused by having a HTTPS endpoint (which requires OAuth tokens now for Github Actions) instead of SSH. Switched to git:// and it's rolling again.

@johanneskoester
Copy link
Contributor

Awesome, ready for merging?

@brainstorm
Copy link
Member Author

@johanneskoester It is! The last small bit to "solve" would be to put CFLAGS=-I/usr/local/include/musl as part of the build.rs script depending on cc's target (only when MUSL, as @pmarks pointed out), but it wasn't clear to me from the cc docs how to query that.

Can be solved post-merge though since it's clearly mentioned on the README.md, I hope it doesn't generate too many issues from users.

Merging then ;)

@brainstorm brainstorm merged commit 8107160 into rust-bio:master Jun 2, 2020
TomKellyGenetics added a commit to TomKellyGenetics/rust-htslib that referenced this pull request Jun 5, 2020
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.

3 participants