rust-bio / rust-bio-tools

A set of command line utilities based on Rust-Bio.
MIT License
182 stars 24 forks source link

Compile error when depending on `rust-bio` HEAD/master #201

Closed RagnarGrootKoerkamp closed 2 years ago

RagnarGrootKoerkamp commented 2 years ago

I'm getting this error when building rust-bio-tools against the latest version of rust-bio:

error[E0277]: the trait bound `rust_htslib::bam::Record: SequenceRead` is not satisfied
   --> src/bam/collapse_reads_to_fragments/calc_consensus.rs:190:10
    |
190 | impl<'a> CalcConsensus<'a, bam::Record> for CalcOverlappingConsensus<'a> {
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `SequenceRead` is not implemented for `rust_htslib::bam::Record`
    |
   ::: src/common.rs:14:32
    |
14  | pub trait CalcConsensus<'a, R: SequenceRead> {
    |                                ------------ required by this bound in `CalcConsensus`

It looks like SequenceRead is implemented for a fastq record, but not for bam records.

FelixMoelder commented 2 years ago

Hey @RagnarGrootKoerkamp, I just updated to the latest version of rust-bio but could not reproduce your issue. There were a few breaking changes that needed to be fixed (see https://github.com/rust-bio/rust-bio-tools/pull/202) but your error was not showing up. I also set bio = {git ="https://github.com/rust-bio/rust-bio", branch="master"} in Cargo.toml but that also works well. Could you give some more details to reproduce this? Have you also changed the version of rust-htslib?

RagnarGrootKoerkamp commented 2 years ago

Oh I see. It actually seems to be using rust-bio-types at HEAD. Sorry for the confusion:

bio-types = {git ="https://github.com/rust-bio/rust-bio-types", branch="master"}

Gives the error, but bio-types = "0.12.1" does not.

Edit: Huh; this doesn't make sense, since the bump to 0.12.1 is the last commit. ~I'll investigate more.~ -> Still, this is exactly the change that causes the problem. This is using bio = "0.39" as you just added it.

FelixMoelder commented 2 years ago

That's indeed a bit weird. As you say it works when setting bio-types = "0.12.1" but not when setting the git branch. The SequenceRead trait is implemented in rust-htslib (https://github.com/rust-bio/rust-htslib/blob/a21aff289bc03c7549afc7a958084ed57e8c93f2/src/bam/record.rs#L1114) and has not been touched for 3 years. Also the release history in rust-htslib is confusing and seems to be messed up.

Why exactly are you trying to use the git-repo directly? I assume you are trying to depend on the branch of your current PR in rust-bio-types?

@johanneskoester Any idea on this?

RagnarGrootKoerkamp commented 2 years ago

Yes, I have open PRs on all of rust-bio-types, rust-bio, and rust-bio-tools now, so I'm using bio = {path = "../rust-bio"} and bio-types={path="../rust-bio-types"} locally in the Cargo.toml files. (I'm not actually using the git versions; local versions are easier, but have the same problem.)

If there is a better solution that'd be nice anyway, since it's a bit tedious to keep a local version of Cargo.toml outside of git.

RagnarGrootKoerkamp commented 2 years ago

Ah, maybe the issue is that when using the github version, there are somehow multiple definitions of SequenceRead, coming from different versions of bio-types? I don't have enough experience with Rust to know how managing duplicate package dependencies work, but it could be that it doesn't collapse a git dependency to other versions.

RagnarGrootKoerkamp commented 2 years ago

Looking at cargo tree -d suggest that duplicate versions when using the git branch is indeed the issue.

bio-types v0.12.1
├── bio v0.39.0 (*)
└── rust-htslib v0.38.2 (*)

bio-types v0.12.1 (https://github.com/rust-bio/rust-bio-types?branch=master#d78b8ca4)
└── rust-bio-tools v0.32.0 (.../git/rust-bio-tools)

There's https://github.com/rust-lang/rust/issues/89143 to improve the error message to hint that there may be multiple versions.

Here's a report of someone else running into the same problem, with possible fixes: https://github.com/Malax/libcnb.rs/issues/90, in case we'd like to avoid this problem in the future.

RagnarGrootKoerkamp commented 2 years ago

Here's the 'proper' way to use local versions of crates, for future reference:

Just add this at the bottom of Cargo.toml:

[patch.crates-io]
bio = { path = "../rust-bio" }
bio-types = { path = "../rust-bio-types" }

You'll still need to not commit that hunk, but at least it's in one place, and it overrides the bio dependency everywhere where it uses the same major version (if I understand the docs correctly).