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

primitive-types: update to scale-info 0.9 #556

Merged
merged 12 commits into from
Jul 1, 2021
Merged

Conversation

ascjones
Copy link
Member

@ascjones ascjones commented Jun 30, 2021

Latest scale-info release https://github.com/paritytech/scale-info/releases/tag/v0.9.0.

Required for paritytech/substrate#8615.

It removes the parity-scale-codec direct dependency, so might help with #552.

@ascjones ascjones requested a review from ordian June 30, 2021 11:51
@ascjones ascjones changed the title primitive-types: update scale-info, removes parity-scale-codec direct dependency primitive-types: update to scale-info 0.9 Jun 30, 2021
@ordian
Copy link
Member

ordian commented Jun 30, 2021

could you please update the changelogs as well?

ordian
ordian previously approved these changes Jun 30, 2021
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.

It's quite unfortunate that a breaking change in scale-info propagates to every single crate.
The problem is largely parity-util-mem though.

rlp/Cargo.toml Outdated Show resolved Hide resolved
triehash/Cargo.toml Outdated Show resolved Hide resolved
ethbloom/Cargo.toml Outdated Show resolved Hide resolved
@ordian ordian dismissed their stale review June 30, 2021 12:03

premature approval

triehash/Cargo.toml Outdated Show resolved Hide resolved
ascjones and others added 2 commits June 30, 2021 14:01
Co-authored-by: Andronik Ordian <write@reusable.software>
@ascjones
Copy link
Member Author

It's quite unfortunate that a breaking change in scale-info propagates to every single crate.
The problem is largely parity-util-mem though.

Indeed it is very annoying. Perhaps we could follow the pattern with impl-codec etc. and have a impl-scale-info sub crate, that would generate the TypeInfo impl instead of using the derive. They are simple types so should be straightforward.

@ascjones
Copy link
Member Author

Perhaps we could follow the pattern with impl-codec etc. and have a impl-scale-info sub crate

Gah, even then a version bump would still cascade up the dependency tree.

Once we have scale-info 1.0 we can relax the requirement here to 1, so then will only need bumping/rereleasing when there is a major version bump

@ordian
Copy link
Member

ordian commented Jun 30, 2021

Gah, even then a version bump would still cascade up the dependency tree.

Yes, the problem is largely the design of parity-util-mem and Rust Orphan Rules, which result in this cascade. I don't have a solution for this yet. But bumping versions is also not that hard.

contract-address/CHANGELOG.md Outdated Show resolved Hide resolved
primitive-types/CHANGELOG.md Outdated Show resolved Hide resolved
parity-util-mem/CHANGELOG.md Outdated Show resolved Hide resolved
parity-crypto/CHANGELOG.md Outdated Show resolved Hide resolved
kvdb/CHANGELOG.md Outdated Show resolved Hide resolved
kvdb-web/CHANGELOG.md Outdated Show resolved Hide resolved
ethereum-types/CHANGELOG.md Outdated Show resolved Hide resolved
kvdb-memorydb/CHANGELOG.md Outdated Show resolved Hide resolved
keccak-hash/CHANGELOG.md Outdated Show resolved Hide resolved
kvdb-rocksdb/CHANGELOG.md Outdated Show resolved Hide resolved
kvdb-shared-tests/CHANGELOG.md Outdated Show resolved Hide resolved
ascjones and others added 3 commits July 1, 2021 08:49
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!

parity-util-mem/CHANGELOG.md Show resolved Hide resolved
Co-authored-by: Andronik Ordian <write@reusable.software>
@ascjones
Copy link
Member Author

ascjones commented Jul 1, 2021

Hey @ordian, just had an idea: in order to prevent another release like this, what about if we expand the requirements for scale-info to e.g.

scale-info-crate = { package = "scale-info", version = ">=0.9, <2"

This means that when scale-info goes to 1.0 (soon) we won't need another release here. All we do is use the derives here so as long as the derive(TypeInfo) is valid (which it definitely will be for 1.0, then we are good.

@ordian
Copy link
Member

ordian commented Jul 1, 2021

Hey @ordian, just had an idea: in order to prevent another release like this, what about if we expand the requirements for scale-info to e.g.

scale-info-crate = { package = "scale-info", version = ">=0.9, <2"

This means that when scale-info goes to 1.0 (soon) we won't need another release here. All we do is use the derives here so as long as the derive(TypeInfo) is valid (which it definitely will be for 1.0, then we are good.

Brilliant! If you're confident it won't change in 1.0 and the tests won't break, then go ahead!

@ascjones
Copy link
Member Author

ascjones commented Jul 1, 2021

Great, I'll try it out now and see if works.

@ascjones
Copy link
Member Author

ascjones commented Jul 1, 2021

and the tests

Hm actually those tests were failing because that builder code has changed, looks like the CI doesn't run them with the scale-info feature.

Actually I don't think those tests provide much value, essentially just checking the generated code from scale-info-derive, so adding some unnecessary coupling.

@ordian
Copy link
Member

ordian commented Jul 1, 2021

Hm actually those tests were failing because that builder code has changed, looks like the CI doesn't run them with the scale-info feature.

The CI is being fixed in #559, looks like we lost travis at some point.

Actually I don't think those tests provide much value, essentially just checking the generated code from scale-info-derive, so adding some unnecessary coupling.

The tests are not a blocker for a breaking change actually (because it won't break compilation upstream), but I think they provide a value in making sure the generated derive is correct, but feel free to remove them.

@ordian ordian merged commit 5ac8cdb into master Jul 1, 2021
@ordian ordian deleted the aj-update-scale-info branch July 1, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants