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

Add conditional compilation support for impl_runtime_apis! #14709

Merged
merged 24 commits into from
Aug 24, 2023

Conversation

tdimitrov
Copy link
Contributor

@tdimitrov tdimitrov commented Aug 4, 2023

This PR handles cfg_attr attribute for iml_runtime_apis!. It allows conditionally to compile different runtime api version implementations based on feature flag. E.g.:

pub struct Runtime {}
decl_runtime_apis! {
pub trait ApiWithStagingMethod {
    fn stable_one(data: u64);
    #[api_version(99)]
    fn staging_one();
}

impl_runtime_apis! {
#[cfg_attr(feature = "enable-staging-api", api_version(99))]
impl self::ApiWithStagingMethod<Block> for Runtime {
    fn stable_one(_: u64) {}

    #[cfg(feature = "enable-staging-api")]
    fn staging_one() {}
}

The code above will compile version 1 (with just stable_one) by default and version 99 (with stable_one and staging_one) if enable-staging-api feature is enabled.

EDIT: modified the sample code to match the latest implementation suggested by @bkchr

@tdimitrov tdimitrov added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. labels Aug 4, 2023
@tdimitrov tdimitrov requested a review from bkchr August 4, 2023 07:38
@tdimitrov tdimitrov requested a review from a team as a code owner August 4, 2023 07:38
@@ -224,6 +224,8 @@ test-linux-stable:
--features runtime-benchmarks,try-runtime,experimental
--manifest-path ./bin/node/cli/Cargo.toml
--partition count:${CI_NODE_INDEX}/${CI_NODE_TOTAL}
# run runtime-api tests with `enable-staging-api` feature
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a good idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest not to do this for now, as runtime API versions often activate experimental features which may be buggy. It's acceptable for experimental features to be buggy in master, but it should not lead to test failures.

In the PR that stabilizes experimental APIs, we'd encounter the test failure in CI anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't marked the correct line. It doesn't run all tests with enable-staging-api but only sp-api-test:

time cargo nextest run -p sp-api-test --features enable-staging-api

I've got some runtime api unit tests with feature flags, but they need to be invoked individually.

@tdimitrov tdimitrov added D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B0-silent Changes should not be mentioned in any release notes labels Aug 4, 2023
primitives/api/test/Cargo.toml Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/src/lib.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/src/lib.rs Outdated Show resolved Hide resolved
primitives/api/src/lib.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
@tdimitrov tdimitrov requested a review from bkchr August 23, 2023 12:23
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.

Ty! Looks good, just some last nitpicks :)

primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
primitives/api/proc-macro/src/impl_runtime_apis.rs Outdated Show resolved Hide resolved
@bkchr bkchr requested a review from a team August 23, 2023 13:35
@bkchr
Copy link
Member

bkchr commented Aug 23, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@paritytech-ci paritytech-ci requested a review from a team August 24, 2023 07:48
@tdimitrov
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 6b07b97 into master Aug 24, 2023
7 checks passed
@paritytech-processbot paritytech-processbot bot deleted the tsv-runtime-api-feature branch August 24, 2023 08:06
@tdimitrov tdimitrov changed the title Add conditional compilation support for iml_runtime_apis! Add conditional compilation support for impl_runtime_apis! Aug 24, 2023
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