-
Notifications
You must be signed in to change notification settings - Fork 81
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
Update to htslib 1.10.2 (without musl) #192
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Pull 1.10.2 submodule. Initial pass on i32->i64 type conversion, will eventually fix issue #183 * Fixes issue #109 while I'm at it, long hanging fruit fix * Fix compression levels * Going through https://github.com/samtools/htslib/blob/develop/README.large_positions.md to spot 64 bit reference position changes and sam_* function renaming. Only 4 errors left, but need to closely check types afterwards very carefully. Specially for the binary formats where this change does not apply and (sub)fields that do not necessarily require big representations * Repoint submodule to upstream release tag * Switch from rust-bio hosted htslib to upstream's samtools htslib. * Try to amend submodule * Handle submodules manually on TravisCI, since it is failing now: https://travis-ci.org/rust-bio/rust-htslib/builds/645772614 * Odd balls that would need review marked with XXX. New htslib seems to require libcurl * Add curl-sys crate for hplugin_curl, aws s3 et al * Add conditional libcurl compilation flags * Be nice with cargo fmt [ci skip] * Try installing libcurl meta-package instead of the openssl dependent one?: https://travis-ci.org/rust-bio/rust-htslib/builds/645790138#L1529 * Does not work with libcurl-dev metapackage: https://travis-ci.org/rust-bio/rust-htslib/jobs/645812050#L166, rollback and try with bionic ubuntu distro. It does compile locally on OSX * Add matrix compile support for TravisCI, I would like to know why it compiles fine in OSX and fails on Linux now... * PPA source no longer needed for ubuntu bionic? [ci skip] * Bionic packages htslib with gnutls instead of openssl: https://packages.ubuntu.com/source/disco/htslib ... and it even comes with pre-shipped libcurl4-gnutls-dev, so no need to install it explicitly?: https://travis-ci.org/rust-bio/rust-htslib/jobs/645816116#L160 * sed is not gsed on OSX... not entirely sure this substitution is 100% needed * Add libssl-dev to the mix for the openssl-sys: https://travis-ci.org/rust-bio/rust-htslib/jobs/645825022#L1000. Testsuite seems to run partially now: https://travis-ci.org/rust-bio/rust-htslib/jobs/645825022#L623 * Add build-essential in hopes to fix libcurl issue, incorporate #185, fix one testsuite warning * Add curl on default features for hts-sys * Point to the right htslib upstream repo, thanks @dlaehnemann [skip ci] * Appease OSX musl-cross toolchain requirements: https://travis-ci.org/rust-bio/rust-htslib/jobs/645907695#L1222 * TravisCI timesout installing brew deps: https://travis-ci.org/rust-bio/rust-htslib/jobs/647667984#L212 ... Not sure if travis_wait will work with homebrew addons?: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received... trying * Try workaround for https://twitter.com/braincode/status/1226103002550849542 * Do not update homebrew (time intensive), switch from travis_wait to verbose to keep the build output being generated, hopefully * brew install --verbose is not verbose enough for travis, reverting travis_wait and bumping to 40min for brew deps install and musl-cross * Explicitly install system libcurl on linux and osx * This .cargo/config makes it test right locally: rust-lang/rust#60149 (comment), only 5 tests failing now and no linking errors from libcurl (on OSX, at least) :) * Cargo.toml missing quote misshap * Same linker flags should apply for Linux builds? build-essential not necessary * tid is i64 now, CRAM vs BAM test l_data is off by 3 and m_data is off too * There should be no need for travis_wait with -v * Debugging htslib structs with @jkbonfield guidance... * Disable osx and musl-cross homebrew on the TravisCI matrix since it times out anyway, start looking into l_extranul and alignment issues for CRAM/BAM. Add curl_sys crate in hts-sys/src/lib.rs. * Mystery solved for test_read_cram, testing some of the struct fields, explicitly state that l_data should differ according to #184 (comment) * Extract selective CRAM/BAM field comparison to test helper function * Formatting * Temporarily commenting the hfile format category detection makes .bed filetype tests pass but .BAI idx locator fails * Format detection seems to be broken upstream on htslib, see #184 (comment) * Revert to libcurl4-openssl-dev instead of gnutls. All TravisCI script targets compiling and passing tests otherwise * Bump up curl-sys version * Experimenting musl cross-compilation with rustembedded cross. Ideally this would get rid of the homebrew musl-cross issue (too long compile times). /cc @nlhepler * Remove assert_ne since cannot guarantee that l_data is different on all bam/crams * Add minimum deps needed for rustembedded cross docker container * Run docker in TravisCI, even if it is only supported on linux jobs :/ * Add CFLAGS suggested by @nlhepler * Separate musl from GNU toolchains on Rust cross * Bump up bindgen, vendorize openssl, further experimentation with compile flags * Deprecate the mixed GNU/MUSL container * Add minimal explanation on how to use the new docker files [ci skip] * cross 0.2.0 with openssl vendored. Progress but more htslib compile time issues remain still * Make sure libzma is present in both containers [ci skip] * Focusing on LLVMv8 GNU cross build first, musl will come later on (if possible w/ htslib) * Two more usize that should be u64 * Amend and settle BED header debate * Rename test name accordingly * Transition to cross instead of cargo for builds and tests * All GNU/clang/llvm8 builds and tests seem to succeed, transitioning to musl testing now... * Remove stray (repeated) musl build * Remove @FiloSottile musl-cross homebrew formula since it takes too long to compile and timeouts CI, using clux/muslrust gets me a bit farther. Remove local rustup musl target... since htslib 1.10, the introduction of openssl complicated local compilation w/ musl greatly, unfortunately we have to resort to docker these days instead :/ * Experiment with toolchain setup in https://gitlab.com/rust_musl_docker/image. Remove vendored openssl since it's shipped in cross containers. * Remove build.rs flags noise and different tests, containers should handle them transparently depending on the defined environment within, thanks @nlhepler * Indicate dependencies and build flags required for OSX [ci skip]. * enable musl builds * add libclang * minor * dbg * revert * dbg * dbg * Revert cross/docker madness, bottled up musl-cross myself over here: https://github.com/brainstorm/homebrew-musl-cross/commit/9304bf9cd6bc2b784083948771977d502030815a, high sierra bottle coming up soon. Cargo build script draft * Add in openssl to hts-sys * Make clippy boolean logic happy * Remove homebrew OSX check since we'll just use rustembedded cross Co-authored-by: Julian Gehring <[email protected]> Co-authored-by: Johannes Köster <[email protected]>
Closed
Pull Request Test Coverage Report for Build 9c327291c3e20e6590f3ab8f0fcd42d67654da21-PR-192
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Original PR: #184
Pull 1.10.2 submodule. Initial pass on i32->i64 type conversion, will eventually fix issue update to htslib 1.10 #183
Fixes issue Would be helpful if the installation instructions included the required package names #109 while I'm at it, long hanging fruit fix
Fix compression levels
Going through https://github.com/samtools/htslib/blob/develop/README.large_positions.md to spot 64 bit reference position changes and sam_* function renaming. Only 4 errors left, but need to closely check types afterwards very carefully. Specially for the binary formats where this change does not apply and (sub)fields that do not necessarily require big representations
Repoint submodule to upstream release tag
Switch from rust-bio hosted htslib to upstream's samtools htslib.
Try to amend submodule
Handle submodules manually on TravisCI, since it is failing now: https://travis-ci.org/rust-bio/rust-htslib/builds/645772614
Odd balls that would need review marked with XXX. New htslib seems to require libcurl
Add curl-sys crate for hplugin_curl, aws s3 et al
Add conditional libcurl compilation flags
Be nice with cargo fmt [ci skip]
Try installing libcurl meta-package instead of the openssl dependent one?: https://travis-ci.org/rust-bio/rust-htslib/builds/645790138#L1529
Does not work with libcurl-dev metapackage: https://travis-ci.org/rust-bio/rust-htslib/jobs/645812050#L166, rollback and try with bionic ubuntu distro. It does compile locally on OSX
Add matrix compile support for TravisCI, I would like to know why it compiles fine in OSX and fails on Linux now...
PPA source no longer needed for ubuntu bionic? [ci skip]
Bionic packages htslib with gnutls instead of openssl: https://packages.ubuntu.com/source/disco/htslib ... and it even comes with pre-shipped libcurl4-gnutls-dev, so no need to install it explicitly?: https://travis-ci.org/rust-bio/rust-htslib/jobs/645816116#L160
sed is not gsed on OSX... not entirely sure this substitution is 100% needed
Add libssl-dev to the mix for the openssl-sys: https://travis-ci.org/rust-bio/rust-htslib/jobs/645825022#L1000. Testsuite seems to run partially now: https://travis-ci.org/rust-bio/rust-htslib/jobs/645825022#L623
Add build-essential in hopes to fix libcurl issue, incorporate when including hts-sys, don't use default features -- let the outer r… #185, fix one testsuite warning
Add curl on default features for hts-sys
Point to the right htslib upstream repo, thanks @dlaehnemann [skip ci]
Appease OSX musl-cross toolchain requirements: https://travis-ci.org/rust-bio/rust-htslib/jobs/645907695#L1222
TravisCI timesout installing brew deps: https://travis-ci.org/rust-bio/rust-htslib/jobs/647667984#L212 ... Not sure if travis_wait will work with homebrew addons?: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received... trying
Try workaround for https://twitter.com/braincode/status/1226103002550849542
Do not update homebrew (time intensive), switch from travis_wait to verbose to keep the build output being generated, hopefully
brew install --verbose is not verbose enough for travis, reverting travis_wait and bumping to 40min for brew deps install and musl-cross
Explicitly install system libcurl on linux and osx
This .cargo/config makes it test right locally: error: linking with
cc
failed: exit code: 1 rust-lang/rust#60149 (comment), only 5 tests failing now and no linking errors from libcurl (on OSX, at least) :)Cargo.toml missing quote misshap
Same linker flags should apply for Linux builds? build-essential not necessary
tid is i64 now, CRAM vs BAM test l_data is off by 3 and m_data is off too
There should be no need for travis_wait with -v
Debugging htslib structs with @jkbonfield guidance...
Disable osx and musl-cross homebrew on the TravisCI matrix since it times out anyway, start looking into l_extranul and alignment issues for CRAM/BAM. Add curl_sys crate in hts-sys/src/lib.rs.
Mystery solved for test_read_cram, testing some of the struct fields, explicitly state that l_data should differ according to Update to htslib 1.10.2 #184 (comment)
Extract selective CRAM/BAM field comparison to test helper function
Formatting
Temporarily commenting the hfile format category detection makes .bed filetype tests pass but .BAI idx locator fails
Format detection seems to be broken upstream on htslib, see Update to htslib 1.10.2 #184 (comment)
Revert to libcurl4-openssl-dev instead of gnutls. All TravisCI script targets compiling and passing tests otherwise
Bump up curl-sys version
Experimenting musl cross-compilation with rustembedded cross. Ideally this would get rid of the homebrew musl-cross issue (too long compile times). /cc @nlhepler
Remove assert_ne since cannot guarantee that l_data is different on all bam/crams
Add minimum deps needed for rustembedded cross docker container
Run docker in TravisCI, even if it is only supported on linux jobs :/
Add CFLAGS suggested by @nlhepler
Separate musl from GNU toolchains on Rust cross
Bump up bindgen, vendorize openssl, further experimentation with compile flags
Deprecate the mixed GNU/MUSL container
Add minimal explanation on how to use the new docker files [ci skip]
cross 0.2.0 with openssl vendored. Progress but more htslib compile time issues remain still
Make sure libzma is present in both containers [ci skip]
Focusing on LLVMv8 GNU cross build first, musl will come later on (if possible w/ htslib)
Two more usize that should be u64
Amend and settle BED header debate
Rename test name accordingly
Transition to cross instead of cargo for builds and tests
All GNU/clang/llvm8 builds and tests seem to succeed, transitioning to musl testing now...
Remove stray (repeated) musl build
Remove @FiloSottile musl-cross homebrew formula since it takes too long to compile and timeouts CI, using clux/muslrust gets me a bit farther. Remove local rustup musl target... since htslib 1.10, the introduction of openssl complicated local compilation w/ musl greatly, unfortunately we have to resort to docker these days instead :/
Experiment with toolchain setup in https://gitlab.com/rust_musl_docker/image. Remove vendored openssl since it's shipped in cross containers.
Remove build.rs flags noise and different tests, containers should handle them transparently depending on the defined environment within, thanks @nlhepler
Indicate dependencies and build flags required for OSX [ci skip].
enable musl builds
add libclang
minor
dbg
revert
dbg
dbg
Revert cross/docker madness, bottled up musl-cross myself over here: https://github.com/brainstorm/homebrew-musl-cross/commit/9304bf9cd6bc2b784083948771977d502030815a, high sierra bottle coming up soon. Cargo build script draft
Add in openssl to hts-sys
Make clippy boolean logic happy
Remove homebrew OSX check since we'll just use rustembedded cross
Co-authored-by: Julian Gehring [email protected]
Co-authored-by: Johannes Köster [email protected]