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.

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

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

Closed
maxbla opened this issue May 15, 2020 · 1 comment · Fixed by #46
Closed

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

maxbla opened this issue May 15, 2020 · 1 comment · Fixed by #46

Comments

@maxbla
Copy link
Contributor

maxbla commented May 15, 2020

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
Copy link
Contributor

Keats commented May 18, 2020

I would welcome a PR with that!

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 a pull request may close this issue.

2 participants