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

Bump parking_lot to 0.10 #332

Merged
merged 7 commits into from
Feb 6, 2020
Merged

Bump parking_lot to 0.10 #332

merged 7 commits into from
Feb 6, 2020

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Feb 5, 2020

No description provided.

@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 5, 2020

Took me a while to realise that it is different versions of parking_lot in substrate and parity-common

ordian
ordian previously approved these changes Feb 5, 2020
Copy link
Member

@ordian ordian left a 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 }
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like no.

Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Member

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 }
Copy link
Member

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 :)

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@NikVolf NikVolf Feb 6, 2020

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

Copy link
Member

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

Copy link
Contributor Author

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 :)

Copy link
Member

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

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 :)

Copy link
Member

@ordian ordian Feb 6, 2020

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

Copy link
Contributor Author

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 :)

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks!

@ordian
Copy link
Member

ordian commented Feb 6, 2020

(also kvdb-shared-tests)

@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 6, 2020

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

@NikVolf NikVolf merged commit 27e218f into master Feb 6, 2020
@NikVolf NikVolf deleted the nv-bump-parking-lot branch February 6, 2020 12:32
@NikVolf NikVolf changed the title Bump parking_lot to 0.10 and minor versions Bump parking_lot to 0.10 Feb 6, 2020
ordian added a commit that referenced this pull request Feb 6, 2020
* master:
  [parity-crypto] Use upstream secp256k1 (#258)
  Bump parking_lot to 0.10 and minor versions (#332)
ordian added a commit that referenced this pull request Feb 7, 2020
* 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)
dvdplm added a commit that referenced this pull request Feb 8, 2020
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants