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

Conversation

@davxy
Copy link
Member

@davxy davxy commented May 22, 2023

As per title...

If I'm not terribly wrong, before this PR was not possible to define a host function gated by some #[cfg(feature = ...)]. A similar technique has been used by frame.


Usage scenario: we require to early integrate and experiment with an host function that is still in "beta" phase as this one:

https://github.com/davxy/substrate/blob/5d2e2bbf17bb7f375d6d24bfe696be2c6e54aa2a/primitives/io/src/lib.rs#L1146-L1159


I'm not a rust macro wizard 🧙 so please carefully check

@davxy davxy requested a review from koute as a code owner May 22, 2023 15:32
@davxy davxy requested review from a team and bkchr May 22, 2023 15:32
@davxy davxy self-assigned this May 22, 2023
@davxy davxy added A0-please_review Pull request needs code review. 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 22, 2023
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

A test with a feature that doesn't exist and thus, doesn't gets activated would be nice. (There is a testing crate for this stuff)

@davxy
Copy link
Member Author

davxy commented May 22, 2023

Required by #13974

Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

It would be nice to have the example of usage in: primitives/runtime-interface/README.md.

method.sig.ident,
);
let return_value = &method.sig.output;
let cfg_attrs = method.attrs.iter().filter(|a| a.path().is_ident("cfg"));
Copy link
Contributor

@michalkucharczyk michalkucharczyk May 23, 2023

Choose a reason for hiding this comment

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

nit: this filtering is repeated multiple times, maybe it is worth putting it to some shared util?

michalkucharczyk
michalkucharczyk approved these changes May 23, 2023
@michalkucharczyk michalkucharczyk requested a review from a team May 23, 2023 08:09
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@bkchr
Copy link
Member

bkchr commented May 23, 2023

It would be nice to have the example of usage in: primitives/runtime-interface/README.md.

Please add it to the crate docs as well (or only the crate docs), but never the README exclusively. Most of these readmes were generated from the crate docs.

@davxy davxy merged commit 2cc2e05 into paritytech:master May 25, 2023
@davxy davxy deleted the host-functions-cfg-support branch May 25, 2023 11:27
Ank4n pushed a commit that referenced this pull request Jul 8, 2023
* Support cfg attribute in host functions definitions

* Added test to check feature gated methods are not included

* Versioned conditional compiled host function are forbidden

* Improve runtime-interface macro docs

* Fix doc

* Apply review suggestion

Co-authored-by: Koute <[email protected]>

* Better error message

* Rust fmt

* More refinements to the docs

---------

Co-authored-by: Koute <[email protected]>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…h#14189)

* Support cfg attribute in host functions definitions

* Added test to check feature gated methods are not included

* Versioned conditional compiled host function are forbidden

* Improve runtime-interface macro docs

* Fix doc

* Apply review suggestion

Co-authored-by: Koute <[email protected]>

* Better error message

* Rust fmt

* More refinements to the docs

---------

Co-authored-by: Koute <[email protected]>
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. 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants