onecodex / finch-rs

A genomic minhashing implementation in Rust
https://www.onecodex.com
MIT License
92 stars 8 forks source link

Transitive dependency memoffset won't build on old nightly #45

Closed maxbla closed 4 years ago

maxbla commented 4 years ago

cloning this repository and running cargo test gives a compile error in transitive dependency memoffset. cloning memoffset and running cargo +nightly-2019-10-24 test gives the same error,

error[E0658]: `cfg(doctest)` is experimental and subject to change
  --> src/lib.rs:75:7
   |
75 | #[cfg(doctest)]
   |       ^^^^^^^
   |
   = note: for more information, see https://github.com/rust-lang/rust/issues/62210
   = help: add `#![feature(cfg_doctest)]` to the crate attributes to enable

This problem is not present in memoffset 0.5.3, so the breaking change was introduced recently, after your last CI run, it appears. This might technically be a bug in memoffset's semantic versioning, but it is unclear whether semver applies to nightly, and their semver works for stable as far as I can tell.

Suggested fix: delete rust-toolchain, making it respect the user's settings by default You could update rust-toolchain to a more recent nightly (tests pass for me on today's nightly) but I'm not entirely sure why you're not using stable rust, as tests pass on stable too. I realize this crate optionally uses Pyo3, which requires nightly, but the proper approach here is to document (in the README and/or top level docs) that the python feature requires nightly and update CI accordingly.

Alternative fix: commit a cargo.lock file. This is generally considered good practice for a binary, and bad practice for a library. It seems like finch is both though, so šŸ¤·. It could be split into a separate library and binary.

PS while I was looking around, it seems like your github actions does not use the --all-features flag with cargo, so it never compiles the python module. When I test the python feature, I get linker errors on current nightly as well as 2019-10-24 nightly. This is definitely a Pyo3 bug, but you should test your builds with all valid combinations of feature-flags. Also the readme states a minimum rust version of 1.15, but CI only tests with 1.35 and stable. You have (transitive) dependencies that require newer versions of rust, for example rand 0.5+ requires rust 1.22+, and many, many more - in fact finch doesn't even compile with 1.35 anymore due to transitive dependencies.

I would be happy to submit a PR with the suggested fix.

Keats commented 4 years ago

I would welcome a PR with that!