-
Notifications
You must be signed in to change notification settings - Fork 469
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
Prepare for the next release #444
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that "is bumping MSRV considered a breaking change?" problem is about as hard as P ?= NP, and I would fully accept and agree with any maintainer's decision here without further argument.
However, I'd like to lay the reasons why I personally think it would have been better to not make a semver-incompatible release in this case.
First, it creates additional works on the users who use crossbeam crates as public a API. For example, I maintain an
lsp-server
crate, which usescrossbeam-channel
in public API. With this new release of crossbeam channel, I need to publish a new release of mylsp-server
crate (update Cargo.toml manually in two places, commit & tag, cargo publish), and then I need to updatelsp-server
version in rust-analyzer (again, manual Cargo.toml change). If this were a minor upgrade, I would only need to run an automatedcargo update
in rust-analyzer's repo, without touchinglsp-server
at all.Second, this change hurts compile times for large projects: rust-analyzer will have two copies of crossbeam in its crate graph for quite some time, as propagating updates across the ecosystem takes time.
Third, I am sure hardly anyone would have noticed an MSRV bump from whatever it was previously to 1.28 anyway. 1.28 is extremely conservative (that is excellent, thank you!). rayon uses 1.28.0 and it is notorious for sticking with older rustcs. lazy_static bumped MSRV from 1.24 to 1.27 three months ago in a minor release, and, from what I can tell, nobody complained about this. (In other words, it makes more sense to treat MSRV change as breaking if it is not conservative, but 1.28 is very conservative, as the ecosystem goes).
To clarify, my goal here is not to do something right now, but rather to make sure that, in the future, in this and other projects, publishing a non-semver compatible update is more carefully considered.
I see that this was discussed a little, and the argument was "lazy static made a breaking change, which affected our CI, so breaking changes are bad". However,
lazy_static
did this very intentionally and with a good reason: there are trade offs here, and making this a semver-incompatible change also creates additional work on various folks. A solution for CI breakage is to use aCargo.lock
file when running tests with MSRV.I also can't help but appreciate a fine irony of the fact that bumping lazy_static's MSRV (for which I am to blame) caused me, indirectly, to write this comment :)
References:
rust-lang/api-guidelines#123
EDIT: there's actually a forth reason:
lazy_static
is a dependency ofcrossbeam-utils
, and, as it can bump MSRV in a semver-compatible release, additional guarantee ofcrossbeam
doesn't actually add much.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for writing this up! I agree with you and we should change our policy on whether bumping minimum version of Rust is a breaking change.
But, more importantly, I think we really should bring Crossbeam to 1.0 soon. I will follow up with an issue on what changes we need to do to get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One additional thing I've noticed: the umbrella crate
crossbeam
did not made a breaking change, it just bumped major versions of it's public deps, which is, dejure, a breaking change (which doesn't mean it creates problems in practice, only bug reports will tell :) ).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will admit it: this was a very rushed and belated release.
Part of the reason is the fact that I dread making releases because they're so time-consuming and finicky (update readmes, changelogs, and
Cargo.toml
s for every crate, fix up dependencies, publish to crates.io, publish git tags) and I always make mistakes while releasing new versions. I don't think there's been a single release that went smoothly without mistakes.Grouping all (or most) crossbeam crates into the main crate (kind of like what
tokio
is doing these days) and publishing 1.0s would alleviate this pain for everyone.