BUG: Does not work with `no_std` under 1.29 (MSRV) by mcroad · Pull Request #690 · rust-bitcoin/rust-bitcoin

mcroad · 2021-11-06T19:40:25Z

rust-bitcoin does not work with rust 1.29 under a no_std environment. This could be considered a bug. However, no_std support is a recent addition and this is likely not breaking anyone's builds.

A decision needs to be made, either no_std MSRV is the current stable version while keeping the std MSRV as 1.29, or it needs to be fixed.

This pr adds no_std to the 1.29 test suite.

This came as I try to get rust-bitcoin/rust-miniscript#277 working and got stuck on the issue of testing no_std under 1.29.

Kixunil · 2021-11-08T10:10:39Z

I believe it's intentional as no_std is generally not well-supportd in old versions of Rust. But indeed, it does conflict with:

This library should always compile with any combination of features on Rust 1.29.

In README.md

in accordance with rust-bitcoin/rust-bitcoin#690

mcroad · 2021-12-16T20:34:54Z

@Kixunil Maybe my first report was not clear enough. As it stands, rust-bitcoin is broken because it does not compile under Rust 1.29 with no_std enabled.

My suggestion to fix it is to maintain 2 different MSRVs: 1.29 for std and the current stable version (1.57) for no_std.

Kixunil · 2021-12-16T21:22:02Z

As it stands, rust-bitcoin is broken because it does not compile under Rust 1.29 with no_std enabled.

That's exactly what I said. However it also may be that the documentation is outdated and it was intended to be an exception.

My suggestion to fix it is to maintain 2 different MSRVs: 1.29 for std and the current stable version (1.57) for no_std.

I agree. However we plan to switch to 1.36 after the Taproot release. Maybe it'll be enough? Could you check?

@apoelstra what do you think about an exception from MSRV for no_std stuff?

mcroad · 2021-12-16T22:24:08Z

https://github.com/mcroad/rust-bitcoin/runs/4553927944

Still broken under Rust 1.36.

TheBlueMatt · 2021-12-16T23:13:19Z

rust-lightning's no-std MSRV is 1.47. It would be nice if rust-bitcoin could match that (that is also core2's MSRV).

apoelstra · 2021-12-22T22:59:47Z

Oops, yes, we should have an exception. I didn't realize there wasn't already one. (I'm sure if you read the initial no_std PRs you will find some agreement on this.)

Targeting 1.47 sounds good to me. I believe the current situation is "no MSRV for nostd" which isn't great.

mcroad · 2021-12-23T00:56:49Z

All tests pass under 1.47. PR is ready to be merged.

https://github.com/mcroad/rust-bitcoin/runs/4613143768?check_suite_focus=true

While I have your attention: any idea when a new version will be released? I am waiting on #637 to be relased to finalize rust-bitcoin/rust-miniscript#277.

Thanks for the input everyone.

apoelstra · 2021-12-23T14:55:17Z

@mcroad we'll have a release once #503 is done

mcroad · 2022-02-08T17:30:56Z

Can this PR be merged to formalize 1.47 as the no-std MSRV?

Kixunil · 2022-02-08T17:49:34Z

I think it's appropriate. Doesn't fundamentally change the code so no problem for RC.

apoelstra · 2022-02-08T18:16:51Z

@mcroad the overall diff looks great. Can you squash all the commits, or otherwise clean them up?

mcroad · 2022-02-11T05:43:11Z

Cleaned up commit history. PR is ready.

Kixunil

ACK 4588f5bc8ab628961d1e019ff93100d11523a184

.github/workflows/rust.yml

mcroad · 2022-02-24T15:37:26Z

Is there anything else I need to do on my end to get the PR merged?

apoelstra · 2022-02-24T16:57:19Z

It looks like CI is failing -- it looks like #590 broke the 1.47 MSRV.

I guess we should just merge this and fix the MSRV in a separate PR.

apoelstra

ACK 4588f5bc8ab628961d1e019ff93100d11523a184

.github/workflows/rust.yml

README.md

tcharding · 2022-02-28T12:17:48Z

If you decide to remove the trailing whitespace @mcroad perhaps rebase on master at the same time and see if it fixes the CI fail?

tcharding · 2022-04-25T03:51:49Z

Thanks for your patience @mcroad, this still has PIN_VERSIONS env var being set in the CI job, can you remove that please as the variable is longer used now it got removed from test.sh in #964. Cheers

Kixunil · 2022-04-25T09:42:42Z

Unneeded PIN_VERSIONS var is the only issue I can see.

Fixes rust-bitcoin#690, rust-bitcoin#947

mcroad · 2022-04-25T16:17:02Z

Fixed PIN_VERSIONS issue.

Kixunil

ACK 7854bd7

tcharding

ACK 7854bd7

Kixunil · 2022-04-26T08:42:40Z

README.md


 ## Minimum Supported Rust Version (MSRV)

-This library should always compile with any combination of features on **Rust 1.29**.


We forgot to update other references to 1.29 but let's do it in a followup PR.

sanket1729

ACK 7854bd7

apoelstra

ACK 7854bd7

Thanks for doing so many iterations!

devrandom · 2022-05-03T14:26:49Z

would be nice for this to be included in a point release (0.28.1), so that dependent projects can upgrade to the 0.28 API ASAP

apoelstra · 2022-05-03T14:44:09Z

Sure, though we'll need to do a backport already.

apoelstra · 2022-05-03T23:01:38Z

#986

e45f6c7 bump version to 0.28.1 (Andrew Poelstra) eaaa3d0 [backport] Fix `no_std` MSRV (mcroad) Pull request description: Backport of #690 to 0.28 ACKs for top commit: tcharding: ACK e45f6c7 Tree-SHA512: 5e1459b83044acb2a628e612a45f5be5b05067429fcc7b4c4c46713bd61ee43032bbe52a80a2fc9a3ebe7cb649c2554c6500e7f7d6ea3da35e97150ca6431fc1

ac16e05 Add no_std support (mcroad) Pull request description: Closes #201. Related to bitcoindevkit/bdk#205 Not ready. - [x] `no_std` example - [x] updated readme with instructions - [x] tests pass - [x] rust-bitcoin/rust-bitcoin#637 includes `secp256k1/alloc` feature in `bitcoin/no-std`. Released on `0.28.0` - [x] rust-bitcoin/rust-bitcoin#690 Rust `1.47` set as `no_std` MSRV. Released in `0.28.1`. Maintains backward compatibility. To use it in a `no_std` context set `default-features = false` and enable the `no-std` feature. ~~To the maintainers: I added the `no-std-compat` crate. It wraps `core` and `hashbrown` and makes them look like `std`. This is a different than what `rust-bitcoin` did. They use `core` and `hashbrown` directly. I believe using `no-std-compat` makes the code more readable. I'd like to hear your thoughts on this.~~ Now using `hashbrown` and `core` directly. ACKs for top commit: sanket1729: ACK ac16e05 Tree-SHA512: 5c26cb50374b7844acbcc5a7607f5710ce19ec0aff4caed1346ed4a2fb708a909205a7d87cc27a773624f43b2a99a71c3ba4bb1cdf2dfec0584833df00b9f032

… under 1.29 (MSRV) 7854bd7 Fix `no_std` MSRV Fixes #690, #947 (mcroad) Pull request description: `rust-bitcoin` does not work with rust 1.29 under a `no_std` environment. This could be considered a bug. However, `no_std` support is a recent addition and this is likely not breaking anyone's builds. A decision needs to be made, either `no_std` MSRV is the current stable version while keeping the `std` MSRV as 1.29, or it needs to be fixed. This pr adds `no_std` to the 1.29 test suite. This came as I try to get rust-bitcoin/rust-miniscript#277 working and got stuck on the issue of testing `no_std` under 1.29. ACKs for top commit: Kixunil: ACK 7854bd7 tcharding: ACK 7854bd7 sanket1729: ACK 7854bd7 apoelstra: ACK 7854bd7 Tree-SHA512: 1614fb2193f760ed340592bdb94d076066f6f783bc1dc2b145d97f7151a28316e56b1975f1ad948460eb26db04e7e9382e60076686a681e46dcf33521fda5fca

ac16e05aa9943152e4235b490fd4ad03a496a7c1 Add no_std support (mcroad) Pull request description: Closes #201. Related to bitcoindevkit/bdk#205 Not ready. - [x] `no_std` example - [x] updated readme with instructions - [x] tests pass - [x] rust-bitcoin/rust-bitcoin#637 includes `secp256k1/alloc` feature in `bitcoin/no-std`. Released on `0.28.0` - [x] rust-bitcoin/rust-bitcoin#690 Rust `1.47` set as `no_std` MSRV. Released in `0.28.1`. Maintains backward compatibility. To use it in a `no_std` context set `default-features = false` and enable the `no-std` feature. ~~To the maintainers: I added the `no-std-compat` crate. It wraps `core` and `hashbrown` and makes them look like `std`. This is a different than what `rust-bitcoin` did. They use `core` and `hashbrown` directly. I believe using `no-std-compat` makes the code more readable. I'd like to hear your thoughts on this.~~ Now using `hashbrown` and `core` directly. ACKs for top commit: sanket1729: ACK ac16e05aa9943152e4235b490fd4ad03a496a7c1 Tree-SHA512: 5c26cb50374b7844acbcc5a7607f5710ce19ec0aff4caed1346ed4a2fb708a909205a7d87cc27a773624f43b2a99a71c3ba4bb1cdf2dfec0584833df00b9f032

mcroad mentioned this pull request Nov 12, 2021

Add no_std support rust-bitcoin/rust-miniscript#277

Merged

5 tasks

mcroad added a commit to mcroad/rust-miniscript that referenced this pull request Nov 12, 2021

only test no_std under rust stable and beta

ce572ce

in accordance with rust-bitcoin/rust-bitcoin#690

Kixunil added the waiting for author This can only progress if the author responds to a request. label Dec 2, 2021

mcroad changed the title ~~Add test with no_std under 1.29~~ BUG: Does not work with no_std under 1.29 (MSRV) Dec 16, 2021

Kixunil removed the waiting for author This can only progress if the author responds to a request. label Dec 16, 2021

Kixunil added the question label Dec 16, 2021

Kixunil mentioned this pull request Jan 14, 2022

Cannot build no-std with Rust 1.36 #782

Closed

mcroad force-pushed the master branch from e44072d to 4588f5b Compare February 11, 2022 05:42

Kixunil previously approved these changes Feb 11, 2022

View reviewed changes

tcharding reviewed Feb 11, 2022

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

apoelstra previously approved these changes Feb 24, 2022

View reviewed changes

tcharding reviewed Feb 28, 2022

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

mcroad dismissed stale reviews from apoelstra and Kixunil via 7890e76 April 9, 2022 03:04

mcroad force-pushed the master branch from 0c4fd57 to 5c4769f Compare April 9, 2022 03:20

Kixunil added bug ci and removed question labels Apr 25, 2022

Kixunil added this to the 0.29.0 milestone Apr 25, 2022

Fix no_std MSRV

7854bd7

Fixes rust-bitcoin#690, rust-bitcoin#947

mcroad force-pushed the master branch from e67ef47 to 7854bd7 Compare April 25, 2022 16:15

Kixunil approved these changes Apr 25, 2022

View reviewed changes

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Apr 25, 2022

Kixunil requested review from apoelstra and tcharding April 25, 2022 19:28

tcharding approved these changes Apr 25, 2022

View reviewed changes

Kixunil reviewed Apr 26, 2022

View reviewed changes

sanket1729 approved these changes Apr 27, 2022

View reviewed changes

apoelstra approved these changes Apr 30, 2022

View reviewed changes

apoelstra closed this in 7854bd7 Apr 30, 2022

apoelstra merged commit ff6dc61 into rust-bitcoin:master Apr 30, 2022

devrandom mentioned this pull request May 3, 2022

Update bitcoin crate to 0.28.1 lightningdevkit/rust-lightning#1389

Merged

3 tasks

tcharding pushed a commit to tcharding/rust-bitcoin that referenced this pull request May 3, 2022

Fix no_std MSRV Fixes rust-bitcoin#690, rust-bitcoin#947

f40a2eb

tcharding mentioned this pull request May 3, 2022

DO NOT MERGE: Backport 'Fix no_std MSRV Fixes' to 0.28.0 #985

Closed

apoelstra mentioned this pull request May 3, 2022

[backport] Fix no_std MSRV #986

Merged


		## Minimum Supported Rust Version (MSRV)

		This library should always compile with any combination of features on Rust 1.29.

Conversation

mcroad commented Nov 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Nov 8, 2021

Uh oh!

mcroad commented Dec 16, 2021

Uh oh!

Kixunil commented Dec 16, 2021

Uh oh!

mcroad commented Dec 16, 2021

Uh oh!

TheBlueMatt commented Dec 16, 2021

Uh oh!

apoelstra commented Dec 22, 2021

Uh oh!

mcroad commented Dec 23, 2021

Uh oh!

apoelstra commented Dec 23, 2021

Uh oh!

mcroad commented Feb 8, 2022

Uh oh!

Kixunil commented Feb 8, 2022

Uh oh!

apoelstra commented Feb 8, 2022

Uh oh!

mcroad commented Feb 11, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcroad commented Feb 24, 2022

Uh oh!

apoelstra commented Feb 24, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tcharding commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Apr 25, 2022

Uh oh!

Kixunil commented Apr 25, 2022

Uh oh!

mcroad commented Apr 25, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

devrandom commented May 3, 2022

Uh oh!

apoelstra commented May 3, 2022

Uh oh!

apoelstra commented May 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mcroad commented Nov 6, 2021 •

edited

Loading

tcharding commented Feb 28, 2022 •

edited

Loading