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

Release tracking PR - v0.29.0 #1109

Merged
merged 1 commit into from Aug 8, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jul 20, 2022

Add changelog notes and bump the version number to v0.29.0.

Notes

I attempted to go through all the PRs since last release, please sing out if you had a PR merged that is not mentioned and you would like it mentioned.

The changelog notes can be changed or improved, please do not take me writing them to imply I know exactly what goes on round here - I just made an effort to save others having to do it.

TODO

@tcharding tcharding changed the title Realese tracking PR - v0.29.0 Release tracking PR - v0.29.0 Jul 20, 2022
@sanket1729
Copy link
Member

I tried to add label:"release notes mention" to aid this. But did not do so consistently.

@tcharding
Copy link
Member Author

tcharding commented Jul 20, 2022

Nice idea, who do you imagine adds that label, the author wanting a mention, reviewers thinking its important enough, or the merger?

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 21, 2022

I suggest removing #1114 from requirements since it doesn't affect consumer code.

@tcharding I suggest the label is added by whoever thinks it should be and if there's disagreement we can discuss. I'd also like two more labels: no-release-notes-mention and aggregate-release-notes-mention first one to skip PR completely when making release notes, the second to just put e.g. "Various documentation improvements" - should have label for appropriate category. Then CI check that makes it mandatory for a PR to have exactly one label so that we don't forget.

@apoelstra
Copy link
Member

Agreed, I don't think it's important who adds these labels -- they're unlikely to be controversial and if they are, that seems evidence enough to put something into the release notes :)

@tcharding
Copy link
Member Author

Then CI check that makes it mandatory for a PR to have exactly one label so that we don't forget.

Doing this implies that the author must add the label, I'd like to prevent burdening newer contributors with tasks that are just there to help with administrative tasks (writing the release notes).

If the label is optional it at least means we give authors a chance to request that their work gets mentioned. Any PRs that don't get tagged just get left out of the release notes, no big deal.

I've added the two additional labels you requested @Kixunil

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 22, 2022

implies that the author

It doesn't, it implies a reviewer has to do that. However, I just realized it's possibly annoying, since a non-member would have to trigger CI re-run after the label was added.

burdening newer contributors with tasks that are just there to help with administrative tasks

From my experience of contributing to LND where they have very similar rule - just it has to be in a file, it's not a big deal if not for exactly these problems:

  • You can't reliably predict PR id, so you have to make a PR first and then immediately amend it with release notes addition
  • Multiple parallel PRs always cause rebases

Ideally GitHub would have a native tool to resolve that but it could be done with this trick:

  • Require 3 approvals
  • Upon PR opening a bot immediately checks labels and requests changes, possibly the author can add the label himself with a command similar to rustbot
  • After label is added the bot approves the PR.

From my experience contributing to Rust lang, these bots are pretty nice and can do multiple things including welcoming new contributors, assigning reviewers...

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 26, 2022

I request that if #994 gets merged then also #1129 gets merged.

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 27, 2022

I added #1137 to required because it'd greatly suck to not have TryFrom and have inherent try_from method since the trait was added after MSRV bump.
I have also moved #1129 to required because it fixes Sequence not having stringly conversions. Without them original code sequence: some_str.parse()? would be annoying to rewrite while with them it'll continue to work fine.

@sanket1729
Copy link
Member

Can we also add #1151 which includes a bug fix and some cleanups

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 1, 2022

I've triggered re-run of CI.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 1, 2022

All, green, almost ready. However I just realized maybe #1144 would be good to include. Pros/cons:

  • Not merging gives more time to people to update, we still remove maintenance cost by merging shortly after release.
  • Merging is final "no longer supported" act that removes any potential "bug fix" issues/PRs; we would just wontfix that though.
  • Joining breaking changes into single release makes point releases easier but probably non issue with clean removal of stuff.
  • Not merging can make release sooner.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 1, 2022

I did go through all PRs and added missing milestone/API break markers. We should make the list of breaking changes visible.

@apoelstra
Copy link
Member

Sure, I'll try to sneak in #1144 today.

@apoelstra
Copy link
Member

It's been deprecated forever and AFAIK never had any value, I see no reason to keep it around for another cycle.

sanket1729
sanket1729 previously approved these changes Aug 2, 2022
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.

ACK 1255de6. Let's get rolling

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

I think we should make breaking changes more visible. Other than that ACK.

CHANGELOG.md Outdated
same time we were able to start using `rustfmt`. We also started linting as part
of CI.

Some highlights:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Some highlights:
## Breaking changes
There are numerous breaking changes in this release related to the new language features but also other improvements such as more newtypes added. Note that not all changes cause compilation failure! [`Witness` serialization was changed](https://github.com/rust-bitcoin/rust-bitcoin/pull/1068) to support human-readable formats.
[Detailed list of breaking changes](https://github.com/rust-bitcoin/rust-bitcoin/pulls?q=is%3Apr+label%3A%22API+break%22+is%3Aclosed+milestone%3A0.29.0+)
## Highlights

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. And we need to add #1161 to the list

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the list of breaking changes, it raises a question - if we are to use this sort of link (filtered search) for each release we have to ensure all PRs have a milestone when they merge. It would be nice if this was automated so we don't miss any.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added the sections you suggest, thanks. Added #1161 to highlights also.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, this time I went over them and manually modified. I'd like to avoid repeating it...

Kixunil
Kixunil previously approved these changes Aug 2, 2022
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 87328ab

Let's goooo!

@apoelstra
Copy link
Member

Should we update our rust-bitcoinconsensus dep as well?

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 3, 2022

@apoelstra definitely! Good catch. I'm confused about the versioning though. I think cargo treats things with - as development versions and will not allow later version that's compatible. Can you explain why there's such versioning?

@apoelstra
Copy link
Member

@Kixunil rust-bitcoin/rust-bitcoinconsensus#40 everything before the - is the Bitcoin Core version.

As far as I'm concerned this is a problem with crates.io/cargo and there's not much we can do about it.

@tcharding can you change this PR to bump the bitcoinconsensus version?

@tcharding
Copy link
Member Author

@tcharding can you change this PR to bump the bitcoinconsensus version?

I did it as a separate PR: #1165

This raised the question, do we want to update bitcoinconsensus to use bitcoin core 21 (taproot release) and depend on that before this release or does that not matter? For the record I have an open PR to update bitcoinconsensus but its broken, working on fixing it now.

@tcharding
Copy link
Member Author

Maybe we should just go ahead and release with bitcoinconsensus 0.20.2-0.5.0 because I cannot get the upgrade to bitcoin core 21 working.

@apoelstra
Copy link
Member

Agreed. I'd like to have 21 but I don't think it's a blocker, and if it's nontrivial to do then we can let it slip.

@tcharding
Copy link
Member Author

I added P-high label to #1165, so we can get that through. I'll push update to release notes and rebase soon as that one merges. Then we should be about right to go, anything else we are missing?

@sanket1729
Copy link
Member

@tcharding merged #1165 . I think we are good to go

Add changelog notes and bump the version number to v0.29.0.
@tcharding
Copy link
Member Author

tcharding commented Aug 5, 2022

Rebased and added chagelog entry for bitcoinconsensus dependency upgrade - lets go!

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 110b5d8

Let's release!

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 5, 2022

Oh, can we get #1171 in? It's trivial and would be nice to combine it with having Sequence.

@apoelstra
Copy link
Member

OK, the two remaining checklist questions are #1106 :

  • Assess the semver trick (which should actually be done after release ... in fact we should go back and see if we can't semver-trick the last 5 or 10 releases to ease migration)
  • See if we should write an upgrade guide (I'm going to punt on this for now, let's build up to having sane processes)

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 110b5d8

@apoelstra
Copy link
Member

Will wait for one more ACK then let's go!

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 5, 2022

I suspect that semver trick is too complex for these changes but I'm willing to review if someone figures it out. Also upgrading guide doesn't feel required since we were pretty careful about things like people accidentally unwrapping stuff. (Maybe that part can only be decided by the number of "I can't upgrade" issues.)

@apoelstra
Copy link
Member

Agreed that we won't be able to e.g. semver-trick Transaction since we introduced these new Sequence/LockTime types that can't possibly be made compatible with old versions of rust-bitcoin.

But we could semver-trick e.g. the entire bip32 module, which has barely changed at all for several versions.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 6, 2022

Hmm, partial semver trick may be useful too. I'm not sure how much it helps though.

@apoelstra
Copy link
Member

@Kixunil i think we could partial-semver-trick Script and Address and bip32 say, so then projects that depend on both bitcoin and miniscript would have a very good chance of continuing to work even if users updated bitcoin but not miniscript.

@Kixunil
Copy link
Collaborator

Kixunil commented Aug 6, 2022

Oh, Address sounds good indeed, also Amount I have crates that use only those, so I guess others may as well.

@apoelstra
Copy link
Member

BTW @tcharding could we have an ACK on this? I don't wanna publish without your awareness.

@tcharding
Copy link
Member Author

ACK 110b5d8

Thanks!

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.

reACK 110b5d8

@apoelstra
Copy link
Member

Let's go!

@apoelstra apoelstra merged commit 8a30169 into rust-bitcoin:master Aug 8, 2022
@apoelstra
Copy link
Member

Published, signed and tagged.

@tcharding tcharding deleted the 07-20-release-0.29.0 branch August 10, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants