-
Notifications
You must be signed in to change notification settings - Fork 254
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
deps: use polkadot-sdk umbrella crate #1786
Conversation
] | ||
substrate-compat = ["sp-core", "sp-runtime"] | ||
substrate-compat = ["polkadot-sdk/sp-core", "polkadot-sdk/sp-runtime", "polkadot-sdk/std"] |
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.
So sp-core wasn't std before and sp-runtime was I think.. I guess std is needed here for it to work?
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.
sp-runtime was already std, https://github.com/paritytech/subxt/blob/master/Cargo.toml#L143
That's why it's required to be std but yeah it could brings in more dependencies because sp-core/std wasn't enabled before...
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.
I wonder how many more deps it ends up bringing in; maybe worth checking?
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.
It should only affect --feature --substrate-compat
and not the std or no-std which I'm okay with...
Sure, lemme check
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.
cargo check -p subxt-core --features substrate-compat
this branch brings 351 deps vs 350 on master, I think it's fine :)
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 all good then!
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 only thing that's a little scary is that if we need to have std enabled for some but not all of the sp crates somewhere then I guess we'll run into issues having this umbrella crate, but on the other hand it makes it much easier to keep these versions in sync and uptodate so I think it's a net win!
Yupp, good point I didn't think about it before :) |
No description provided.