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

sp-api: Make the generated code act based on std in sp-api #14267

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented May 31, 2023

Instead of letting the macro generate code that checks if the std feature is enabled, it will now generate code that checks if the std feature is enabled for the sp-api crate. The old implementation basically required that the crate in which the macro was used, had a std feature. Now we don't have this requirement anymore and act accordingly the feature in sp-api directly.

Instead of letting the macro generate code that checks if the `std` feature is enabled, it will now
generate code that checks if the `std` feature is enabled for the `sp-api` crate. The old
implementation basically required that the crate in which the macro was used, had a `std` feature.
Now we don't have this requirement anymore and act accordingly the feature in `sp-api` directly.
@bkchr bkchr added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes T2-API This PR/Issue is related to APIs. labels May 31, 2023
@bkchr bkchr requested a review from kianenigma May 31, 2023 00:32
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label May 31, 2023
@bkchr bkchr requested review from a team May 31, 2023 12:15
@bkchr
Copy link
Member Author

bkchr commented May 31, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkchr bkchr requested a review from a team June 1, 2023 10:47
@@ -539,8 +539,6 @@ impl<'a> Fold for ToClientSideDecl<'a> {
input.supertraits.push(parse_quote!( #crate_::Core<#block_ident> ));
}

// The client side trait is only required when compiling with the feature `std` or `test`.
input.attrs.push(parse_quote!( #[cfg(any(feature = "std", test))] ));
Copy link
Contributor

Choose a reason for hiding this comment

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

Removal of test is intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was basically done to ensure that someone may not declares the std feature for their crate. However, this was not really working as otherwise this pr would not have been required.

@bkchr bkchr merged commit 230876c into master Jun 1, 2023
@bkchr bkchr deleted the bkchr-sp-api-std-check branch June 1, 2023 14:29
@kianenigma
Copy link
Contributor

So this doesn't solve #14145 yet it seems?

@bkchr
Copy link
Member Author

bkchr commented Jun 3, 2023

So this doesn't solve #14145 yet it seems?

No. This wasn't meant to solve this. This needs to be solved as the FRAME macros which also suffer the same problem, as I already told you. The idea of also supporting the frame crate as potential crate to import the sp-api stuff will be a good solution.

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ytech#14267)

* sp-api: Make the generated code act based on `std` in `sp-api`

Instead of letting the macro generate code that checks if the `std` feature is enabled, it will now
generate code that checks if the `std` feature is enabled for the `sp-api` crate. The old
implementation basically required that the crate in which the macro was used, had a `std` feature.
Now we don't have this requirement anymore and act accordingly the feature in `sp-api` directly.

* Missing feature!

---------

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T2-API This PR/Issue is related to APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants