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

fix: move changelog entries to correct version #3541

Merged
merged 5 commits into from
Mar 10, 2023

Conversation

thomaseizinger
Copy link
Contributor

Description

Whilst #3312 was in development, we pushed a new release out and forgot to move the changelog entries to the new version. Unfortunately, this is all still very manual until we have a solution for #2902 so this stuff keeps happening.

Notes

I did not bump the versions because this only updates the changelog so no code is affected, i.e. we don't actually require the upcoming patch release in any of our crates.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@mxinden
Copy link
Member

mxinden commented Mar 7, 2023

Thanks for preparing the follow-up patch!

I did not bump the versions because this only updates the changelog so no code is affected,

This is a follow-up to #3312 which did affect code. Why not change the version field in the crate Cargo.tomls? Thus far we always kept a crate's CHANGELOG.md heading and Cargo.toml version field in sync.

i.e. we don't actually require the upcoming patch release in any of our crates.

I am in favor of not bumping the dependents. That sounds fine to me.

@thomaseizinger
Copy link
Contributor Author

I did not bump the versions because this only updates the changelog so no code is affected,

This is a follow-up to #3312 which did affect code. Why not change the version field in the crate Cargo.tomls? Thus far we always kept a crate's CHANGELOG.md heading and Cargo.toml version field in sync.

Right, I mixed it up. For some reason I thought that the changes are released already but they are not which is the whole reason of fixing up the changelog entries! Don't mind me ...

I'll fix up the versions.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Good to go from my end @thomaseizinger.

Question below does not need to block the pull request, thus feel free to add the send-it label.

@@ -1,6 +1,6 @@
[package]
name = "libp2p-webrtc"
version = "0.4.0-alpha.2"
version = "0.4.0-alpha.3"
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that one can import this new libp2p-webrtc version through the v0.51.0 libp2p meta crate, as the v0.51.0 libp2p meta crate does not pin a specific version, more specifically 0.4.0-alpha.2 (note that there is no =)?

https://github.com/libp2p/rust-libp2p/blob/v0.51.0/Cargo.toml#L126

Should we release v0.51.1 of the libp2p meta crate, pinning all the alpha releases, like we did for v0.50.1? Similar to #3540? Otherwise we will get into the same trouble once we release a breaking new alpha of libp2p-{quic,tls,webrtc}, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I thought that we did pin those? Did my PR for moving the code into the libp2p directory mess this up?

Copy link
Member

Choose a reason for hiding this comment

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

On master we do, but not v0.51.0.


A thought that I had just now: Are we doing alphas wrong? What if we never bump the versions behind the alpha tag, but instead use major, minor and patch just like for any other crate? In this case, we would bump to libp2p-webrtc 0.4.1-alpha. For breaking changes we would do 0.5.0-alpha. That should stop cargo from automatically upgrading to breaking changes, thus causing pain to users. We would no longer need to pin xxx-alpha versions via = in the dependent's Cargo.toml.

@thomaseizinger am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do alphas correctly as per the semver spec. Given that pre-releases allow for breaking changes, one could argue that cargo should pin them by default and not upgrade from one to the other.

I am not sure if this has ever been considered.

Copy link
Member

Choose a reason for hiding this comment

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

Any objections to the above suggestion? As far as I can tell it solves all our issues.

  • No more accidental automatic upgrade, unless intended (patch version bump).
  • No issues with transient dependencies, e.g. having to pin libp2p-tls in both libp2p-quic and libp2p where libp2p-quic is as well an alpha, thus needing a bump, thus needing a bump in libp2p, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think my preference would be to remove the dependencies instead and have users deal with it or we just drop the pre-release entirely.

Copy link
Member

Choose a reason for hiding this comment

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

I think my preference would be to remove the dependencies instead

I am in favor of removing libp2p-quic and libp2p-webrtc from the meta crate.

and have users deal with it or we just drop the pre-release entirely.

How would they deal with a breaking change in libp2p-tls? Would they pin it even though it is a transient dependency? The above suggestion solves this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and have users deal with it or we just drop the pre-release entirely.

How would they deal with a breaking change in libp2p-tls? Would they pin it even though it is a transient dependency? The above suggestion solves this issue.

That is still one we'd have to pin within libp2p-quic but I think pinning alpha's within other alpha's is simply something that you have to accept?

I think my preference would be to remove the dependencies instead

I am in favor of removing libp2p-quic and libp2p-webrtc from the meta crate.

libp2p-tls too right? Or should we promote that one as non-alpha? To be honest, I don't see why it would be alpha, we are depending on rustls there which is quite tested already, no?

@mergify mergify bot merged commit 0cad636 into master Mar 10, 2023
@mergify mergify bot deleted the fix/quick-protobuf-changelog-entries branch March 10, 2023 10:59
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Whilst libp2p#3312 was in development, we pushed a new release out and forgot to move the changelog entries to the new version. Unfortunately, this is all still very manual until we have a solution for libp2p#2902 so this stuff keeps happening.

Pull-Request: libp2p#3541.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants