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.

Sign up for GitHub

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

Jump to bottom

Release tracking PR: secp256k1-sys 0.10.0 #688

Merged
merged 1 commit into from Apr 2, 2024
Merged

Release tracking PR: secp256k1-sys 0.10.0 #688

merged 1 commit into from Apr 2, 2024

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 26, 2024 •
edited

Vendor latest tagged version of secp256k1 and prepare for release:

  • Bump the version number to 0.10.0
  • Run the vendor script (vendoring secp256k1 0.4.1)
  • Update lock files
  • Add changelog entry
  • Depend on new version in secp256k1/Cargo.toml

@tcharding
Copy link
Member Author

Nothing is ever as simple as it seems, if we do this we will have to do another release of secp-sys as well.

This was referenced Mar 26, 2024
Merged
Merged
@apoelstra
Copy link
Member

Heh, we might as well leave secp-sys at 1.48 then. It's okay if it's behind the other crates.

@tcharding
Copy link
Member Author

Ok, I'll spilt out another CI job for 1.48 just for secp-sys

@tcharding
Copy link
Member Author

Bother, cargo 1.48 tries to parse the top level manifest still so chokes on the edition = 2021 even though we are only testing secp256k1-sys.

@tcharding tcharding changed the title Bump MSRV to 1.56.1 Release tracking PR: secp256k1-sys 0.10.0 Mar 27, 2024
@tcharding
Copy link
Member Author

Just in case you lost context for this PR with the name changes, the problem with leaving secp256k1-sys on 1.48.0 was that cargo test didn't run in the secp256k1-sys/ directory because cargo attempts to parse to top level manifest.

@tcharding tcharding force-pushed the 03-27-bump-msrv branch 2 times, most recently from a3bf0ad to 8e0db2a Compare March 27, 2024 03:09
sanket1729
sanket1729 reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

One comment. Rest looks good

secp256k1-sys/CHANGELOG.md Outdated Show resolved Hide resolved
@apoelstra
Copy link
Member

Nothing is ever as simple as it seems, if we do this we will have to do another release of secp-sys as well.

Actually I'm a bit confused -- what is "this" here? If you mean, revendoring libsecp requires us to release a new version of secp-sys, then yes, this is true. And we ought to do that before releasing a new version of rust-secp. And if we're doing a release, why wouldn't we bump the MSRV at the same time?

I had initially read your comment as "if we bump the MSRV then we need to release a new secp-sys version, which we could avoid if we didn't bump the MSRV". But it seems like we need a new version regardless, because of the revendoring (which we should definitely do, there have been some further sidechannel defenses added upstream).

@apoelstra
Copy link
Member

Oh, I understand, we're revendoring only to change the version number.

Why don't we just update to the latest version of libsecp256k1?

@tcharding
Copy link
Member Author

Why don't we just update to the latest version of libsecp256k1?

I wondered that myself, was just artificially limiting scope, and I was lazy, I see now that lates is 0.4.1 - we should definitely upgrade to that. Will have another go.

@apoelstra
Copy link
Member

Sounds good -- I think we should update to 0.4.1, do a new secp-sys release, bump the MSRV of secp-sys while we're at it, and then we have a release that is clearly worthwhile and no questions about whether we should try to hack around the need for it.

@tcharding tcharding mentioned this pull request Mar 27, 2024
Merged
@tcharding
Copy link
Member Author

MSRV change is over in #693 now, I'll do the vendoring here still because its tied into the version number. And use 0.4.1

@tcharding tcharding marked this pull request as draft March 27, 2024 19:53
@tcharding
Copy link
Member Author

tcharding commented Mar 27, 2024 •
edited

Doing #693 separately may have been an overkill but now I've used it in the CHANGELOG, it serves some purpose (maybe) as a historical record.

apoelstra added a commit that referenced this pull request Mar 28, 2024
@apoelstra
Merge #693: Bump MSRV to 1.56.1
ac70617 
2d0c783 Tighten the version grep in vendor script (Tobin C. Harding)
a2b78f4 Bump MSRV to 1.56.1 (Tobin C. Harding)

Pull request description:

  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.

ACKs for top commit:
  apoelstra:
    ACK 2d0c783

Tree-SHA512: 35ac5632428211b02f5b25780c3a680d8c9a68b238de7299242510091f9243fe2f6718817c865c3420e3afb64b32d52daf2cf372706067204e7de42e188c31c6
@tcharding tcharding force-pushed the 03-27-bump-msrv branch 2 times, most recently from ed6f2dd to 5f75e4e Compare March 28, 2024 19:27
@tcharding tcharding marked this pull request as ready for review March 28, 2024 19:29
@apoelstra
Copy link
Member

When I run the script locally I find four files that you committed that aren't committed on my end:

        supprimé :        depend/secp256k1/TAGS
        supprimé :        depend/secp256k1/src/libsecp256k1-config.h
        supprimé :        depend/secp256k1/src/libsecp256k1-config.h.in
        supprimé :        depend/secp256k1/src/stamp-h1

(Supprimé means "deleted" in French.)

This happens both directly on my system and in my local CI.

@tcharding
Copy link
Member Author

No worries, I'm not going to get to this today but I will at some stage.

You speak French as well as English with no accent eh, that explains your grammar skills.

@apoelstra
Copy link
Member

I can read French, though I am missing a lot of vocabulary and struggle to understand even simple news articles. I keep my locale set to fr_CA mostly to fuzz my software and make sure it doesn't do something crazy/broken with a non-English local.

But I cannot speak or write French. :)

@tcharding
Copy link
Member Author

I keep my locale set to fr_CA mostly to fuzz my software and make sure it doesn't do something crazy/broken with a non-English local.

Lol, things like this remind me you are a good bloke to have on the team.

@tcharding
Copy link
Member Author

Is there a reason we haven't merged this PR and release secp256k1-sys v.010.0 - is this waiting on anything from me?

@tcharding
Copy link
Member Author

Rebased on master so we can merge this PR then #685 goes right on top of it.

@apoelstra
Copy link
Member

Is there a reason we haven't merged this PR and release secp256k1-sys v.010.0 - is this waiting on anything from me?

I was waiting for it to show up in my github notifications again, indicating that everything this was blocked on had merged, and that you'd noticed so I didn't have to :).

@apoelstra
Copy link
Member

When I run the script locally I find four files that you committed that aren't committed on my end:

        supprimé :        depend/secp256k1/TAGS
        supprimé :        depend/secp256k1/src/libsecp256k1-config.h
        supprimé :        depend/secp256k1/src/libsecp256k1-config.h.in
        supprimé :        depend/secp256k1/src/stamp-h1

(Supprimé means "deleted" in French.)

This happens both directly on my system and in my local CI.

All of these files are still present.

@tcharding
Copy link
Member Author

Jee wiz, spent so much time talking about your French skills I forgot to actually do the fix ... its hard to get good help.

@tcharding
secp256k1-sys: Vendor latest secp256k1
2bba8f9 
Vendor the latest secp256k1 `v0.4.1`. Bump the version number of
`secp256k1-sys` to `v0.10.0` and run the vendor script.

Also depend on the new version in `rust-secp256k1`, and add a changelog
entry.
@tcharding
Copy link
Member Author

Should be right now man.

apoelstra
apoelstra approved these changes Apr 2, 2024
View reviewed changes
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 2bba8f9

@apoelstra apoelstra merged commit 1e814e7 into rust-bitcoin:master Apr 2, 2024
21 checks passed
@apoelstra
Copy link
Member

Tagged and published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanket1729 sanket1729 sanket1729 left review comments

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tcharding @apoelstra @sanket1729