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
@@ -8,7 +8,7 @@ else
SECP_VENDOR_GIT_ROOT="$(realpath "$SECP_VENDOR_GIT_ROOT")"
fi
SECP_SYS="$SECP_VENDOR_GIT_ROOT"/secp256k1-sys
DEFAULT_VERSION_CODE=$(grep version "$SECP_SYS/Cargo.toml" | sed 's/\./_/g' | sed 's/.*"\(.*\)".*/\1/')
DEFAULT_VERSION_CODE=$(grep "^version" "$SECP_SYS/Cargo.toml" | sed 's/\./_/g' | sed 's/.*"\(.*\)".*/\1/')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also wrong - toml allows leading whitespaces.

Just kidding. I mean it is strictly wrong but nobody cares.

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if there were a tool like jq but for TOML.

We could try using Nix, which has a fromTOML builtin function, but I'd guess that most people do not want to maintain complicated logic in the Nix language, which combines laziness and dynamic typing in a truly impossible-to-read-or-debug way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realized, would it make sense to rewrite our scripts in Nushell? It can convert Toml and a bunch of other formats to its own structured data representation. So this line would be just let version = (open Cargo.toml | get package.version). Also it handles structured data and error much better than bash but it's not as annoying when launching external programs as Python is. Though one big downside is we'd need to keep tests which we want contributors to run in bash to not annoy them too much.

I tried it out a long time ago (and even contributed a bit) so don't remember it that much but I'd love to learn it properly. I also have a nushell script wrapping lncli and converting string-encoded integers to normal integers (yes, they do that 🤦 ) if anyone is interested and I wanted to have a bitcoin-cli wrapper too. (IDK if it encodes ints as strings but at least outputting structured data would be nice.)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds very interesting. I think I'd probably go Nix first, despite its shortcomings as a language, since it's a litte more mainstream (at least, within this project it is much more well-known). In Nix it's easy to hand off to bash and share code with bash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe Nushell is more appealing to learn than Nix based on your complaints? :) I find Nushell very readable. It does have dynamic typing but it's at least strong typing and some commands know their types upfront IIRC so they can report error messages nicely.

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.

3 participants