Fix nostd build of several crates #4060

ggwpez · 2024-04-09T22:10:18Z

Preparation for #3935

Changes:

Add some default-features = false for the case that a crate and that dependency both support nostd builds.
Shuffle files around of some benchmarking-only crates. These conditionally disabled the cfg_attr for nostd and pulled in libstd. Example here. The actual logic is moved into a inner.rs to preserve nostd capability of the crate in case the benchmarking feature is disabled.
Add some use sp_std::vec where needed.
Remove some optional = true in cases where it was not optional.
Removed one superfluous cfg_attr(not(feature = "std"), no_std...

All in all this should be logical no-op.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Can be tried with v1.4 `zepter lint no-std default-features-of-nostd-dependencies-disabled --fix` Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

I dont know why this is needed, but otherwise it will not build with the error of the pallet being unknown. Maybe the construct_runtime! syntax does not actually support feature gates. Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez · 2024-04-09T22:11:55Z

cumulus/parachains/runtimes/assets/asset-hub-rococo/Cargo.toml

@@ -36,7 +36,7 @@ pallet-nfts = { path = "../../../../../substrate/frame/nfts", default-features =
 pallet-nfts-runtime-api = { path = "../../../../../substrate/frame/nfts/runtime-api", default-features = false }
 pallet-proxy = { path = "../../../../../substrate/frame/proxy", default-features = false }
 pallet-session = { path = "../../../../../substrate/frame/session", default-features = false }
-pallet-state-trie-migration = { path = "../../../../../substrate/frame/state-trie-migration", default-features = false, optional = true }
+pallet-state-trie-migration = { path = "../../../../../substrate/frame/state-trie-migration", default-features = false }


This should still be gated by the construct_runtime! feature gate. But i think that is not actually working, otherwise this change would not be needed 🤷‍♂️

What?

Generally we don't need this anymore and the state migration should already have been finished.

It is still deployed though, will double-check and remove it then.

Uhf, did we really genesis the asset-hub-rococo with state_version 0?
I just queried Core::version of the 1.1 runtime and it returns state_version=0...

Looks like it was copy&pasted from AHK. @cheme can you double-check please?

Ouch https://github.com/polkadot-fellows/runtimes/blob/31ba26287ec752574244f0d690167f7ae8430c8b/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L153 . (kusuma is fine).
Statemint on rococo (I guess it is now assethub, not sure) was migrated IIRC or was it only on westend cc @PierreBesson ?

I meant it here for the Asset-hub-rococo:

polkadot-sdk/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

Line 110 in 5601f28

#[cfg(feature = "state-trie-version-1")]

Not sure, since when i queried the runtime api it returned version 0. Maybe we can check with the RPC but the public node does not support it.
PS: Can you maybe create an issue to track the migration of the different runtimes? Or it already exists?

maybe polkadot-fellows/runtimes#74

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

prdoc/pr_4060.prdoc

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

substrate/frame/contracts/Cargo.toml

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

bkchr · 2024-04-15T07:53:31Z

cumulus/parachains/runtimes/assets/asset-hub-rococo/Cargo.toml

@@ -36,7 +36,7 @@ pallet-nfts = { path = "../../../../../substrate/frame/nfts", default-features =
 pallet-nfts-runtime-api = { path = "../../../../../substrate/frame/nfts/runtime-api", default-features = false }
 pallet-proxy = { path = "../../../../../substrate/frame/proxy", default-features = false }
 pallet-session = { path = "../../../../../substrate/frame/session", default-features = false }
-pallet-state-trie-migration = { path = "../../../../../substrate/frame/state-trie-migration", default-features = false, optional = true }
+pallet-state-trie-migration = { path = "../../../../../substrate/frame/state-trie-migration", default-features = false }


What?

Generally we don't need this anymore and the state migration should already have been finished.

substrate/frame/root-offences/Cargo.toml

bkchr · 2024-04-15T08:06:09Z

substrate/frame/try-runtime/src/lib.rs

@@ -18,36 +18,9 @@
 //! Supporting types for try-runtime, testing and dry-running commands.

 #![cfg_attr(not(feature = "std"), no_std)]
-#![cfg(feature = "try-runtime")]


It makes no sense that this crate has a try-runtime feature.. This crate should only be activated if you use try-runtime.

Then it would enable the try-runtime feature of all dependencies by default?
I dont remember if this can cause issues with feature unification, but maybe it works since the crate would disabled otherwise.

Okay actually this will make --workspace not compile anymore, since it enables all crates. You can check it with cargo tree --workspace and see that frame-system-benchmarking is enabled at the top-level @bkchr.

Hmm yeah okay... 🦭

Maybe having the try_runtime types in frame-support not behind a feature would be a better solution.

We have some traits with feature gated functions. Maybe that is an anti-pattern in itself... should probably be an extension trait that is only selectively required.

I mean the functions is fine, I would probably just not hide the traits etc in try_runtime.rs. But in the end this is all discussable and we can keep it like it is done right now.

Generally just don't like these inner.rs files.

bkchr · 2024-04-15T08:07:03Z

substrate/frame/system/benchmarking/src/lib.rs


 #![cfg_attr(not(feature = "std"), no_std)]
-#![cfg(feature = "runtime-benchmarks")]


Same. This crate should only be activated when you are compiling runtime benchmarks.

bkchr · 2024-04-15T08:09:47Z

substrate/frame/src/lib.rs

+// NOTE that we put the actual code into another module to keep the no-std capability of this create
+// even when the `experimental` feature is disabled.


I don't get this comment. Also generally it doesn't make sense that the experimental feature disables everything of this crate.

What is the purpose of this feature then? Just don't use the crate if you don't want to use experimental stuff.

Okay i reverted the changes to the FRAME crate. Apparently it is not published anyway - so should not be needed for the umbrella crate until then.
Also increased the warning in the docs. I could not rename it since the name is used in some crate-access generation macros and repeated often in docs... can fix in the future once we start publishing.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

bkchr

Looks good % https://github.com/paritytech/polkadot-sdk/pull/4060/files#r1565349040

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

cheme · 2024-04-17T11:14:23Z

cumulus/parachains/runtimes/assets/asset-hub-rococo/Cargo.toml

-# This pallet will migrate the entire state, controlled through some account.
-#
-# This feature should be removed when the main-net will be migrated.
-state-trie-version-1 = []


oh now I remembre, this is used for multiple network so the feature was needed to have one running V0 while another one was already migrated (likely westend/rococo a long time ago).
Sounds ok to remove it so migration got started everywhere. One point I am not sure is related to the account use as controller: currently it is hardcoded to :

pub const MigController: AccountId = AccountId::from(hex_literal::hex!("8458ed39dc4b6f6c7255f7bc42be50c2967db126357c999d44e12ca7ac80dc52")); pub const RootMigController: AccountId = AccountId::from(hex_literal::hex!("8458ed39dc4b6f6c7255f7bc42be50c2967db126357c999d44e12ca7ac80dc52"));

probably would need a different one per network, this is not really convenient.
Worst case, what can happen with this chain is some parachain moving to hybrid mode (both v0 and v1 in tree, if warpsync is not use by the parachain this should not be an issue) and not being able to migrate because no account to process the typescript driven migration.

Okay thanks for the context. For Rococo it is probably fine, as it will be decommissioned soon anyway.
Just going to leave it as is, and hope that DevOps has the keys. Otherwise we do another upgrade or root call etc.

I confirm we have the key to 0x8458ed39dc4b6f6c7255f7bc42be50c2967db126357c999d44e12ca7ac80dc52 to be able to process the migration.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Preparation for #3935 Changes: - Add some `default-features = false` for the case that a crate and that dependency both support nostd builds. - Shuffle files around of some benchmarking-only crates. These conditionally disabled the `cfg_attr` for nostd and pulled in libstd. Example [here](ggwpez/zepter#95). The actual logic is moved into a `inner.rs` to preserve nostd capability of the crate in case the benchmarking feature is disabled. - Add some `use sp_std::vec` where needed. - Remove some `optional = true` in cases where it was not optional. - Removed one superfluous `cfg_attr(not(feature = "std"), no_std..`. All in all this should be logical no-op. --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez added 8 commits April 9, 2024 17:51

Remove wrong nostd

fc2869f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Zepter: make nostd-deps optional

15a6851

Can be tried with v1.4 `zepter lint no-std default-features-of-nostd-dependencies-disabled --fix` Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Always enable scale feature

dd6b1a5

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Remove optional: these pallets are not optional at all

db09175

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Add use sp_std::vec stuff

a17f95e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix benchmarking crate

b18244a

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix docs

58c424d

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez requested review from athei and a team as code owners April 9, 2024 22:10

ggwpez commented Apr 9, 2024

View reviewed changes

ggwpez added R0-silent Changes should not be mentioned in any release notes T2-pallets This PR/Issue is related to a particular pallet. and removed R0-silent Changes should not be mentioned in any release notes labels Apr 9, 2024

add prdoc

8155936

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez commented Apr 9, 2024

View reviewed changes

prdoc/pr_4060.prdoc Show resolved Hide resolved

Fix feature gates

dddf1b0

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

liamaharon approved these changes Apr 10, 2024

View reviewed changes

ggwpez added 4 commits April 10, 2024 08:23

Fix feature gate

0ee158e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix imports

430868e

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Also fix crates FRAME and frame-try-runtime

1578135

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix more

4b8a29c

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

athei reviewed Apr 10, 2024

View reviewed changes

substrate/frame/contracts/Cargo.toml Outdated Show resolved Hide resolved

ggwpez added 2 commits April 10, 2024 10:06

Revert uapi feature changes

06e8c43

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge branch 'master' into oty-fix-nostd

a1cc2b1

bkchr reviewed Apr 15, 2024

View reviewed changes

ggwpez added 2 commits April 16, 2024 20:14

Use alloc instead of sp-std

83f5967

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ah-rococo: Enable V1 state trie migration

cecb8f4

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

bkchr approved these changes Apr 17, 2024

View reviewed changes

Revert changes to FRAME crate

c70875c

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

cheme reviewed Apr 17, 2024

View reviewed changes

ggwpez added 4 commits April 17, 2024 14:21

FRAME crate: larger experimental warning

0d32f27

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge remote-tracking branch 'origin/master' into oty-fix-nostd

54552f3

Merge remote-tracking branch 'origin/master' into oty-fix-nostd

3d764a5

Fix features

3a2f964

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez added this pull request to the merge queue Apr 17, 2024

Merged via the queue into master with commit 7a2c9d4 Apr 17, 2024
132 of 135 checks passed

ggwpez deleted the oty-fix-nostd branch April 17, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nostd build of several crates #4060

Fix nostd build of several crates #4060

ggwpez commented Apr 9, 2024 •

edited

Loading

ggwpez Apr 9, 2024

bkchr Apr 15, 2024

ggwpez Apr 15, 2024 •

edited

Loading

ggwpez Apr 16, 2024

cheme Apr 16, 2024

ggwpez Apr 16, 2024 •

edited

Loading

cheme Apr 17, 2024

bkchr Apr 15, 2024

bkchr Apr 15, 2024

ggwpez Apr 15, 2024

ggwpez Apr 15, 2024 •

edited

Loading

bkchr Apr 16, 2024

ggwpez Apr 16, 2024

bkchr Apr 17, 2024

bkchr Apr 17, 2024

bkchr Apr 15, 2024

bkchr Apr 15, 2024

ggwpez Apr 17, 2024

bkchr left a comment

cheme Apr 17, 2024

ggwpez Apr 17, 2024 •

edited

Loading

PierreBesson Apr 17, 2024


		#![cfg_attr(not(feature = "std"), no_std)]
		#![cfg(feature = "runtime-benchmarks")]

		// NOTE that we put the actual code into another module to keep the no-std capability of this create
		// even when the `experimental` feature is disabled.

Fix nostd build of several crates #4060

Fix nostd build of several crates #4060

Conversation

ggwpez commented Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez commented Apr 9, 2024 •

edited

Loading

ggwpez Apr 15, 2024 •

edited

Loading

ggwpez Apr 16, 2024 •

edited

Loading

ggwpez Apr 15, 2024 •

edited

Loading

ggwpez Apr 17, 2024 •

edited

Loading