Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

update most of the dependencies #1946

Merged
ordian merged 12 commits into from
Nov 17, 2020
Merged

update most of the dependencies #1946

ordian merged 12 commits into from
Nov 17, 2020

Conversation

@ordian
Copy link

@ordian ordian commented Nov 11, 2020

The update was done via cargo upgrade --workspace and then a couple of updates were reverted:

  • tiny-keccak was updated manually

as mentioned in paritytech/substrate#7509 (comment), it would be nice to do a burnin.

@ordian ordian added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. A1-needsburnin labels Nov 11, 2020
@ordian
Copy link
Author

ordian commented Nov 13, 2020

Haven't noticed any anomalies in the metrics.

mxinden
mxinden reviewed Nov 16, 2020
View reviewed changes
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @ordian!

For what my review is worth, the changes look good to me, apart from the question below. Given my low involvement in the Polkadot code base and the size of this change, I would very much appreciate more eyes on this pull request.

Haven't noticed any anomalies in the metrics.

Same here. This has been running on a sentry node (kusama-sentry-ew4-0) for more than 5 days. Before merging, I would recommend running it on a validator as well. What do you think @ordian?

node/core/candidate-validation/Cargo.toml Outdated

sp-core = { package = "sp-core", git = "https://github.com/paritytech/substrate", branch = "master" }
parity-scale-codec = { version = "1.3.0", default-features = false, features = ["bit-vec", "derive"] }
parity-scale-codec = { version = "1.3.5", default-features = false, features = ["bit-vec", "derive"], package = "parity-scale-codec" }
Copy link
Contributor

@mxinden mxinden Nov 16, 2020

Choose a reason for hiding this comment

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

If I understand package correctly, one can use it to rename dependencies. Why is it needed here, given that it does not rename the import variable, but instead just repeats the name?

Copy link
Author

@ordian ordian Nov 16, 2020

Choose a reason for hiding this comment

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

This was a bug in cargo-edit tool that I used to automatically bump all the dependencies. Reverted this change manually for futures in 3d16db4, thanks for catching this for parity-scale-codec, will fix now.

tomaka
tomaka reviewed Nov 16, 2020
View reviewed changes
Copy link
Contributor

@tomaka tomaka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There's indeed a lot of these package = "...", notably for futures and parity-scale-codec.

LGTM once these are removed.
I'm neutral about the question of whether to test it on a validator as well.

Andronik Ordian added 3 commits November 16, 2020 12:48
remove all renames on parity-scale-codec
65a5f59
Merge branch 'master' into ao-update-dependencies
bb1c419 
* master:
  Make sure adder collator test is executed in CI (#1949)
  Restrict collating_using_adder_collator test on feature real-overseer (#1947)
remove the leftovers
4a36454
@ordian
Copy link
Author

ordian commented Nov 16, 2020

Addressed the renaming for parity-scale-codec and the remaining for futures.

Note that this PR doesn't upgrade dependencies constrained by sub-dependencies, like libp2p (constrained by substrate) or bitvec (by parity-scale-codec). Once we upgrade dependencies in substrate, there might be a new bunch to upgrade here.

@ordian
Copy link
Author

ordian commented Nov 16, 2020

Before merging, I would recommend running it on a validator as well. What do you think @ordian?

I don't think a validator burn-in is necessary here, but don't have a strong opinion either.

mxinden
mxinden approved these changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

For what my review is worth, this looks good to me.

Andronik Ordian added 2 commits November 16, 2020 16:09
remove unused dep
d44045b
Merge branch 'master' into ao-update-dependencies
247613e 
* master:
  Breakdown the Router module on Dmp, Ump, Hrmp modules (#1939)
  Add CI job to verify extrinsic ordering (#1950)
@ordian ordian merged commit 31fb8fe into master Nov 17, 2020
@ordian ordian deleted the ao-update-dependencies branch November 17, 2020 10:16
This was referenced Nov 17, 2020
Merged
Merged
@ordian ordian mentioned this pull request Nov 18, 2020
Merged
16 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@tomaka tomaka tomaka approved these changes

@mxinden mxinden mxinden approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ordian @tomaka @mxinden