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

Update MSRV to 1.56.1 #639

Merged
merged 1 commit into from
Feb 17, 2024
Merged

Conversation

storopoli
Copy link
Contributor

@storopoli storopoli commented Feb 16, 2024

Following rust-bitcoin MSRV of 1.56.1, this pins MSRV to 1.56.1.

Quoting a recent merge in rust-bitcoin

Rust version 1.56.0 introduced edition 2021. Shortly afterwards, on
October 21 2021 Rust version 1.56.1 was released.

Debian stable is currently shipping rustc 1.63.0.

Our stated MSRV policy is: In Debian stable and at least 2 years old.

Therefore our MSRV policy is met by Rust version 1.56.1 and we can strat
to bump our MSRV org wide.

Links:

Removing the pins in contrib/test.sh since they are not necessary
in 1.56.1.

EDIT: CI fails because of the #[allow(dead_code)] implemented in #638 on some Psbt traits.
Do I port these to here? Conceptually they belong to #638 and not here.

storopoli added a commit to storopoli/rust-miniscript that referenced this pull request Feb 16, 2024
Nothing out of the ordinary.
The most alarming was the non-inclusive range in
`test_utils.rs`.

In some test functions I allowed myself to use
`#[allow(clippy::type_complexity)]` because clippy
was asking to create a type instead of using the big
tuple. Since these were used just once, I thought it
was overkill.

Note, depends on:

- rust-bitcoin#639 (we can also merge this into it
instead of `master`)
- rust-bitcoin#638 (has some allows on dead_code)
contrib/test.sh Outdated Show resolved Hide resolved
Following `rust-bitcoin` MSRV of 1.56.1, this pins MSRV to 1.56.1.

Quoting @tcharding
in rust-bitcoin/rust-bitcoin@d9cc724

> Rust version 1.56.0 introduced edition 2021. Shortly afterwards, on
> October 21 2021 Rust version 1.56.1 was released.
>
> Debian stable is currently shipping `rustc 1.63.0`.
>
> Our stated MSRV policy is: In Debian stable and at least 2 years old.
>
> Therefore our MSRV policy is met by Rust version 1.56.1 and we can strat
> to bump our MSRV org wide.
>
> Links:
> - https://blog.rust-lang.org/2021/11/01/Rust-1.56.1.html
> - https://blog.rust-lang.org/2021/10/21/Rust-1.56.0.html
> - https://packages.debian.org/stable/rust/rustc

Removing the pins in contrib/test.sh since they are not necessary
in 1.56.1.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK c301ce3

@apoelstra
Copy link
Member

I ACKed because my local tests pass, but the real CI fails (an unsurprising outcome for a PR that's trying to change CI and not much else).

@tcharding
Copy link
Member

tcharding commented Feb 16, 2024

Hey @storopoli I've sent you on a wild goose chase, I forgot we do not run the linter in CI in this repo. I think you should throw the dead_code attribute on those two functions but @apoelstra something is not right, needs further investigation as to why those to functions aren't used.

You could put this on the attribute: // TODO(Tobin): We shouldn't need this attribute

@apoelstra
Copy link
Member

Actually -- if the CI failures are because of missing parts from #638, then I can merge this. But I probably won't have time to check this until tomorrow.

@tcharding
Copy link
Member

kludge and merge!

@tcharding
Copy link
Member

What! I don't have overlord powers to change peoples PR descriptions - outrageous. @storopoli can you please remove the mention in the PR description, it makes github spam my notifications. Thanks man.

@storopoli
Copy link
Contributor Author

What! I don't have overlord powers to change peoples PR descriptions - outrageous. @storopoli can you please remove the mention in the PR description, it makes github spam my notifications. Thanks man.

Done, sorry about the spam.

storopoli added a commit to storopoli/rust-miniscript that referenced this pull request Feb 17, 2024
Nothing out of the ordinary.
The most alarming was the non-inclusive range in
`test_utils.rs`.

In some test functions I allowed myself to use
`#[allow(clippy::type_complexity)]` because clippy
was asking to create a type instead of using the big
tuple. Since these were used just once, I thought it
was overkill.

Note, depends on:

- rust-bitcoin#639 (we can also merge this into it
instead of `master`)
- rust-bitcoin#638 (has some allows on dead_code)
@apoelstra
Copy link
Member

What! I don't have overlord powers to change peoples PR descriptions - outrageous. @storopoli can you please remove the mention in the PR description, it makes github spam my notifications. Thanks man.

Just so you know, the merge script warns about this (and if you can't edit descriptions, you can't merge either, I guess you're not a maintainer here) and I always edit out @ characters before merging peoples' stuff.

@apoelstra
Copy link
Member

Lol, CI won't run the MSRV job unless the nightly job also passes. That's a dumb ordering because nightly breaks on its own and MSRV only breaks if the code is wrong.

Anyway I'll just run contrib.sh locally and see how things work.

@apoelstra
Copy link
Member

Ok, looks good. On the off chance that the pins are actually needed we can reinstate them when we're done other the other CI fixes.

@apoelstra apoelstra merged commit f1211c7 into rust-bitcoin:master Feb 17, 2024
10 of 16 checks passed
@storopoli storopoli deleted the js/msrv-1.56.1 branch February 17, 2024 16:14
storopoli added a commit to storopoli/rust-miniscript that referenced this pull request Feb 17, 2024
Nothing out of the ordinary.
The most alarming was the non-inclusive range in
`test_utils.rs`.

In some test functions I allowed myself to use
`#[allow(clippy::type_complexity)]` because clippy
was asking to create a type instead of using the big
tuple. Since these were used just once, I thought it
was overkill.

Note, depends on:

- rust-bitcoin#639 (we can also merge this into it
instead of `master`)
- rust-bitcoin#638 (has some allows on dead_code)
apoelstra added a commit that referenced this pull request Feb 17, 2024
e2636f3 Clippy lints MSRV 1.56.1 (Jose Storopoli)

Pull request description:

  Nothing out of the ordinary.
  The most alarming was the non-inclusive range in
  `test_utils.rs`.

  In some test functions I allowed myself to use
  `#[allow(clippy::type_complexity)]` because clippy was asking to create a type instead of using the big tuple. Since these were used just once, I thought it was overkill.

  Note, depends on:

  - #639 (we can also merge this into it instead of `master`)
  - #638 (has some allows on dead_code)

ACKs for top commit:
  apoelstra:
    ACK e2636f3

Tree-SHA512: d1f38d1e2a1415da816898806377c90986072fe0bc9a9dbc3e5476026c05c53743453cbd0a237a70aa27762f1be86e44c19422575b7e62f686140accc412177c
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.

None yet

3 participants