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

fix: move impl-codec/std to default instead of std #630

Closed
wants to merge 1 commit into from

Conversation

gakonst
Copy link
Contributor

@gakonst gakonst commented Mar 1, 2022

Otherwise we're unable to turn on the std feature without also pulling in
all of the Parity codec dependencies which are unnecessary in non-substrate related usecases.

Otherwise we're unable to turn on the `std` feature without also pulling in
all of the Parity codec dependencies which are unnecessary in non-substrate related usecases.
@ordian
Copy link
Member

ordian commented Mar 1, 2022

Hmm, this is interesting solution to #475, but ultimately, we want https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#weak-dependency-features feature, which is going to be stabilized in rust 1.60 (coming soon: https://www.whatrustisit.com/).

That being said, I'm open to merging this PR, but need to think whether this is a breaking change.

@gakonst
Copy link
Contributor Author

gakonst commented Mar 1, 2022

Thanks for the quick response!

I am not sure how this is handled downstream, but yeah happy to help push this over the line / help with any breaking changes.

Trying to trim deps and both this PR and the one opened by Artem in #625 would be very helpful!

@gakonst
Copy link
Contributor Author

gakonst commented Mar 1, 2022

FWIW it does feel that even if it's a breaking change, should be a simple one to resolve.

@ordian
Copy link
Member

ordian commented Mar 1, 2022

FWIW it does feel that even if it's a breaking change, should be a simple one to resolve.

I wish it was easy, but unfortunately it's not (e.g. #623). If you can wait for 1.60 rustc release, I would prefer to implement this properly with the weak dependency feature. I mean we will use it anyway, but it's a matter of doing 1 breaking release now and 1 later or only 1 later.

@ordian ordian added the breaking-change Breaking change label Mar 1, 2022
@gakonst
Copy link
Contributor Author

gakonst commented Mar 1, 2022

Sounds good. How does the weak dependency feature help solve this btw?

@ordian
Copy link
Member

ordian commented Mar 1, 2022

Take a look at https://doc.rust-lang.org/nightly/cargo/reference/features.html#dependency-features

It will be impl-codec?/std instead of impl-codec/std. Which means enable std feature of impl-codec but only if impl-codec is also enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants