-
Notifications
You must be signed in to change notification settings - Fork 215
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
Conversation
Took me a while to realise that it is different versions of |
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.
changelogs please?
@@ -25,7 +25,7 @@ impl-trait-for-tuples = "0.1.3" | |||
|
|||
smallvec = { version = "1.0.0", optional = true } | |||
ethereum-types = { version = "0.8.0", optional = true, path = "../ethereum-types" } | |||
parking_lot = { version = "0.9.0", optional = true } | |||
parking_lot = { version = "0.10.0", optional = true } |
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, but parity-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
@@ -10,4 +10,4 @@ edition = "2018" | |||
[dependencies] | |||
smallvec = "1.0.0" | |||
bytes = { package = "parity-bytes", version = "0.1", path = "../parity-bytes" } | |||
parity-util-mem = { path = "../parity-util-mem", version = "0.4", default-features = false } | |||
parity-util-mem = { path = "../parity-util-mem", version = "0.5", default-features = false } |
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, but kvdb-rocksdb
v0.4.3 (if you bump it that way) doesn't implement it (as it implements parity-util-mem::MallocSizeOf
v0.5.0) and hence it doesn't implement kvdb
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 its Cargo.toml
;
that way the lib will end up with 2 versions of parity-util-mem
0.4.0 and 0.5.0, which are incompatible
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.
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 from parity_util_mem::MallocSizeOfv0.4.0
Line 112 in 996d370
pub trait KeyValueDB: Sync + Send + parity_util_mem::MallocSizeOf { |
You don't bump the kvdb
version here, but bump only kvdb-rocksdb
to 0.4.3
, so it no longer implements KeyValueDBv0.3.1
, right? Now, if you bump kvdb
to 0.3.2
, then kvdb-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.
hy 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 :)
Imagine this code
#[derive(MallocSizeOf)]
pub struct Foo {
foo: third_party_lib::implements_parity_util_mem::MallocSizeOf_v_0_4_0,
bar: parking_lot::RwLock<Baz>,
kvdb: Arc<dyn KeyValueDB>,
}
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.
easier to just bump the major versions
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.
Thanks!
(also kvdb-shared-tests) |
9bdc869
to
45a4a12
Compare
I imagine doing this for substrate crates if they go to crates.io @bkchr The whole thing just to update parking_lot 0.9 -> 0.10 |
df47594
to
3078dc3
Compare
* master: Add different mode for malloc_size_of_is_0 macro dealing with generics (#334) [parity-crypto] Use upstream secp256k1 (#258) Bump parking_lot to 0.10 and minor versions (#332) Remove libc completely (#333) update changelogs (#329) bump parity-util-mem to 0.4.2 (#328) remove libc feature from fixed-hash (#317) kvdb-rocksdb: release 0.4.2 (#327) kvdb-rocksdb: fix iter_from_prefix being slow (#326) MallocSizeOf for BTreeSet (#325) split off primitives (#323) travis: disable kvdb-web tests for chrome (#324) Expand const fn coverage (#319) uint: make zero const fn (#318) README: fix appveyor badge (#316)
No description provided.