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.
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 parking_lot to 0.10 #332
Bump parking_lot to 0.10 #332
Changes from 6 commits
287ea37
89448b6
564ead9
ed74d1f
29edac1
45a4a12
3078dc3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
sorry for being semver-nazzi, but this is a breaking change for all
kvdb-*
libs now :)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.
Now I think not.
How do you think it can break?
in previous example it could break because of implementation for parking-lot:0.9.0 gone missing, seems this is not the case here.
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.
The way to think about breaking changes is imagine you bump a version of only this crate in a lib that uses it as a dependency, will the compilation of the lib break? In this case the answer is yes, because the lib uses
parity-util-mem::MallocSizeOf
v0.4.0 and e.g.kvdb-rocksdb
v0.4.2, butkvdb-rocksdb
v0.4.3 (if you bump it that way) doesn't implement it (as it implementsparity-util-mem::MallocSizeOf
v0.5.0) and hence it doesn't implementkvdb
0.4.0 etc, so you will get compilation errors.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.
This breakage can be avoided by requiring kvdb ^0.4.3 (which is default when requiring just 0.4.3) in kvdb-rocksdb
So if you upgrade one to latest, you are forced to upgrade another
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.
sometimes you can upgrade to the latest breaking change easily, but it doesn't make the change non-breaking
sometimes you can't control all of your dependencies, imagine that
parity-util-mem
is a transitive dependency of some lib, so the lib can't just upgrade it by bumping in itsCargo.toml
;that way the lib will end up with 2 versions of
parity-util-mem
0.4.0 and 0.5.0, which are incompatibleThere 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.
Why incompatible? If you use it directly, you use only one of the versions. If you don’t use it directly, you don’t care if they are duplicated :)
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.
Well,
KeyValueDBv0.3.1
inherits fromparity_util_mem::MallocSizeOfv0.4.0
parity-common/kvdb/src/lib.rs
Line 112 in 996d370
You don't bump the
kvdb
version here, but bump onlykvdb-rocksdb
to0.4.3
, so it no longer implementsKeyValueDBv0.3.1
, right? Now, if you bumpkvdb
to0.3.2
, thenkvdb-rocksdb_v0.4.2
no longer implements that.Also parity-util-mem has a feature of global allocator which we use in parity-ethereum, so having two versions there will probably not work.
But I feel like we've spend too much time debating on this issue already and it's easier to just bump the major versions :)
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.
Imagine this code
this will break
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.
Yeah, of course it is easier, I just thought if we could avoid this cascading bump spam
But seems like no way :)
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.
Isn't that a breaking change for
parity-util-mem
?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.
Seems like no.
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.
But that's public API.
parity-util-mem::MallocSizeOf
v0.4.2 is implemented for parking_lot v0.9.0, butparity-util-mem::MallocSizeOf
v0.4.3 is not (as 0.10.0 is a breaking change to 0.9.0)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.
hmm technically yes, but it will require major bump just for everything
ok to me though
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.
yeah, that's version bump creep is unfortunate, but we already have to do this e.g. for
rand
and other libs, so it's better to safe than sorry