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.

Sign up for GitHub

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

Jump to bottom

Add basic functionality to read indexed FASTA files (continued) #214

Merged
merged 22 commits into from
Aug 4, 2020
Merged

Add basic functionality to read indexed FASTA files (continued) #214

merged 22 commits into from
Aug 4, 2020

Conversation

landesfeind
Copy link
Contributor

@landesfeind landesfeind commented Jun 14, 2020 •
edited
Loading

This pull request supersedes the #165 in using an own branch for easier integration. Most changes of @dlaehnemann (see #165 review) are incorporated.

@landesfeind landesfeind mentioned this pull request Jun 14, 2020
Closed
@dlaehnemann dlaehnemann mentioned this pull request Jun 14, 2020
Closed
@dlaehnemann
Copy link
Member

To make the clippy errors go away and make this pull request work, I think you have to add #include "htslib/htslib/faidx.h" to the include list at the start of hts-sys/wrapper.h:

rust-htslib/hts-sys/wrapper.h

Lines 1 to 9 in 61b0644

#include "htslib/htslib/hts.h"
#include "htslib/htslib/vcf.h"
#include "htslib/htslib/sam.h"
#include "htslib/htslib/cram.h"
#include "htslib/htslib/bgzf.h"
#include "htslib/htslib/vcfutils.h"
#include "htslib/htslib/tbx.h"
#include "htslib/htslib/synced_bcf_reader.h"
#include "htslib/htslib/kbitset.h"

@landesfeind
Copy link
Contributor Author

Hm, that one is actually there: https://github.com/landesfeind/rust-htslib/blob/faidx/hts-sys/wrapper.h#L10
Also, I ran the full CI pipeline in my own repository before I submitted the pull-request: https://github.com/landesfeind/rust-htslib/actions/runs/135013019

Let's try with a merging of the master branch.

@dlaehnemann
Copy link
Member

You're right. Overlooked that, as well... But it still can't find the relevant structs and functions, weird. I guess merging in the master is always worth a try...

@landesfeind
Copy link
Contributor Author

Do the *_prebuilt_bindings.rs probably miss faidx?
After merging the master, also my local build fails using cargo build. But it works well when enforcing generation of the bindings using cargo build --features bindgen.
Any chance you can give me a hint how the prebuilt are update? I compared the bindings.rs generated during my "bindgen-build" with the prebuilt one but there are many many differences. I think I can not simply copy them over.

@dlaehnemann
Copy link
Member

From looking at it, this does seem like the problem. Check out the new info at the end of the README.md section on Usage of rust-htslib. It's because using bindgen is now an optional feature, which requires prebuilt bindings for all functionality that is to work without bindgen. For context, you could checkout PR #189 which was merged 3 days ago.
But it looks like for the faidx functions and structs, this can easily be modelled upon existing code, right? Otherwise just ping in pmarks and brainstorm, they should know much more about this than I do.

@landesfeind landesfeind mentioned this pull request Jun 15, 2020
Merged
2 tasks
@pmarks
Copy link
Contributor

pmarks commented Jun 15, 2020

@landesfeind I will add updated bindings to include faidx stuff & send you a branch that you can merge into this. (I'll also include instructions for how to do the update).

@pmarks
Copy link
Contributor

pmarks commented Jun 16, 2020

@landesfeind try merging my branch here into this branch -- that should fix build without bindgen.

@landesfeind
Copy link
Contributor Author

Hi @pmarks, that did the trick! Thanks a lot! Ready for merge, unless there are more ideas on code improvement :-)

brainstorm
brainstorm requested changes Jun 22, 2020
View reviewed changes
.github/workflows/rust.yml Outdated Show resolved Hide resolved
johanneskoester
johanneskoester requested changes Jun 22, 2020
View reviewed changes
src/faidx/errors.rs Outdated Show resolved Hide resolved
johanneskoester
johanneskoester requested changes Jun 22, 2020
View reviewed changes
src/faidx/mod.rs Outdated Show resolved Hide resolved
@landesfeind
Merge remote-tracking branch 'upstream/master' into faidx
16692a3
@landesfeind
Migrate faidx to use thiserror instead of snafu (#214 (comment))
78d0e84
@landesfeind
Use i64 in faidx and return byte array
1770a09 
Internal C method faidx_fetch_seq64 uses i64 as type to index into large FASTA
sequences.
The fetch function now returns a byte array instead of a String to be more
efficient.
A new method was added to automatically convert to a String for convenience.
@landesfeind
Formatting using cargo fmt
b4c265b
brainstorm and others added 2 commits July 2, 2020 13:36
@brainstorm
Merge branch 'master' into faidx
f735af8
@landesfeind
Use usize-type for position paramter in faidx
c6dc913 
The return type of the faidx functions is changed to a Result
capturing a potential failure in converting the usize
into a i64 (hts_pos_t in the C API).
brainstorm
brainstorm requested changes Jul 20, 2020
View reviewed changes
Copy link
Member

@brainstorm brainstorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Manuel! Just the minor typo and we should be good to go IMHO.

@johanneskoester , seems that manual migrated to thiserror now, any other comments on this one?

src/faidx/errors.rs Outdated Show resolved Hide resolved
@brainstorm brainstorm merged commit 4a44444 into rust-bio:master Aug 4, 2020
@brainstorm
Copy link
Member

brainstorm commented Aug 4, 2020 •
edited
Loading

Thanks for your contribution @landesfeind 👏🏻

@landesfeind
Copy link
Contributor Author

You are welcome - thank you all for the feedback!

@landesfeind landesfeind deleted the faidx branch August 4, 2020 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brainstorm brainstorm brainstorm approved these changes

@johanneskoester johanneskoester Awaiting requested review from johanneskoester

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@landesfeind @dlaehnemann @pmarks @brainstorm @johanneskoester