Skip to content

Conversation

@brainstorm
Copy link
Member

@brainstorm brainstorm requested a review from nlhepler June 5, 2020 05:29
@juliangehring
Copy link
Contributor

Nice! Just for reference, some benchmarks that show the benefits for htslib: http://www.htslib.org/benchmarks/zlib.html

…optional or just ship it enabled by default in light of @juliangehring htslib benchmarks link
@brainstorm
Copy link
Member Author

Thanks @juliangehring for the benchmarks pointer, interesting!

@pmarks Would you mind factoring libdeflate in while working on #207 (comment)? I'm not entirely sure if it pays off to have it optional or we could just ship it enabled by default (see benchmarks)?

I very much trust your judgement more than mine since I'm new around here, so happy to adapt ;)

Cheers!

@pmarks
Copy link
Contributor

pmarks commented Jun 7, 2020

@brainstorm cool! See my PR - I'm trying to refactor how we build htslib to give us more control of the options without relying on ./configure. libdeflater should fold into that nicely, I think that crate just needs some directives added to their build.rs.

@brainstorm
Copy link
Member Author

Gotcha, I'll leave this PR only with the docker changes then, good work Patrick!

@brainstorm brainstorm merged commit 6d17c4f into rust-bio:master Jun 9, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2020

Pull Request Test Coverage Report for Build 129323802

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.605%

Totals Coverage Status
Change from base Build 128694331: 0.0%
Covered Lines: 10156
Relevant Lines: 10967

💛 - Coveralls

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2020

Pull Request Test Coverage Report for Build 129323493

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.605%

Totals Coverage Status
Change from base Build 128694331: 0.0%
Covered Lines: 10156
Relevant Lines: 10967

💛 - Coveralls

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