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

Upgrade dependencies #239

Merged
merged 16 commits into from
Oct 24, 2019
Merged

Upgrade dependencies #239

merged 16 commits into from
Oct 24, 2019

Conversation

Demi-Marie
Copy link
Contributor

The hard work was done by cargo upgrade. Split out by request of
@niklasad1.

The hard work was done by `cargo upgrade`.  Split out by request of
@niklasad1.
@parity-cla-bot
Copy link

It looks like @demimarie-parity hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@Demi-Marie Demi-Marie mentioned this pull request Oct 9, 2019
uint/tests/uint_tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@debris debris left a comment

Choose a reason for hiding this comment

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

it's not needed to bump patch versions in Cargo.toml [dependencies], nor it is needed to bump minor version if major version >= 1

@Demi-Marie
Copy link
Contributor Author

@debris The reason to bump the minor and patch versions is to ensure that we are using a version that has all of the features and bug-fixes we need. For example, hyper 0.12.35 fixes a security vulnerability (HTTP request smuggling).

authors = ["Parity Technologies <admin@parity.io>"]
license = "Apache-2.0/MIT"
homepage = "https://github.com/paritytech/parity-common"
description = "Serde serialization support for uint and fixed hash."

[dependencies]
serde = "1.0"
serde = "1.0.101"
Copy link
Member

Choose a reason for hiding this comment

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

let's not be too restrictive and revert this change

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 see your point, but I also wonder why someone would want to use an older patch release of serde. 1.0.101 also matches any higher minor or patch version.

Copy link
Member

Choose a reason for hiding this comment

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

Serde doesn't follow semver though and makes patch releases for new features as well, so this doesn't state which features of serde we rely on. On the other hand, I don't see why anyone would not want the latest version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason I can see is if they need to compile on ancient versions of rustc.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a well defined policy wrt older rustc versions other than: "compiles on stable at the time of writing (the code)".

Personally I don't mind being explicit about the version here.

uint/Cargo.toml Outdated Show resolved Hide resolved
uint/fuzz/Cargo.toml Outdated Show resolved Hide resolved
uint/Cargo.toml Outdated Show resolved Hide resolved
primitive-types/impls/rlp/Cargo.toml Outdated Show resolved Hide resolved
parity-path/Cargo.toml Outdated Show resolved Hide resolved
kvdb-memorydb/Cargo.toml Outdated Show resolved Hide resolved
fixed-hash/Cargo.toml Show resolved Hide resolved
contract-address/Cargo.toml Outdated Show resolved Hide resolved
uint/Cargo.toml Outdated Show resolved Hide resolved
@Demi-Marie
Copy link
Contributor Author

I had to re-add the rand dependency. Otherwise, the code did not compile 😢.

@ordian
Copy link
Member

ordian commented Oct 17, 2019

We can make rand optional with the following patch

diff --git a/uint/Cargo.toml b/uint/Cargo.toml
index e62e04f..b573958 100644
--- a/uint/Cargo.toml
+++ b/uint/Cargo.toml
@@ -12,13 +12,14 @@ edition = "2018"
 [dependencies]
 byteorder = { version = "1.3.2", default-features = false }
 rustc-hex = { version = "2.0.1", default-features = false }
-quickcheck = { version = "0.9.0", optional = true }
+qc = { package = "quickcheck", version = "0.9.0", optional = true }
 crunchy = { version = "0.2.2", default-features = false }
-rand = { version = "0.7.2", default-features = false }
+rand = { version = "0.7.2", default-features = false, optional = true }
 
 [features]
 default = ["std"]
-std = ["byteorder/std", "rustc-hex/std", "crunchy/std", "rand/std"]
+std = ["byteorder/std", "rustc-hex/std", "crunchy/std"]
+quickcheck = ["qc", "rand"]
 
 [[example]]
 name = "modular"
diff --git a/uint/src/lib.rs b/uint/src/lib.rs
index 5f803a2..44bcab3 100644
--- a/uint/src/lib.rs
+++ b/uint/src/lib.rs
@@ -23,7 +23,11 @@ pub use rustc_hex;
 
 #[cfg(feature="quickcheck")]
 #[doc(hidden)]
-pub use quickcheck;
+pub use qc;
+
+#[cfg(feature="quickcheck")]
+#[doc(hidden)]
+pub use rand;
 
 #[doc(hidden)]
 pub use crunchy::unroll;
diff --git a/uint/src/uint.rs b/uint/src/uint.rs
index baf2265..cf339d7 100644
--- a/uint/src/uint.rs
+++ b/uint/src/uint.rs
@@ -38,9 +38,6 @@ pub enum FromDecStrErr {
 	InvalidLength,
 }
 
-#[doc(hidden)]
-pub use rand;
-
 #[macro_export]
 #[doc(hidden)]
 macro_rules! impl_map_from {
@@ -1581,8 +1578,8 @@ macro_rules! impl_std_for_uint {
 #[doc(hidden)]
 macro_rules! impl_quickcheck_arbitrary_for_uint {
 	($uint: ty, $n_bytes: tt) => {
-		impl $crate::quickcheck::Arbitrary for $uint {
-			fn arbitrary<G: $crate::quickcheck::Gen>(g: &mut G) -> Self {
+		impl $crate::qc::Arbitrary for $uint {
+			fn arbitrary<G: $crate::qc::Gen>(g: &mut G) -> Self {
 				let mut res = [0u8; $n_bytes];
 
 				use $crate::rand::Rng;
diff --git a/uint/tests/uint_tests.rs b/uint/tests/uint_tests.rs
index 7f36b6a..0dedc07 100644
--- a/uint/tests/uint_tests.rs
+++ b/uint/tests/uint_tests.rs
@@ -1102,7 +1102,7 @@ pub mod laws {
 	macro_rules! uint_laws {
 		($mod_name:ident, $uint_ty:ident) => {
 			mod $mod_name {
-				use quickcheck::{TestResult, quickcheck};
+				use qc::{TestResult, quickcheck};
 				use super::$uint_ty;
 
 				quickcheck! {

@ordian
Copy link
Member

ordian commented Oct 21, 2019

@demimarie-parity uint/tests/uint_tests.rs part of the patch was not applied, hence CI is failing

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, thank you for taking this on.

Given how much work this turned out to be I'd say cargo upgrade is far from perfect. The nature of the crates in parity-common makes dependency upgrades pretty common. I think we should have a CONTRIBUTE file in the repo where we state (and explain) things like why we like patch updates, how we tag new releases, how to apply semver for pub use something and in general when to decide what needs to be bumped (not everything is published etc). @ordian maybe you could draft it and we'll collect some opinions?

EDIT: #248

@dvdplm dvdplm merged commit 3a028f9 into master Oct 24, 2019
@dvdplm dvdplm deleted the demi-upgrade-deps branch October 24, 2019 18:45
@dvdplm
Copy link
Contributor

dvdplm commented Oct 24, 2019

@demimarie-parity can you please take care of publishing the updated crates when you have a chance? (And don't forget to tag!)

@Demi-Marie
Copy link
Contributor Author

@dvdplm Yes, if I have the proper permissions. I will let you know if I need help.

ordian added a commit that referenced this pull request Oct 29, 2019
* master:
  Fix `impl-serde::serializa/_raw` for empty slices (#253)
  Release impl-rlp 0.2.1 (#250)
  export FromDecStrErr (#244)
  Upgrade dependencies (#239)
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.

6 participants