Skip to content

Add no_std support for crates using compiler versions >1.36 #46

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

Closed
wants to merge 4 commits into from

Conversation

reuvenpo
Copy link

@reuvenpo reuvenpo commented Apr 7, 2020

This PR raises the minimum supported compiler version to 1.36 which allows us to import the alloc built-in crate and make only the minimum set of changes required to the codebase and API.

The only change to the public API is that crates which disable the default set of features (which enables no_std) will not observe the following implementation:

 impl std::error::Error for Error { ... }

And while running the test suite, the doctests and test called convert_bits_invalid_bit_size will not run in no_std mode.

Related to issue #45 that tried adding support without changing the minimum supported compiler version.

@reuvenpo
Copy link
Author

reuvenpo commented Apr 7, 2020

Tagging recent maintainers:
@duesee @clarkmoody @sgeisler

@reuvenpo
Copy link
Author

This PR is being used to enable one of the sub-crates in this repo:
https://github.com/enigmampc/EnigmaBlockchain

@gregdhill
Copy link

Any chance we can get this merged?

@reuvenpo
Copy link
Author

It's been a few months - if this repo is under active development then it might be necessary to merge master into this branch and do some adjustments. @gregdhill feel free to take over this branch and push to it if you need to. Until this gets merged, remember you can depend on specific commit in this repo from Cargo.toml

@@ -2,11 +2,13 @@ language: rust
rust:
- stable
- nightly
- 1.22.0 # project wide min version
- 1.36.0 # project wide min version
Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing 1.29 on the main rust-bitcoin repository.

Copy link
Author

Choose a reason for hiding this comment

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

The PR description explains why i chose 1.36. I'm afraid this solution isn't supported on earlier versions.

Choose a reason for hiding this comment

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

There are workarounds for conditional compilation against the version of rustc (rustversion, rustc-version-rs) perhaps it is worth considering if backwards compatibility is necessary?

Copy link
Member

@clarkmoody clarkmoody Oct 19, 2020

Choose a reason for hiding this comment

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

Thanks for those links @gregdhill, but I think one of the strengths of this library is having zero dependencies.

It might be better to maintain a fork of the repo with this work and merge once the rust-bitcoin project moves past Rust 1.36.

Copy link
Contributor

@sgeisler sgeisler Dec 12, 2020

Choose a reason for hiding this comment

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

While raising the general msrv to 1.36.0 is a non-starter I don't think this is necessary here. It appears the incompatibility comes from using the alloc crate which only happens in no-std mode. That means we should still test the against rust 1.22.0 in std mode but against 1.36.0 in no_std mode. I think that would be a reasonable compromise. rust-bitcoin can still depend on the std version compatible with 1.22.0 and e.g. embedded devs that have to use more recent versions anyway can use no_std.

That being said, I don't need it currently, so won't put work in beyond reviewing/approving. What do you think @clarkmoody, would this be acceptable? Also: please don't merge in master into your branch, rather rebase.

Copy link
Member

Choose a reason for hiding this comment

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

@sgeisler That proposal sounds good. Would this be reflected in an update to .travis.yml?

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on if you want to run tests in no-std mode. If there was a feature that controlled std-availability, lime most crates do it, we could just add a dimension to our test matrix afaik to test everything in no-std mode too for stable and nightly (just not 1.22/1.29).

@RCasatta RCasatta mentioned this pull request Apr 11, 2021
@clarkmoody
Copy link
Member

With #51 merged, I think this PR can be closed.

@sgeisler sgeisler closed this May 21, 2021
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.

4 participants