From dbb48a5ba28eba7716339312692f498d59d7633c Mon Sep 17 00:00:00 2001
From: Tsvetomir Dimitrov <tsv.dimitrov@gmail.com>
Date: Thu, 24 Aug 2023 11:06:07 +0300
Subject: [PATCH] Add conditional compilation support for `iml_runtime_apis!`
(#14709)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* Handle `cfg_attr` in `decl_runtime_api`
* Integration tests
* UI tests
* Enable staging api tests on CI
* docs
* Comments and minor style fixes
* Fix doc comments
* Apply suggestions from code review
Co-authored-by: Bastian Köcher <git@kchr.de>
* Apply suggestions from code review
Co-authored-by: Bastian Köcher <git@kchr.de>
* Fix formatting and a compilation error
* Fix a doc comment
* Combine the errors from `ApiImplItem`
* Fix a typo in UI test
* Use attribute span when reporting multiple conditional `api_version` error
* Apply suggestions from code review
Co-authored-by: Bastian Köcher <git@kchr.de>
* Don't use `cfg_attr` for methods. Use simple feature gate instead
* Remove a stale comment
* Update primitives/api/proc-macro/src/impl_runtime_apis.rs
Co-authored-by: Bastian Köcher <git@kchr.de>
* Revert "Update primitives/api/proc-macro/src/impl_runtime_apis.rs"
This reverts commit 4da20a79bd762580ebf502e9f807c2d7e5876106.
* Code review feedback
* Style improvements
---------
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>
---
.../api/proc-macro/src/impl_runtime_apis.rs | 183 ++++++++++++++++--
substrate/primitives/api/src/lib.rs | 48 +++++
substrate/primitives/api/test/Cargo.toml | 3 +
.../api/test/tests/decl_and_impl.rs | 83 ++++++++
substrate/scripts/ci/gitlab/pipeline/test.yml | 2 +
5 files changed, 301 insertions(+), 18 deletions(-)
diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs
index 66bc5b0e9e5..74cfa098062 100644
--- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs
+++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs
@@ -31,10 +31,11 @@ use quote::quote;
use syn::{
fold::{self, Fold},
+ parenthesized,
parse::{Error, Parse, ParseStream, Result},
parse_macro_input, parse_quote,
spanned::Spanned,
- Attribute, Ident, ImplItem, ItemImpl, Path, Signature, Type, TypePath,
+ Attribute, Ident, ImplItem, ItemImpl, LitInt, LitStr, Path, Signature, Type, TypePath,
};
use std::collections::HashSet;
@@ -67,6 +68,7 @@ fn generate_impl_call(
runtime: &Type,
input: &Ident,
impl_trait: &Path,
+ api_version: &ApiVersion,
) -> Result<TokenStream> {
let params =
extract_parameter_names_types_and_borrows(signature, AllowSelfRefInParameters::No)?;
@@ -111,11 +113,40 @@ fn generate_impl_call(
)
};
+ let fn_calls = if let Some(feature_gated) = &api_version.feature_gated {
+ let pnames = pnames2;
+ let pnames2 = pnames.clone();
+ let pborrow2 = pborrow.clone();
+
+ let feature_name = &feature_gated.0;
+ let impl_trait_fg = extend_with_api_version(impl_trait.clone(), Some(feature_gated.1));
+ let impl_trait = extend_with_api_version(impl_trait.clone(), api_version.custom);
+
+ quote!(
+ #[cfg(feature = #feature_name)]
+ #[allow(deprecated)]
+ let r = <#runtime as #impl_trait_fg>::#fn_name(#( #pborrow #pnames ),*);
+
+ #[cfg(not(feature = #feature_name))]
+ #[allow(deprecated)]
+ let r = <#runtime as #impl_trait>::#fn_name(#( #pborrow2 #pnames2 ),*);
+
+ r
+ )
+ } else {
+ let pnames = pnames2;
+ let impl_trait = extend_with_api_version(impl_trait.clone(), api_version.custom);
+
+ quote!(
+ #[allow(deprecated)]
+ <#runtime as #impl_trait>::#fn_name(#( #pborrow #pnames ),*)
+ )
+ };
+
Ok(quote!(
#decode_params
- #[allow(deprecated)]
- <#runtime as #impl_trait>::#fn_name(#( #pborrow #pnames2 ),*)
+ #fn_calls
))
}
@@ -130,7 +161,6 @@ fn generate_impl_calls(
let trait_api_ver = extract_api_version(&impl_.attrs, impl_.span())?;
let impl_trait_path = extract_impl_trait(impl_, RequireQualifiedTraitPath::Yes)?;
let impl_trait = extend_with_runtime_decl_path(impl_trait_path.clone());
- let impl_trait = extend_with_api_version(impl_trait, trait_api_ver);
let impl_trait_ident = &impl_trait_path
.segments
.last()
@@ -139,14 +169,23 @@ fn generate_impl_calls(
for item in &impl_.items {
if let ImplItem::Fn(method) = item {
- let impl_call =
- generate_impl_call(&method.sig, &impl_.self_ty, input, &impl_trait)?;
+ let impl_call = generate_impl_call(
+ &method.sig,
+ &impl_.self_ty,
+ input,
+ &impl_trait,
+ &trait_api_ver,
+ )?;
+ let mut attrs = filter_cfg_attrs(&impl_.attrs);
+
+ // Add any `#[cfg(feature = X)]` attributes of the method to result
+ attrs.extend(filter_cfg_attrs(&method.attrs));
impl_calls.push((
impl_trait_ident.clone(),
method.sig.ident.clone(),
impl_call,
- filter_cfg_attrs(&impl_.attrs),
+ attrs,
));
}
}
@@ -441,6 +480,18 @@ fn extend_with_api_version(mut trait_: Path, version: Option<u64>) -> Path {
trait_
}
+/// Adds a feature guard to `attributes`.
+///
+/// Depending on `enable`, the feature guard either enables ('feature = "something"`) or disables
+/// (`not(feature = "something")`).
+fn add_feature_guard(attrs: &mut Vec<Attribute>, feature_name: &str, enable: bool) {
+ let attr = match enable {
+ true => parse_quote!(#[cfg(feature = #feature_name)]),
+ false => parse_quote!(#[cfg(not(feature = #feature_name))]),
+ };
+ attrs.push(attr);
+}
+
/// Generates the implementations of the apis for the runtime.
fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result<TokenStream> {
let mut impls_prepared = Vec::new();
@@ -451,12 +502,29 @@ fn generate_api_impl_for_runtime(impls: &[ItemImpl]) -> Result<TokenStream> {
let trait_api_ver = extract_api_version(&impl_.attrs, impl_.span())?;
let mut impl_ = impl_.clone();
+ impl_.attrs = filter_cfg_attrs(&impl_.attrs);
+
let trait_ = extract_impl_trait(&impl_, RequireQualifiedTraitPath::Yes)?.clone();
let trait_ = extend_with_runtime_decl_path(trait_);
- let trait_ = extend_with_api_version(trait_, trait_api_ver);
+ // If the trait api contains feature gated version - there are staging methods in it. Handle
+ // them explicitly here by adding staging implementation with `#cfg(feature = ...)` and
+ // stable implementation with `#[cfg(not(feature = ...))]`.
+ if let Some(feature_gated) = trait_api_ver.feature_gated {
+ let mut feature_gated_impl = impl_.clone();
+ add_feature_guard(&mut feature_gated_impl.attrs, &feature_gated.0, true);
+ feature_gated_impl.trait_.as_mut().unwrap().1 =
+ extend_with_api_version(trait_.clone(), Some(feature_gated.1));
+
+ impls_prepared.push(feature_gated_impl);
+
+ // Finally add `#[cfg(not(feature = ...))]` for the stable implementation (which is
+ // generated outside this if).
+ add_feature_guard(&mut impl_.attrs, &feature_gated.0, false);
+ }
+ // Generate stable trait implementation.
+ let trait_ = extend_with_api_version(trait_, trait_api_ver.custom);
impl_.trait_.as_mut().unwrap().1 = trait_;
- impl_.attrs = filter_cfg_attrs(&impl_.attrs);
impls_prepared.push(impl_);
}
@@ -663,7 +731,8 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result<TokenStream> {
let c = generate_crate_access();
for impl_ in impls {
- let api_ver = extract_api_version(&impl_.attrs, impl_.span())?.map(|a| a as u32);
+ let versions = extract_api_version(&impl_.attrs, impl_.span())?;
+ let api_ver = versions.custom.map(|a| a as u32);
let mut path = extend_with_runtime_decl_path(
extract_impl_trait(impl_, RequireQualifiedTraitPath::Yes)?.clone(),
@@ -687,11 +756,34 @@ fn generate_runtime_api_versions(impls: &[ItemImpl]) -> Result<TokenStream> {
}
let id: Path = parse_quote!( #path ID );
- let version = quote!( #path VERSION );
- let attrs = filter_cfg_attrs(&impl_.attrs);
+ let mut attrs = filter_cfg_attrs(&impl_.attrs);
+
+ // Handle API versioning
+ // If feature gated version is set - handle it first
+ if let Some(feature_gated) = versions.feature_gated {
+ let feature_gated_version = feature_gated.1 as u32;
+ // the attributes for the feature gated staging api
+ let mut feature_gated_attrs = attrs.clone();
+ add_feature_guard(&mut feature_gated_attrs, &feature_gated.0, true);
+ populate_runtime_api_versions(
+ &mut result,
+ &mut sections,
+ feature_gated_attrs,
+ id.clone(),
+ quote!( #feature_gated_version ),
+ &c,
+ );
+
+ // Add `#[cfg(not(feature ...))]` to the initial attributes. If the staging feature flag
+ // is not set we want to set the stable api version
+ add_feature_guard(&mut attrs, &feature_gated.0, false);
+ }
- let api_ver = api_ver.map(|a| quote!( #a )).unwrap_or_else(|| version);
- populate_runtime_api_versions(&mut result, &mut sections, attrs, id, api_ver, &c)
+ // Now add the stable api version to the versions list. If the api has got staging functions
+ // there might be a `#[cfg(not(feature ...))]` attribute attached to the stable version.
+ let base_api_version = quote!( #path VERSION );
+ let api_ver = api_ver.map(|a| quote!( #a )).unwrap_or_else(|| base_api_version);
+ populate_runtime_api_versions(&mut result, &mut sections, attrs, id, api_ver, &c);
}
Ok(quote!(
@@ -758,12 +850,64 @@ fn filter_cfg_attrs(attrs: &[Attribute]) -> Vec<Attribute> {
attrs.iter().filter(|a| a.path().is_ident("cfg")).cloned().collect()
}
+/// Parse feature flagged api_version.
+/// E.g. `#[cfg_attr(feature = "enable-staging-api", api_version(99))]`
+fn extract_cfg_api_version(attrs: &Vec<Attribute>, span: Span) -> Result<Option<(String, u64)>> {
+ let cfg_attrs = attrs.iter().filter(|a| a.path().is_ident("cfg_attr")).collect::<Vec<_>>();
+
+ let mut cfg_api_version_attr = Vec::new();
+ for cfg_attr in cfg_attrs {
+ let mut feature_name = None;
+ let mut api_version = None;
+ cfg_attr.parse_nested_meta(|m| {
+ if m.path.is_ident("feature") {
+ let a = m.value()?;
+ let b: LitStr = a.parse()?;
+ feature_name = Some(b.value());
+ } else if m.path.is_ident(API_VERSION_ATTRIBUTE) {
+ let content;
+ parenthesized!(content in m.input);
+ let ver: LitInt = content.parse()?;
+ api_version = Some(ver.base10_parse::<u64>()?);
+ }
+ Ok(())
+ })?;
+
+ // If there is a cfg attribute containing api_version - save if for processing
+ if let (Some(feature_name), Some(api_version)) = (feature_name, api_version) {
+ cfg_api_version_attr.push((feature_name, api_version, cfg_attr.span()));
+ }
+ }
+
+ if cfg_api_version_attr.len() > 1 {
+ let mut err = Error::new(span, format!("Found multiple feature gated api versions (cfg attribute with nested `{}` attribute). This is not supported.", API_VERSION_ATTRIBUTE));
+ for (_, _, attr_span) in cfg_api_version_attr {
+ err.combine(Error::new(attr_span, format!("`{}` found here", API_VERSION_ATTRIBUTE)));
+ }
+
+ return Err(err)
+ }
+
+ Ok(cfg_api_version_attr
+ .into_iter()
+ .next()
+ .map(|(feature, name, _)| (feature, name)))
+}
+
+/// Represents an API version.
+struct ApiVersion {
+ /// Corresponds to `#[api_version(X)]` attribute.
+ pub custom: Option<u64>,
+ /// Corresponds to `#[cfg_attr(feature = "enable-staging-api", api_version(99))]`
+ /// attribute. `String` is the feature name, `u64` the staging api version.
+ pub feature_gated: Option<(String, u64)>,
+}
+
// Extracts the value of `API_VERSION_ATTRIBUTE` and handles errors.
// Returns:
// - Err if the version is malformed
-// - Some(u64) if the version is set
-// - None if the version is not set (this is valid).
-fn extract_api_version(attrs: &Vec<Attribute>, span: Span) -> Result<Option<u64>> {
+// - `ApiVersion` on success. If a version is set or not is determined by the fields of `ApiVersion`
+fn extract_api_version(attrs: &Vec<Attribute>, span: Span) -> Result<ApiVersion> {
// First fetch all `API_VERSION_ATTRIBUTE` values (should be only one)
let api_ver = attrs
.iter()
@@ -782,7 +926,10 @@ fn extract_api_version(attrs: &Vec<Attribute>, span: Span) -> Result<Option<u64>
}
// Parse the runtime version if there exists one.
- api_ver.first().map(|v| parse_runtime_api_version(v)).transpose()
+ Ok(ApiVersion {
+ custom: api_ver.first().map(|v| parse_runtime_api_version(v)).transpose()?,
+ feature_gated: extract_cfg_api_version(attrs, span)?,
+ })
}
#[cfg(test)]
diff --git a/substrate/primitives/api/src/lib.rs b/substrate/primitives/api/src/lib.rs
index e575f6b9bbf..c3f80acf09a 100644
--- a/substrate/primitives/api/src/lib.rs
+++ b/substrate/primitives/api/src/lib.rs
@@ -344,6 +344,54 @@ pub use sp_api_proc_macro::decl_runtime_apis;
/// ```
/// In this case `Balance` api version 3 is being implemented for `Runtime`. The `impl` block
/// must contain all methods declared in version 3 and below.
+///
+/// # Conditional version implementation
+///
+/// `impl_runtime_apis!` supports `cfg_attr` attribute for conditional compilation. For example
+/// let's say you want to implement a staging version of the runtime api and put it behind a
+/// feature flag. You can do it this way:
+/// ```ignore
+/// pub struct Runtime {}
+/// sp_api::decl_runtime_apis! {
+/// pub trait ApiWithStagingMethod {
+/// fn stable_one(data: u64);
+///
+/// #[api_version(99)]
+/// fn staging_one();
+/// }
+/// }
+///
+/// sp_api::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() {}
+/// }
+/// }
+/// ```
+///
+/// [`decl_runtime_apis!`] declares two version of the api - 1 (the default one, which is
+/// considered stable in our example) and 99 (which is considered staging). In
+/// `impl_runtime_apis!` a `cfg_attr` attribute is attached to the `ApiWithStagingMethod`
+/// implementation. If the code is compiled with `enable-staging-api` feature a version 99 of
+/// the runtime api will be built which will include `staging_one`. Note that `staging_one`
+/// implementation is feature gated by `#[cfg(feature = ... )]` attribute.
+///
+/// If the code is compiled without `enable-staging-api` version 1 (the default one) will be
+/// built which doesn't include `staging_one`.
+///
+/// `cfg_attr` can also be used together with `api_version`. For the next snippet will build
+/// version 99 if `enable-staging-api` is enabled and version 2 otherwise because both
+/// `cfg_attr` and `api_version` are attached to the impl block:
+/// ```ignore
+/// #[cfg_attr(feature = "enable-staging-api", api_version(99))]
+/// #[api_version(2)]
+/// impl self::ApiWithStagingAndVersionedMethods<Block> for Runtime {
+/// // impl skipped
+/// }
+/// ```
pub use sp_api_proc_macro::impl_runtime_apis;
/// Mocks given trait implementations as runtime apis.
diff --git a/substrate/primitives/api/test/Cargo.toml b/substrate/primitives/api/test/Cargo.toml
index 87c5eb6199e..48eb067e4e4 100644
--- a/substrate/primitives/api/test/Cargo.toml
+++ b/substrate/primitives/api/test/Cargo.toml
@@ -35,3 +35,6 @@ static_assertions = "1.1.0"
[[bench]]
name = "bench"
harness = false
+
+[features]
+"enable-staging-api" = []
diff --git a/substrate/primitives/api/test/tests/decl_and_impl.rs b/substrate/primitives/api/test/tests/decl_and_impl.rs
index 274f80bd1b4..6e895680e41 100644
--- a/substrate/primitives/api/test/tests/decl_and_impl.rs
+++ b/substrate/primitives/api/test/tests/decl_and_impl.rs
@@ -50,6 +50,29 @@ decl_runtime_apis! {
#[api_version(4)]
fn glory_one();
}
+
+ pub trait ApiWithStagingMethod {
+ fn stable_one(data: u64);
+ #[api_version(99)]
+ fn staging_one();
+ }
+
+ pub trait ApiWithStagingAndVersionedMethods {
+ fn stable_one(data: u64);
+ #[api_version(2)]
+ fn new_one();
+ #[api_version(99)]
+ fn staging_one();
+ }
+
+ #[api_version(2)]
+ pub trait ApiWithStagingAndChangedBase {
+ fn stable_one(data: u64);
+ fn new_one();
+ #[api_version(99)]
+ fn staging_one();
+ }
+
}
impl_runtime_apis! {
@@ -82,6 +105,33 @@ impl_runtime_apis! {
fn new_one() {}
}
+ #[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() { }
+ }
+
+ #[cfg_attr(feature = "enable-staging-api", api_version(99))]
+ #[api_version(2)]
+ impl self::ApiWithStagingAndVersionedMethods<Block> for Runtime {
+ fn stable_one(_: u64) {}
+ fn new_one() {}
+
+ #[cfg(feature = "enable-staging-api")]
+ fn staging_one() {}
+ }
+
+ #[cfg_attr(feature = "enable-staging-api", api_version(99))]
+ impl self::ApiWithStagingAndChangedBase<Block> for Runtime {
+ fn stable_one(_: u64) {}
+ fn new_one() {}
+
+ #[cfg(feature = "enable-staging-api")]
+ fn staging_one() {}
+ }
+
impl sp_api::Core<Block> for Runtime {
fn version() -> sp_version::RuntimeVersion {
unimplemented!()
@@ -179,12 +229,38 @@ fn check_runtime_api_info() {
// The stable version of the API
assert_eq!(<dyn ApiWithMultipleVersions::<Block>>::VERSION, 2);
+
+ assert_eq!(<dyn ApiWithStagingMethod::<Block>>::VERSION, 1);
+ assert_eq!(<dyn ApiWithStagingAndVersionedMethods::<Block>>::VERSION, 1);
+ assert_eq!(<dyn ApiWithStagingAndChangedBase::<Block>>::VERSION, 2);
}
fn check_runtime_api_versions_contains<T: RuntimeApiInfo + ?Sized>() {
assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, T::VERSION)));
}
+fn check_staging_runtime_api_versions<T: RuntimeApiInfo + ?Sized>(_staging_ver: u32) {
+ // Staging APIs should contain staging version if the feature is set...
+ #[cfg(feature = "enable-staging-api")]
+ assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, _staging_ver)));
+ //... otherwise the base version should be set
+ #[cfg(not(feature = "enable-staging-api"))]
+ check_runtime_api_versions_contains::<dyn ApiWithStagingMethod<Block>>();
+}
+
+#[allow(unused_assignments)]
+fn check_staging_multiver_runtime_api_versions<T: RuntimeApiInfo + ?Sized>(
+ _staging_ver: u32,
+ _stable_ver: u32,
+) {
+ // Staging APIs should contain staging version if the feature is set...
+ #[cfg(feature = "enable-staging-api")]
+ assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, _staging_ver)));
+ //... otherwise the base version should be set
+ #[cfg(not(feature = "enable-staging-api"))]
+ assert!(RUNTIME_API_VERSIONS.iter().any(|v| v == &(T::ID, _stable_ver)));
+}
+
#[test]
fn check_runtime_api_versions() {
check_runtime_api_versions_contains::<dyn Api<Block>>();
@@ -192,6 +268,13 @@ fn check_runtime_api_versions() {
assert!(RUNTIME_API_VERSIONS
.iter()
.any(|v| v == &(<dyn ApiWithMultipleVersions<Block>>::ID, 3)));
+
+ check_staging_runtime_api_versions::<dyn ApiWithStagingMethod<Block>>(99);
+ check_staging_multiver_runtime_api_versions::<dyn ApiWithStagingAndVersionedMethods<Block>>(
+ 99, 2,
+ );
+ check_staging_runtime_api_versions::<dyn ApiWithStagingAndChangedBase<Block>>(99);
+
check_runtime_api_versions_contains::<dyn sp_api::Core<Block>>();
}
diff --git a/substrate/scripts/ci/gitlab/pipeline/test.yml b/substrate/scripts/ci/gitlab/pipeline/test.yml
index d986fa34022..ab294ccb436 100644
--- a/substrate/scripts/ci/gitlab/pipeline/test.yml
+++ b/substrate/scripts/ci/gitlab/pipeline/test.yml
@@ -237,6 +237,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
+ - time cargo nextest run -p sp-api-test --features enable-staging-api
# we need to update cache only from one job
- if [ ${CI_NODE_INDEX} == 1 ]; then rusty-cachier cache upload; fi
# Upload tests results to Elasticsearch
--
GitLab