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

Bump MSRV to 1.56.1 #693

Merged
merged 2 commits into from
Mar 28, 2024
Merged

Conversation

tcharding
Copy link
Member

As we have done in other parts of the ecosystem bump the MSRV to Rust v1.56.1.

Done for secp256k1 and secp256k1-sys.

This was originally in #688 but there are too many things going on so here it is separately.

@apoelstra
Copy link
Member

Should also update clippy.toml and both READMEs.

As we have done in other parts of the ecosystem bump the MSRV to Rust
`v1.56.1`.

Done for `secp256k1` and `secp256k1-sys`.
@tcharding
Copy link
Member Author

Ok, done properly now

git grep '1\.48'
CHANGELOG.md:22: * Bump MSRV to 1.48 [#595](https://github.com/rust-bitcoin/rust-secp256k1/pull/595)
secp256k1-sys/CHANGELOG.md:17: * Bump MSRV to 1.48 [#595](https://github.com/rust-bitcoin/rust-secp256k1/pull/595)

@apoelstra
Copy link
Member

Heh, this breaks the vendoring script, which looks for patterns like x.y.z to find the version of secp256k1. Adding rust_version = "1.56.1" causes it to find two values, which derails the rest of the script.

The offending line is line 11 of secp256k1-sys/vendor-libsecp.sh which goes

DEFAULT_VERSION_CODE=$(grep version "$SECP_SYS/Cargo.toml" | sed 's/\./_/g' | sed 's/.*"\(.*\)".*/\1/')

We need to tighten this sed line.

To save you time I asked the chatbot to do it, which gave me

grep version "$SECP_SYS/Cargo.toml" | sed -n 's/version = "\([0-9]\+\.[0-9]\+\.[0-9]\+\)"/\1/p' | tr '.' '_'

which looks a lot more legit than my old code.

@tcharding
Copy link
Member Author

The bot is way better at writing shell that we are - glad I don't depend on writing shell for a paycheck.

@apoelstra
Copy link
Member

Lol, yeah. Shell has gotta be the biggest area where I'm not even close to its skill (and don't want to be :P).

We just added `rust-version = ` to the `secp256k1-sys` manifest, doing
so causes a grep statement from the vendor script to match this line -
we don't want that.

Tighten up the grep statement by only matching on `version` at the start
of the line.
@apoelstra
Copy link
Member

Oh, but lol, I think in this case it still won't work. I think we need to change the initial grep version to grep '^version =' or something, so it doesn't match on rust_version =.

@tcharding
Copy link
Member Author

The bot was wrong! I used ^ to only match on the start of the line. Not entirely sure if the quotes are needed but it works as is. FTR I was setting this value manually when I ran the script pereviously.

@apoelstra
Copy link
Member

Hah, jinx.

@tcharding
Copy link
Member Author

Ha, real time chatting on github.

@tcharding
Copy link
Member Author

jinx again

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 2d0c783

@apoelstra apoelstra merged commit ac70617 into rust-bitcoin:master Mar 28, 2024
21 checks passed
@tcharding tcharding deleted the 03-28-bump-msrv branch April 1, 2024 21:49
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

2 participants